Bug 182578

Summary: [Curl] Unify logic of ResourceHandleCurlDelegate into ResourceHandle
Product: WebKit Reporter: Basuke Suzuki <basuke>
Component: WebCore Misc.Assignee: Basuke Suzuki <basuke>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, annulen, basuke, bfulgham, commit-queue, don.olmstead, ews-watchlist, pvollan, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 117300    
Attachments:
Description Flags
patch
none
Another try
none
Fix break of other port
none
WIP redesign classes
none
Patch
none
PATCH
none
Fix reviewed points
none
Fix style none

Basuke Suzuki
Reported 2018-02-07 13:35:05 PST
ResourceHandleCurlDelegate was created for migration to the CurlRequest classes. Every features required by NetworkLoadTask are moved to CurlRequest class and remainings are ResourceHandle related logic. It's time to merge back to ResourceHandle.
Attachments
patch (46.78 KB, patch)
2018-02-08 11:44 PST, Basuke Suzuki 		no flags
DetailsFormatted DiffDiff
Another try (46.78 KB, patch)
2018-02-08 11:53 PST, Basuke Suzuki 		no flags
DetailsFormatted DiffDiff
Fix break of other port (46.78 KB, patch)
2018-02-08 12:38 PST, Basuke Suzuki 		no flags
DetailsFormatted DiffDiff
WIP redesign classes (61.42 KB, patch)
2018-02-09 17:06 PST, Basuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (65.84 KB, patch)
2018-02-12 09:57 PST, Basuke Suzuki 		no flags
DetailsFormatted DiffDiff
PATCH (66.85 KB, patch)
2018-02-13 11:21 PST, Basuke Suzuki 		no flags
DetailsFormatted DiffDiff
Fix reviewed points (67.24 KB, patch)
2018-02-15 11:43 PST, Basuke Suzuki 		no flags
DetailsFormatted DiffDiff
Fix style (66.89 KB, patch)
2018-02-15 11:56 PST, Basuke Suzuki 		no flags
DetailsFormatted DiffDiff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.
Basuke Suzuki
Comment 1 2018-02-08 11:44:02 PST
Created attachment 333397 [details] patch
Basuke Suzuki
Comment 2 2018-02-08 11:53:13 PST
Created attachment 333400 [details] Another try
Basuke Suzuki
Comment 3 2018-02-08 12:38:26 PST
Created attachment 333410 [details] Fix break of other port
Basuke Suzuki
Comment 4 2018-02-09 17:06:00 PST
Created attachment 333537 [details] WIP redesign classes With Alex's advice, I restructured classes and made no need to implement interface directy on ResourceHandle.
EWS Watchlist
Comment 5 2018-02-09 17:07:03 PST
Attachment 333537 [details] did not pass style-queue: ERROR: Source/WebCore/platform/network/curl/CurlResourceHandleDelegate.cpp:47: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] ERROR: Source/WebCore/platform/network/curl/CurlResourceHandleDelegate.cpp:47: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/network/curl/CurlResourceHandleDelegate.h:1: One or more unexpected \r (^M) found; better to use only a \n [whitespace/carriage_return] [1] ERROR: Suppressing further [whitespace/carriage_return] reports for this file. ERROR: Source/WebCore/platform/network/curl/CurlResourceHandleDelegate.h:45: The parameter name "handle" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/network/curl/CurlResourceHandleDelegate.h:51: The parameter name "response" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 80 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Basuke Suzuki
Comment 6 2018-02-09 17:10:11 PST
One of the goal with this patch is to make WinCairo port looks same with other ports. We use Mac port as a reference for task separation. WebCoreResourceHandleAsOperationQueueDelegate => CurlResourceHandleDelegate We have our dedicated scheduler which handle thread handling, so CurlResourceHandleDelegate is much simpler than Mac's one.
Basuke Suzuki
Comment 7 2018-02-12 09:57:15 PST
Created attachment 333601 [details] Patch
Basuke Suzuki
Comment 8 2018-02-13 11:21:45 PST
Created attachment 333709 [details] PATCH
Konstantin Tokarev
Comment 9 2018-02-15 10:46:24 PST
Comment on attachment 333709 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=333709&action=review > Source/WebCore/platform/network/curl/CurlResourceHandleDelegate.cpp:93 > + for (auto header : response.headers) { const auto& header > Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:342 > + return std::pair<String, String>("", ""); If you really need empty String which is non-null internally, use WTF::emptyString() > Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:519 > + mediaType = "text/plain"; It should be ASCIILiteral("text/plain") > Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:525 > + charset = "US-ASCII"; Ditto
Basuke Suzuki
Comment 10 2018-02-15 11:36:52 PST
(In reply to Konstantin Tokarev from comment #9) > Comment on attachment 333709 [details] > PATCH > > View in context: > https://bugs.webkit.org/attachment.cgi?id=333709&action=review > > > Source/WebCore/platform/network/curl/CurlResourceHandleDelegate.cpp:93 > > + for (auto header : response.headers) { > > const auto& header right. > > Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:342 > > + return std::pair<String, String>("", ""); > > If you really need empty String which is non-null internally, use > WTF::emptyString() Good point. These empty strings are not required to be set, so I will make them std::optional. > > Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:519 > > + mediaType = "text/plain"; > > It should be ASCIILiteral("text/plain") > > > Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:525 > > + charset = "US-ASCII"; > > Ditto Got them.
Basuke Suzuki
Comment 11 2018-02-15 11:43:31 PST
Created attachment 333926 [details] Fix reviewed points
EWS Watchlist
Comment 12 2018-02-15 11:45:58 PST
Attachment 333926 [details] did not pass style-queue: ERROR: Source/WebCore/platform/network/curl/CurlRequest.h:97: The parameter name "client" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Basuke Suzuki
Comment 13 2018-02-15 11:56:48 PST
Created attachment 333927 [details] Fix style
WebKit Commit Bot
Comment 14 2018-02-16 13:39:44 PST
Comment on attachment 333927 [details] Fix style Clearing flags on attachment: 333927 Committed r228577: <https://trac.webkit.org/changeset/228577>
WebKit Commit Bot
Comment 15 2018-02-16 13:39:45 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 16 2018-02-16 13:40:31 PST
<rdar://problem/37616884>
Note You need to log in before you can comment on or make changes to this bug.