[webkit-reviews] review denied: [Bug 136322] [SOUP] WebKitDownload cannot overwrite existing file : [Attachment 237352] 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: [SOUP] WebKitDownload cannot overwrite existing file
https://bugs.webkit.org/show_bug.cgi?id=136322

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

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
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.