Bug 224119 - Add WKURLSchemeTask redirect API
Summary: Add WKURLSchemeTask redirect API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-04-02 10:57 PDT by Brady Eidson
Modified: 2021-04-02 18:06 PDT (History)
3 users (show)

See Also:


Attachments
Patch (35.47 KB, patch)
2021-04-02 11:03 PDT, Brady Eidson
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (36.39 KB, patch)
2021-04-02 13:15 PDT, Brady Eidson
achristensen: review+
Details | Formatted Diff | Diff
Patch (36.45 KB, patch)
2021-04-02 14:18 PDT, Brady Eidson
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2021-04-02 10:57:18 PDT
Add WKURLSchemeTask redirect API

Like http would need.
Comment 1 Brady Eidson 2021-04-02 11:03:16 PDT
Created attachment 425033 [details]
Patch
Comment 2 Alex Christensen 2021-04-02 11:35:59 PDT
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.
Comment 3 Brady Eidson 2021-04-02 12:10:52 PDT
(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.
Comment 4 Brady Eidson 2021-04-02 13:15:47 PDT
Created attachment 425045 [details]
Patch
Comment 5 Alex Christensen 2021-04-02 14:04:46 PDT
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.
Comment 6 Brady Eidson 2021-04-02 14:13:42 PDT
(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.
Comment 7 Chris Dumez 2021-04-02 14:15:27 PDT
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.
Comment 8 Brady Eidson 2021-04-02 14:18:13 PDT
Created attachment 425054 [details]
Patch
Comment 9 EWS 2021-04-02 15:39:26 PDT
Committed r275447: <https://commits.webkit.org/r275447>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425054 [details].
Comment 10 Radar WebKit Bug Importer 2021-04-02 15:40:16 PDT
<rdar://problem/76168418>