Bug 134393

Attachment 233978 [details] did not pass style-queue: ERROR: Source/WebCore/platform/network/curl/CurlDownload.cpp:239: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #0) > Created an attachment (id=233978) [details] > Patch This patch doesn't have a changelog.

Created attachment 315944 [details] fixed With refactoring of CurlDownload with CurlJobManager, the process is much cleaner. Call of success() or cleanup() is now exclusively mandatory by callback of start(), no need to call them from destructor.

Created attachment 315945 [details] add changelog

This patch contains fix for other patch. Should I separate these? https://bugs.webkit.org/show_bug.cgi?id=134392

Comment on attachment 315945 [details] add changelog View in context: https://bugs.webkit.org/attachment.cgi?id=315945&action=review This is probably an improvement, but it's unclear what is going on here. It's certainly a lot more than preventing closing and downloading a file twice. > Source/WebCore/platform/network/curl/CurlDownload.cpp:-60 > - LockHolder locker(m_mutex); There is a lot of mutex removal. There is no explanation why. That and maybe other things should be separated into a different patch. > Source/WebCore/platform/network/curl/CurlDownload.cpp:170 > + // @FIXME error handling may be required @ is unlike the rest of our code base.

Created attachment 315970 [details] Remove unrelated patch After refactoring the architecture of job manager, tasks are more clear when it would be called. Also with use of lambda when starting the download, the lifecycle of the download object is more clear and confident. The downloaded file will be move to the destination on success() or deleted on failure or cancelation. Also deleted unused private static methods.

Attachment 315970 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:6: Line contains tab character. [whitespace/tab] [5] ERROR: Source/WebCore/ChangeLog:7: Line contains tab character. [whitespace/tab] [5] ERROR: Source/WebCore/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] ERROR: Source/WebCore/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] ERROR: Source/WebCore/ChangeLog:10: Line contains tab character. [whitespace/tab] [5] ERROR: Source/WebCore/ChangeLog:11: Line contains tab character. [whitespace/tab] [5] ERROR: Source/WebCore/ChangeLog:13: Line contains tab character. [whitespace/tab] [5] Total errors found: 7 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 315972 [details] style

Sorry, my vi at home is not setup well.

Comment on attachment 315972 [details] style View in context: https://bugs.webkit.org/attachment.cgi?id=315972&action=review > Source/WebCore/platform/network/curl/CurlDownload.cpp:184 > + m_response.setURL(url); If I remember correctly, there used to be a problem calling ResourceResponse methods from a non-main thread. Is this not the case anymore? > Source/WebCore/platform/network/curl/CurlDownload.cpp:-195 > - LockHolder locker(m_mutex); Is it safe to remove this lock? > Source/WebCore/platform/network/curl/CurlDownload.h:95 > + bool m_cancelled; Should we initialize this member here? Are there any regressions when running (http) layout tests with this patch?

Comment on attachment 315972 [details] style (In reply to Per Arne Vollan from comment #11) > Comment on attachment 315972 [details] > style Per Arne, This patch was invalidated by other bug. https://bugs.webkit.org/show_bug.cgi?id=175253

I'll send next patch after the bug lands. Could you take a look at this one first? https://bugs.webkit.org/show_bug.cgi?id=175253

Rename it's title from "curl: Don't try to close & move the downloaded file twice" to "[Curl] Delete temporally downloaded file when it cancelled" because some parts are already fixed while refactoring the structure by other bugs.

Created attachment 324654 [details] patch

Comment on attachment 324654 [details] patch Clearing flags on attachment: 324654 Committed r223995: <https://trac.webkit.org/changeset/223995>

All reviewed patches have been landed. Closing bug.

<rdar://problem/35568999>