[webkit-reviews] review denied: [Bug 116150] [Curl] Unable to download files. : [Attachment 203200] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied peavo at outlook.com's request for
review:
Bug 116150: [Curl] Unable to download files.
https://bugs.webkit.org/show_bug.cgi?id=116150

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

------- Additional Comments from Brent Fulgham <bfulgham at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=203200&action=review


Very nice!  I had a few minor style/nit-picky comments, but this looks great. 
I do feel that we should place the "CurlDownload" class should be in its own
set of files.  It might make sense for the CurlDownload class to live in
WebCore (platform/network/curl).

> Source/WebKit/win/ChangeLog:11
> +	   (CurlDownloadManager):

I would get rid of all of these method call-outs.  The whole file is new, so no
point listing them individually.

> Source/WebKit/win/ChangeLog:33
> +	   (CurlDownloadManager::CurlDownloadManager):

I think this should be in its own file.  I know this means we touch the project
files, but I think putting them in the same file is worse.

> Source/WebKit/win/WebDownloadCurl.cpp:208
> +// CurlDownloadManager
-------------------------------------------------------------------

This should be in its own file.

> Source/WebKit/win/WebDownloadCurl.cpp:458
> +    if (!m_destination.isEmpty())

Preference: I really prefer to handle the "no work" case as an early return.  I
know this method only has one line, but in my mind early return clarifies that
if the destination file name is empty, we do nothing.  As it's written, it's
clear that we don't move the file when the name is empty, but it leaves open
the possibility that other work might be done in this method if the file name
was empty.

> Source/WebKit/win/WebDownloadCurl.cpp:459
> +	   MoveFile(m_tempPath.charactersWithNullTermination(),
m_destination.charactersWithNullTermination());

Nit: We try to expose Win API calls with the global namespace, i.e.,
"::MoveFile"