[webkit-reviews] review granted: [Bug 136423] [SOUP] Race condition when downloading a file due to the intermediate temporary file : [Attachment 237459] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has granted Michael Catanzaro
<mcatanzaro at gnome.org>'s request for review:
Bug 136423: [SOUP] Race condition when downloading a file due to the
intermediate temporary file
https://bugs.webkit.org/show_bug.cgi?id=136423

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

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


Looks good to me in general, I have just a few comments

> Source/WebKit2/Shared/Downloads/soup/DownloadSoup.cpp:67
> +	   if (m_createdDestination) {
> +	       ASSERT(m_destinationFile);
> +	       g_file_delete(m_destinationFile.get(), nullptr, nullptr);
> +	   }

I think we can use m_destinationFile to know if the destination was
successfully created.

> Source/WebKit2/Shared/Downloads/soup/DownloadSoup.cpp:115
> +	       outputStream = adoptGRef(g_file_replace(m_destinationFile.get(),
nullptr, false, G_FILE_CREATE_NONE, nullptr, &error.outPtr()));

g_file_replace expects a gboolean, so better use FALSE, instead of false, even
though it doesn't really matter.

> Source/WebKit2/Shared/Downloads/soup/DownloadSoup.cpp:121
> +	   if (!outputStream) {
> +	       downloadFailed(platformDownloadDestinationError(response,
error->message));
> +	       return;
> +	   }

And here you could m_destinationFile.reset() and you don't need the
m_createdDestination bool member