If the destination of a WebKitDownload is a file that already exists, the download will always fail. This is awkward because some browsers want to allow users to save a downloaded file over an existing file. For example, in Epiphany if you use the Save Image As feature (right click on an image) and try to save over a file that already exists, we have a dialog box to confirm that the user really wants to overwrite the file -> user confirms he wants to overwrite the file -> download fails with error message "file already exists."
Created attachment 237271 [details] Patch
Comment on attachment 237271 [details] Patch Not sure we unconditionally want to overwrite an existing file, we should be careful with destructive operations.
Comment on attachment 237271 [details] Patch 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.
So, I would split this patch. In this bug I would make sure we use the allowOverwrite parameter that we are currently ignoring, adding G_FILE_COPY_OVERWRITE flag when it's true (it will be false anyway, so there's no change in behaviour). And then I would open a new bug for the GTK port to decide how to handle overwrites in the GTK+ API.
Created attachment 237352 [details] Patch
Comment on attachment 237352 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=237352&action=review Thanks, I have a few minor comments. > Source/WebKit2/Shared/Downloads/soup/DownloadSoup.cpp:91 > - m_destinationURI = m_download->decideDestinationWithSuggestedFilename(suggestedFilename, overwrite); > + m_destinationURI = m_download->decideDestinationWithSuggestedFilename(suggestedFilename, m_overwriteDestination); Nit: I would use m_allowOverwrite for consistency with the IPC message param name. > 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(), flags, nullptr, nullptr, nullptr, &error.outPtr())) { I think you can add the if directly here, without the local variable if (!g_file_move(m_intermediateFile.get(), destinationFile.get(), m_allowOverwrite ? G_FILE_COPY_OVERWRITE : G_FILE_COPY_NONE, nullptr, nullptr, nullptr, &error.outPtr())) { > Source/WebKit2/Shared/Downloads/soup/DownloadSoup.cpp:200 > + bool m_overwriteDestination = false; Initialize the member in the constructor.
Created attachment 237430 [details] Patch
Comment on attachment 237430 [details] Patch Clearing flags on attachment: 237430 Committed r173154: <http://trac.webkit.org/changeset/173154>
All reviewed patches have been landed. Closing bug.