Bug 224119 – Add WKURLSchemeTask redirect API

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

[Brady Eidson]

Add WKURLSchemeTask redirect API

Like http would need.

[Brady Eidson]

Created attachment 425033 [details]
Patch

[Alex Christensen]

Comment on attachment 425033 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425033&action=review

> Source/WebKit/UIProcess/API/Cocoa/WKURLSchemeTask.h:46
> + WebKit might decide that changes need to be make to the proposed request.

or that the redirect should not be followed such as if a WKContentRuleList decides to block it.
That would also make a great test case showing why we need _Nullable in the completion handler.

> Source/WebKit/UIProcess/API/Cocoa/WKURLSchemeTask.mm:101
> +    auto handler = adoptNS([completionHandler copy]);

makeBlockPtr is the cool new way to do this.  Then you don't need rawHandler.

> Source/WebKit/UIProcess/WebPageProxy.cpp:9513
> +    bool schemeIsInHTTPFamily = equalIgnoringASCIICase(*canonicalizedScheme, "http") || equalIgnoringASCIICase(*canonicalizedScheme, "https");

Case insensitive compare is unnecessary here because it has already been canonicalized to lower case.

> Source/WebKit/UIProcess/WebURLSchemeTask.cpp:96
> +    Function<void(ResourceRequest&&)> innerCompletionHandler = [protectedThis = makeRef(*this), this, completionHandler = WTFMove(completionHandler)] (ResourceRequest&& request) mutable {

CompletionHandler

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKURLSchemeHandler-1.mm:1446
> +@interface HTTPRedirectTest : NSObject <WKURLSchemeHandler> {

TestURLSchemeHandler might make this nicer.

[Brady Eidson]

(In reply to Alex Christensen from comment #2)
> Comment on attachment 425033 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425033&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKURLSchemeTask.h:46
> > + WebKit might decide that changes need to be make to the proposed request.
> 
> or that the redirect should not be followed such as if a WKContentRuleList
> decides to block it.
> That would also make a great test case showing why we need _Nullable in the
> completion handler.
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKURLSchemeTask.mm:101
> > +    auto handler = adoptNS([completionHandler copy]);
> 
> makeBlockPtr is the cool new way to do this.  Then you don't need rawHandler.
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:9513
> > +    bool schemeIsInHTTPFamily = equalIgnoringASCIICase(*canonicalizedScheme, "http") || equalIgnoringASCIICase(*canonicalizedScheme, "https");
> 
> Case insensitive compare is unnecessary here because it has already been
> canonicalized to lower case.
> 
> > Source/WebKit/UIProcess/WebURLSchemeTask.cpp:96
> > +    Function<void(ResourceRequest&&)> innerCompletionHandler = [protectedThis = makeRef(*this), this, completionHandler = WTFMove(completionHandler)] (ResourceRequest&& request) mutable {
> 
> CompletionHandler

Cannot, because it's sometimes not called (in the exception case)

> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/WKURLSchemeHandler-1.mm:1446
> > +@interface HTTPRedirectTest : NSObject <WKURLSchemeHandler> {
> 
> TestURLSchemeHandler might make this nicer.

All others, cool.

[Brady Eidson]

Created attachment 425045 [details]
Patch

[Alex Christensen]

Comment on attachment 425045 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425045&action=review

> Source/WebKit/UIProcess/API/Cocoa/WKURLSchemeTask.h:39
> +/*! @abstract Mark the task as redirected.

Indicate that the task was redirected.
If it were just marking, it wouldn't need a completion handler.

> Source/WebKit/UIProcess/API/Cocoa/WKURLSchemeTask.mm:105
> +    auto function = [protectedSelf = retainPtr(self), self, protectedResponse = retainPtr(response), response, protectedRequest = retainPtr(request), request, innerCompletionHandler = WTFMove(innerCompletionHandler)] () mutable {

It seems unnecessary to doubly wrap this block.

[Brady Eidson]

(In reply to Alex Christensen from comment #5)
> Comment on attachment 425045 [details]
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKURLSchemeTask.mm:105
> > +    auto function = [protectedSelf = retainPtr(self), self, protectedResponse = retainPtr(response), response, protectedRequest = retainPtr(request), request, innerCompletionHandler = WTFMove(innerCompletionHandler)] () mutable {
> 
> It seems unnecessary to doubly wrap this block.

It does have to be wrapped to translate from ResourceRequest to NSURLRequest, but I moved that inside the outer wrapper instead of as a separate step up front.

[Chris Dumez]

Comment on attachment 425045 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425045&action=review

>> Source/WebKit/UIProcess/API/Cocoa/WKURLSchemeTask.mm:105
>> +    auto function = [protectedSelf = retainPtr(self), self, protectedResponse = retainPtr(response), response, protectedRequest = retainPtr(request), request, innerCompletionHandler = WTFMove(innerCompletionHandler)] () mutable {
> 
> It seems unnecessary to doubly wrap this block.

Also capturing both the protected and unprotected version of the request & response seems overkill.

[Brady Eidson]

Created attachment 425054 [details]
Patch

[EWS]

Committed r275447: <https://commits.webkit.org/r275447>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425054 [details].

[Radar WebKit Bug Importer]

<rdar://problem/76168418>