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/
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.
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 <alexl@redhat.com>. * webkit/webkitdownload.cpp: (webkit_download_open_stream_for_uri): Save the download-uri as file metadata.
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.
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.
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.
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.
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.
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.
(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.
Created attachment 166465 [details] patch updated, async - Use async API.
(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
Comment on attachment 166465 [details] patch updated, async Thanks!
Comment on attachment 166465 [details] patch updated, async Clearing flags on attachment: 166465 Committed r130046: <http://trac.webkit.org/changeset/130046>
All reviewed patches have been landed. Closing bug.
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
(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
(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