Bug 24786 - WebKitDownload sometimes suggests peculiar filenames
Summary: WebKitDownload sometimes suggests peculiar filenames
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2009-03-24 14:44 PDT by Christian Dywan
Modified: 2009-05-06 12:39 PDT (History)
2 users (show)

See Also:


Attachments
Better handling of the URI that is to be downloaded to figure out a (1.92 KB, patch)
2009-04-01 14:01 PDT, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
Refactor the emission of the download-requested signal so that we have less code duplication. (4.90 KB, patch)
2009-04-04 12:10 PDT, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
When a download is requested by an ongoing request, use the already (6.21 KB, patch)
2009-04-04 12:10 PDT, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
When a download is requested by an ongoing request, use the already (6.34 KB, patch)
2009-04-04 12:14 PDT, Gustavo Noronha (kov)
xan.lopez: review+
Details | Formatted Diff | Diff
refactor download-requested signal emission to avoid code duplication (5.51 KB, patch)
2009-04-20 10:59 PDT, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Christian Dywan 2009-03-24 14:44:03 PDT
WebKitDownload provides a "suggested filename". This works often enough but sometimes it suggests something like "jdk-6u13-windows-i586-p.exe&File=jdk-6u13-windows-i586-p.exe" which is obviously not ideal.
Another phenomenon seems to be URI encoded spaces, ie. %20, which is also not useful.

I think we should try to catch these cases.
Comment 1 Gustavo Noronha (kov) 2009-04-01 14:01:44 PDT
Created attachment 29176 [details]
Better handling of the URI that is to be downloaded to figure out a

name suggestion.
---
 WebKit/gtk/ChangeLog                 |   13 +++++++++++++
 WebKit/gtk/webkit/webkitdownload.cpp |    6 +++++-
 2 files changed, 18 insertions(+), 1 deletions(-)
Comment 2 Gustavo Noronha (kov) 2009-04-01 14:04:49 PDT
Comment on attachment 29176 [details]
Better handling of the URI that is to be downloaded to figure out a

KURL has all we need =). We can even use the KURL the request itself holds in the future, when we have a proper WebKitNetworkRequest implementation committed =).
Comment 3 Gustavo Noronha (kov) 2009-04-04 12:10:35 PDT
Created attachment 29254 [details]
Refactor the emission of the download-requested signal so that we have less code duplication.

 WebKit/gtk/ChangeLog                               |   17 +++++++++++++++++
 WebKit/gtk/WebCoreSupport/ContextMenuClientGtk.cpp |   16 +++-------------
 WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp |   14 ++------------
 WebKit/gtk/webkit/webkitprivate.h                  |    4 ++++
 WebKit/gtk/webkit/webkitwebview.cpp                |   18 ++++++++++++++++++
 5 files changed, 44 insertions(+), 25 deletions(-)
Comment 4 Gustavo Noronha (kov) 2009-04-04 12:10:40 PDT
Created attachment 29255 [details]
When a download is requested by an ongoing request, use the already

provided response to set the suggested filename for the WebKitDownload
object, if available.
---
 WebKit/gtk/ChangeLog                               |   20 ++++++++++++++++++
 WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp |    9 ++++++-
 WebKit/gtk/webkit/webkitdownload.cpp               |   22 ++++++++++++++-----
 WebKit/gtk/webkit/webkitprivate.h                  |    5 +++-
 WebKit/gtk/webkit/webkitwebview.cpp                |    7 +++++-
 5 files changed, 53 insertions(+), 10 deletions(-)
Comment 5 Gustavo Noronha (kov) 2009-04-04 12:14:15 PDT
Created attachment 29257 [details]
When a download is requested by an ongoing request, use the already

provided response to set the suggested filename for the WebKitDownload
object, if available.
---
 WebKit/gtk/ChangeLog                               |   20 +++++++++++++++++
 WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp |    9 ++++++-
 WebKit/gtk/webkit/webkitdownload.cpp               |   23 +++++++++++++------
 WebKit/gtk/webkit/webkitprivate.h                  |    5 +++-
 WebKit/gtk/webkit/webkitwebview.cpp                |    7 +++++-
 5 files changed, 53 insertions(+), 11 deletions(-)
Comment 6 Xan Lopez 2009-04-20 08:25:51 PDT
Comment on attachment 29176 [details]
Better handling of the URI that is to be downloaded to figure out a

Looks good to me. Maybe you can briefly say in the changelog how is it better than before (no query, reference, escaping, etc).
Comment 7 Xan Lopez 2009-04-20 09:03:34 PDT
Comment on attachment 29254 [details]
Refactor the emission of the download-requested signal so that we have less code duplication.

>  void ContextMenuClient::downloadURL(const KURL& url)
>  {
> -    WebKitNetworkRequest* network_request = webkit_network_request_new(url.string().utf8().data());
> -    WebKitDownload* download = webkit_download_new(network_request);
> -    g_object_unref(network_request);
> +    WebKitNetworkRequest* networkRequest = webkit_network_request_new(url.string().utf8().data());
>  
> -    gboolean handled;
> -    g_signal_emit_by_name(m_webView, "download-requested", download, &handled);
> -
> -    if (!handled) {
> -        webkit_download_cancel(download);
> -        g_object_unref(download);
> -        return;
> -    }
> -
> -    webkit_download_start(download);
> +    webkit_web_view_request_download(m_webView, networkRequest);
> +    g_object_unref(networkRequest);
>  }


> +void webkit_web_view_request_download(WebKitWebView* webView, WebKitNetworkRequest* request)
> +{
> +    g_return_if_fail(WEBKIT_IS_WEB_VIEW(webView));
> +
> +    WebKitDownload* download = webkit_download_new(request);
> +    g_object_unref(request);
> +


Mmm, aren't you unrefing the network request twice now? Once in the request_download function, and once in the functions called that.
Comment 8 Gustavo Noronha (kov) 2009-04-20 10:17:08 PDT
Comment on attachment 29176 [details]
Better handling of the URI that is to be downloaded to figure out a

Landed as r42672 with a more descriptive changelog message.
Comment 9 Gustavo Noronha (kov) 2009-04-20 10:59:11 PDT
Created attachment 29617 [details]
refactor download-requested signal emission to avoid code duplication

Xan's right, I'm not sure why I didn't get any crashers while testing, though I don't remember how/if I tested doing the actual downloads.
Comment 10 Xan Lopez 2009-04-25 00:31:06 PDT
Comment on attachment 29617 [details]
refactor download-requested signal emission to avoid code duplication

Go for it.
Comment 11 Gustavo Noronha (kov) 2009-04-25 07:04:51 PDT
Comment on attachment 29617 [details]
refactor download-requested signal emission to avoid code duplication

Landed as r42867.
Comment 12 Xan Lopez 2009-05-06 07:23:15 PDT
Comment on attachment 29257 [details]
When a download is requested by an ongoing request, use the already

> +    if (!response.isNull() && !response.suggestedFilename().isEmpty()) {
> +        g_debug("suggested filename exists!");
> +        webkit_download_set_suggested_filename(download, response.suggestedFilename().utf8().data());
> +    }
> +

I guess you want to lose the g_debug. Looks good otherwise.
Comment 13 Gustavo Noronha (kov) 2009-05-06 12:39:46 PDT
Last patch landed as r43319.