Bug 116150 - [Curl] Unable to download files.
Summary: [Curl] Unable to download files.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-15 06:38 PDT by peavo
Modified: 2013-05-31 22:41 PDT (History)
2 users (show)

See Also:


Attachments
Patch (20.20 KB, patch)
2013-05-15 07:34 PDT, peavo
no flags Details | Formatted Diff | Diff
Patch (22.34 KB, patch)
2013-05-29 07:16 PDT, peavo
no flags Details | Formatted Diff | Diff
Patch (26.48 KB, patch)
2013-05-31 04:44 PDT, peavo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description peavo 2013-05-15 06:38:01 PDT
For Curl, the WebDownload class (WebDownloadCurl.cpp) is not implemented.
Comment 1 peavo 2013-05-15 07:34:10 PDT
Created attachment 201829 [details]
Patch
Comment 2 peavo 2013-05-29 07:16:51 PDT
Created attachment 203200 [details]
Patch
Comment 3 peavo 2013-05-29 07:18:25 PDT
Modified the patch slightly by including a crash fix, and creating a new header file.
Comment 4 Brent Fulgham 2013-05-30 22:39:40 PDT
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"
Comment 5 peavo 2013-05-31 04:44:38 PDT
Created attachment 203435 [details]
Patch
Comment 6 peavo 2013-05-31 05:02:12 PDT
(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.
Comment 7 Brent Fulgham 2013-05-31 22:19:57 PDT
Comment on attachment 203435 [details]
Patch

Looks great!  Thanks for making the changes.
Comment 8 WebKit Commit Bot 2013-05-31 22:41:33 PDT
Comment on attachment 203435 [details]
Patch

Clearing flags on attachment: 203435

Committed r151067: <http://trac.webkit.org/changeset/151067>
Comment 9 WebKit Commit Bot 2013-05-31 22:41:34 PDT
All reviewed patches have been landed.  Closing bug.