Summary: | [Curl] Unify logic of ResourceHandleCurlDelegate into ResourceHandle | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Basuke Suzuki <Basuke.Suzuki> | ||||||||||||||||||
Component: | WebCore Misc. | Assignee: | Basuke Suzuki <Basuke.Suzuki> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | achristensen, annulen, Basuke.Suzuki, 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
Basuke Suzuki
2018-02-07 13:35:05 PST
Created attachment 333397 [details]
patch
Created attachment 333400 [details]
Another try
Created attachment 333410 [details]
Fix break of other port
Created attachment 333537 [details]
WIP redesign classes
With Alex's advice, I restructured classes and made no need to implement interface directy on ResourceHandle.
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.
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. Created attachment 333601 [details]
Patch
Created attachment 333709 [details]
PATCH
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 (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. Created attachment 333926 [details]
Fix reviewed points
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.
Created attachment 333927 [details]
Fix style
Comment on attachment 333927 [details] Fix style Clearing flags on attachment: 333927 Committed r228577: <https://trac.webkit.org/changeset/228577> All reviewed patches have been landed. Closing bug. |