[Webkit-unassigned] [Bug 116150] [Curl] Unable to download files.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=116150


Brent Fulgham <bfulgham at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #203200|review?                     |review-
               Flag|                            |




--- Comment #4 from Brent Fulgham <bfulgham at webkit.org>  2013-05-30 22:39:40 PST ---
(From update of attachment 203200)
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"

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.