Bug 95188 – [GTK] Save original uri for downloaded files

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Alexander Larsson]

Created attachment 160948 [details]
Implement saving download-uri in file metadata

It would be very useful if epiphany and all webkit apps would
store the original url as metadata for downloaded files.

I've implemented this in webkit, patch applied. After downloading a file with
this i can then do:

$ gvfs-info -a "metadata::*" ~/ai.php 
attributes:
  metadata::download-uri:
http://rotation.linuxnewmedia.com/www/delivery/ai.php?filename=sm_uso_20120726_x9_onlinemaster_adseries_336x280_idfsf_5workstation_2.jpg&contenttype=jpeg
  metadata::gedit-position: 0

GVfs stores file metadata in a set of journaled mmaped files in $home/.local/share-gvfs-metadata/

[Claudio Saavedra]

We probably need to add the WK2 counterpart, test cases, and proper ChangeLogs. If we agree with doing this I can take it from here.

[Claudio Saavedra]

Created attachment 166431 [details]
        [GTK] Save original uri for downloaded files         https://bugs.webkit.org/show_bug.cgi?id=95188

Reviewed by NOBODY (OOPS!).

        gvfs stores metadata locally, and this information can later be
        used by file management applications. Based on a patch by
        Alexander Larsson <[e-mail]>.

        * webkit/webkitdownload.cpp:
        (webkit_download_open_stream_for_uri): Save the download-uri as
        file metadata.

[Carlos Garcia Campos]

Comment on attachment 166431 [details]
        [GTK] Save original uri for downloaded files         https://bugs.webkit.org/show_bug.cgi?id=95188

View in context: https://bugs.webkit.org/attachment.cgi?id=166431&action=review

> Source/WebKit/gtk/webkit/webkitdownload.cpp:473
> -    g_object_unref(file);
> -
>      if (error) {
>          webkitDownloadEmitError(download, downloadDestinationError(core(priv->networkResponse), error->message));
>          g_error_free(error);
> +        g_object_unref(file);

Use GRefPtr and you don't need this.

> Source/WebKit/gtk/webkit/webkitdownload.cpp:478
> +    g_file_set_attribute(file, "metadata::download-uri", G_FILE_ATTRIBUTE_TYPE_STRING,
> +        (gpointer)webkit_network_request_get_uri(priv->networkRequest), G_FILE_QUERY_INFO_NONE, NULL, &error);

Why not using g_file_set_attribute_string()? so that you don't need the gpointer cast nor the attribute type parameter. Use 0 instead of NULL. We typically don't use that indentation, new line should be under the ( even if the style checker complains, I think it's a bug in the style checker script.

> Source/WebKit/gtk/webkit/webkitdownload.cpp:482
> +    if (error) {
> +        g_warning("Couldn't set downloaded file attribute: %s\n", error->message);
> +        g_error_free(error);

We could use a GOwnPtr for the error too. I'm not sure showing a warning here is useful, maybe we can just silently ignore the error, since it doesn't affect the download at all.

[Mario Sanchez Prada]

View in context: https://bugs.webkit.org/attachment.cgi?id=166431&action=review
> Source/WebKit/gtk/webkit/webkitdownload.cpp:478
> +        (gpointer)webkit_network_request_get_uri(priv->networkRequest), G_FILE_QUERY_INFO_NONE, NULL, &error);

Are you sure priv->networkRequest will never be 0? If not, I think you should add an early return, otherwise an ASSERT might be helpful.

[Claudio Saavedra]

Comment on attachment 166431 [details]
        [GTK] Save original uri for downloaded files         https://bugs.webkit.org/show_bug.cgi?id=95188

View in context: https://bugs.webkit.org/attachment.cgi?id=166431&action=review

>> Source/WebKit/gtk/webkit/webkitdownload.cpp:473
>> +        g_object_unref(file);
> 
> Use GRefPtr and you don't need this.

Fixed in a separate bug, already pushed.

>> Source/WebKit/gtk/webkit/webkitdownload.cpp:478
>> +        (gpointer)webkit_network_request_get_uri(priv->networkRequest), G_FILE_QUERY_INFO_NONE, NULL, &error);
> 
> Why not using g_file_set_attribute_string()? so that you don't need the gpointer cast nor the attribute type parameter. Use 0 instead of NULL. We typically don't use that indentation, new line should be under the ( even if the style checker complains, I think it's a bug in the style checker script.

OK, thanks.

>> Source/WebKit/gtk/webkit/webkitdownload.cpp:482
>> +        g_error_free(error);
> 
> We could use a GOwnPtr for the error too. I'm not sure showing a warning here is useful, maybe we can just silently ignore the error, since it doesn't affect the download at all.

You're right, I'll remove the warning.

[Claudio Saavedra]

Created attachment 166462 [details]
Reviewed version

- Updated, using GRefPtr now.
- Ignore the error.
- Use g_file_set_attribute_string() to avoid the need to cast.
- Use webkit_download_get_uri() instead of accessing directly the network request.

[WebKit Review Bot]

Attachment 166462 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1
Source/WebKit/gtk/webkit/webkitdownload.cpp:477:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Comment on attachment 166462 [details]
Reviewed version

I'm not sure we want to set the metadata if the download fails or is cancelled, I guess the destination file will be deleted anyway, so it would be harmless, but maybe it's better to do it in webkit_download_finished_loading() to not delay the download setting the metadata. Also, we could probably use the async API and simply ignore the result.

[Claudio Saavedra]

(In reply to comment #8)
> (From update of attachment 166462 [details])
> I'm not sure we want to set the metadata if the download fails or is cancelled, I guess the destination file will be deleted anyway, so it would be harmless, but maybe it's better to do it in webkit_download_finished_loading() to not delay the download setting the metadata. Also, we could probably use the async API and simply ignore the result.

I think we should do it immediately when starting the download, because even for partial downloads the metadata can be relevant. I will attach a patch using the async API though.

[Claudio Saavedra]

Created attachment 166465 [details]
patch updated, async

- Use async API.

[Carlos Garcia Campos]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 166462 [details] [details])
> > I'm not sure we want to set the metadata if the download fails or is cancelled, I guess the destination file will be deleted anyway, so it would be harmless, but maybe it's better to do it in webkit_download_finished_loading() to not delay the download setting the metadata. Also, we could probably use the async API and simply ignore the result.
> 
> I think we should do it immediately when starting the download, because even for partial downloads the metadata can be relevant. I will attach a patch using the async API though.

fair enough

[Carlos Garcia Campos]

Comment on attachment 166465 [details]
patch updated, async

Thanks!

[WebKit Review Bot]

Comment on attachment 166465 [details]
patch updated, async

Clearing flags on attachment: 166465

Committed r130046: <http://trac.webkit.org/changeset/130046>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Bastien Nocera]

In which version of WebKitGTK+ did this end up?

"attr -l" on a file downloaded using epiphany only shows an "selinux" attribute having been set.

webkitgtk3-2.0.4-1.fc19.x86_64
epiphany-3.8.2-1.fc19.x86_64

[Carlos Garcia Campos]

(In reply to comment #15)
> In which version of WebKitGTK+ did this end up?
> 
> "attr -l" on a file downloaded using epiphany only shows an "selinux" attribute having been set.
> 
> webkitgtk3-2.0.4-1.fc19.x86_64
> epiphany-3.8.2-1.fc19.x86_64

Works here:

$ gvfs-info -a "metadata::*" /tmp/bug-120404-20130830175701.patch
attributes:
  metadata::download-uri: https://bug-120404-attachments.webkit.org/attachment.cgi?id=210125

[Bastien Nocera]

(In reply to comment #16)
> Works here:
> 
> $ gvfs-info -a "metadata::*" /tmp/bug-120404-20130830175701.patch
> attributes:
>   metadata::download-uri: https://bug-120404-attachments.webkit.org/attachment.cgi?id=210125

Ha, it's not in xattr, but in gvfs's metadata side-loading database. Thanks