Bug 24786 – WebKitDownload sometimes suggests peculiar filenames

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Christian Dywan]

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.

[Gustavo Noronha (kov)]

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(-)

[Gustavo Noronha (kov)]

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 =).

[Gustavo Noronha (kov)]

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(-)

[Gustavo Noronha (kov)]

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(-)

[Gustavo Noronha (kov)]

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(-)

[Xan Lopez]

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).

[Xan Lopez]

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.

[Gustavo Noronha (kov)]

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.

[Gustavo Noronha (kov)]

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.

[Xan Lopez]

Comment on attachment 29617 [details]
refactor download-requested signal emission to avoid code duplication

Go for it.

[Gustavo Noronha (kov)]

Comment on attachment 29617 [details]
refactor download-requested signal emission to avoid code duplication

Landed as r42867.

[Xan Lopez]

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.

[Gustavo Noronha (kov)]

Last patch landed as r43319.