Bug 182578 – [Curl] Unify logic of ResourceHandleCurlDelegate into ResourceHandle

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

[Basuke Suzuki]

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.

[Basuke Suzuki]

Created attachment 333397 [details]
patch

[Basuke Suzuki]

Created attachment 333400 [details]
Another try

[Basuke Suzuki]

Created attachment 333410 [details]
Fix break of other port

[Basuke Suzuki]

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]

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]

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]

Created attachment 333601 [details]
Patch

[Basuke Suzuki]

Created attachment 333709 [details]
PATCH

[Konstantin Tokarev]

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]

(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]

Created attachment 333926 [details]
Fix reviewed points

[EWS Watchlist]

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]

Created attachment 333927 [details]
Fix style

[WebKit Commit Bot]

Comment on attachment 333927 [details]
Fix style

Clearing flags on attachment: 333927

Committed r228577: <https://trac.webkit.org/changeset/228577>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/37616884>