[webkit-reviews] review denied: [Bug 136322] [GTK] WebKitDownload cannot overwrite existing file : [Attachment 237271] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Michael Catanzaro
<mcatanzaro at gnome.org>'s request for review:
Bug 136322: [GTK] WebKitDownload cannot overwrite existing file
https://bugs.webkit.org/show_bug.cgi?id=136322

Attachment 237271: Patch
https://bugs.webkit.org/attachment.cgi?id=237271&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=237271&action=review


Doing this unconditionally might be dangerous. I think it might make sense to
allow overwrite when the destination has been set by the application
explicitly, since apps are supposed to handle that situation when deciding the
destination name, but we should document that. When the destination is decided
automatically by WebKit, I'm not sure sure it's a good idea to overwrite
existing file, though. The safest way would be to add API for that something
like webkit_download_set_allow_overwrite()

> Source/WebKit2/ChangeLog:3
> +	   [GTK] WebKitDownload cannot overwrite existing file

This is not a GTK specific issue, it affects all ports using libsoup as network
backend.

> Source/WebKit2/Shared/Downloads/soup/DownloadSoup.cpp:140
> -	   if (!g_file_move(m_intermediateFile.get(), destinationFile.get(),
G_FILE_COPY_NONE, nullptr, nullptr, nullptr, &error.outPtr())) {
> +	   if (!g_file_move(m_intermediateFile.get(), destinationFile.get(),
G_FILE_COPY_OVERWRITE, nullptr, nullptr, nullptr, &error.outPtr())) {

I think this should be done using the allowOverwrite parameter of
DecideDestinationWithSuggestedFilename message, so that clients can decide and
it won't affect EFL in this case.