Bug 116150 – [Curl] Unable to download files.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

For Curl, the WebDownload class (WebDownloadCurl.cpp) is not implemented.

Created attachment 201829 [details]
Patch

Created attachment 203200 [details]
Patch

Modified the patch slightly by including a crash fix, and creating a new header file.

[Brent Fulgham]

Comment on attachment 203200 [details]
Patch

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"

Created attachment 203435 [details]
Patch

(In reply to comment #4)
> (From update of attachment 203200 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=203200&action=review
> 

Thanks for reviewing! I updated the patch according to your comments.
The files CurlDownload.h/cpp were added to the WebCore/platform/network/curl folder.

[Brent Fulgham]

Comment on attachment 203435 [details]
Patch

Looks great!  Thanks for making the changes.

[WebKit Commit Bot]

Comment on attachment 203435 [details]
Patch

Clearing flags on attachment: 203435

Committed r151067: <http://trac.webkit.org/changeset/151067>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.