Bug 174799 – Add SPI WKNavigationDelegate willSendRequest

• Component: New Bugs
• Status: NEW

[Alex Christensen]

Add SPI WKNavigationDelegate willSendRequest

[Alex Christensen]

Created attachment 316319 [details]
Patch

[Alex Christensen]

Created attachment 316323 [details]
Patch

[Sam Weinig]

I am vehemently against this idea. It was a design decision to omit this.

[Brady Eidson]

(In reply to Sam Weinig from comment #3)
> I am vehemently against this idea. It was a design decision to omit this.

Could you share why the decision was so important?

Maybe we can reflect in 2017 WebKit if reasoning behind this 2009/2010 design decision still holds?

Comment on attachment 316323 [details]
Patch

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

> Source/WebCore/contentextensions/ContentExtensionParser.cpp:265
> +    if (actionType == "will-send-request") {

Seems strange to add a private action type named just like the public ones.

> Source/WebKit/UIProcess/API/Cocoa/WKNavigationDelegatePrivate.h:71
> +- (void)_webView:(WKWebView *)webView willSendRequest:(NSURLRequest *)request redirectResponse:(NSURLResponse *)redirectResponse completionHandler:(void (^)(NSURLRequest *))completionHandler;

This is missing an availability annotation.

> Source/WebKit/UIProcess/Cocoa/NavigationState.mm:145
> +    m_navigationDelegateMethods.webViewWillSendRequest = [delegate respondsToSelector:@selector(_webView: willSendRequest: redirectResponse: completionHandler:)];

Extra spaces after the colons.

> Source/WebKit/UIProcess/Cocoa/NavigationState.mm:303
> +void NavigationState::NavigationClient::willSendRequest(WebCore::ResourceRequest&& request, WebCore::ResourceResponse&& redirectResponse, Function<void(WebCore::ResourceRequest&&)> completionHandler)
> +{
> +    if (!m_navigationState.m_navigationDelegateMethods.webViewWillSendRequest) {
> +        completionHandler(WTFMove(request));
> +        return;
> +    }
> +    [(id <WKNavigationDelegatePrivate>)m_navigationState.navigationDelegate() _webView:m_navigationState.m_webView willSendRequest:request.nsURLRequest(DoNotUpdateHTTPBody) redirectResponse:redirectResponse.nsURLResponse() completionHandler:BlockPtr<void(NSURLRequest *)>::fromCallable([completionHandler = WTFMove(completionHandler)](NSURLRequest *newRequest) {
> +        if (newRequest)
> +            completionHandler(newRequest);
> +        else {
> +            // FIXME: !m_platformRequestUpdated is asserting when encoding a ResourceRequest made from a nil NSURLRequest
> +            // but not from an empty ResourceRequest. That should probably not be the case.
> +            completionHandler({ });
> +        }
> +    }).get()];
> +}

This is missing a CompletionHandlerCallChecker.

[Alex Christensen]

Comment on attachment 316323 [details]
Patch

I'm investigating other ways of solving the same problem.

[Sam Weinig]

(In reply to Brady Eidson from comment #4)
> (In reply to Sam Weinig from comment #3)
> > I am vehemently against this idea. It was a design decision to omit this.
> 
> Could you share why the decision was so important?
> 
> Maybe we can reflect in 2017 WebKit if reasoning behind this 2009/2010
> design decision still holds?

The main reason is willSendRequest provides an easy opportunity for clients to introduce sever performance penalties to their code, if what they do in willSendRequest is expensive. Even if we make willSendRequest async from WebCore perspective, this can still introduce unexpected and hard to diagnose loading performance issues. 

Rather than go this route, I would recommend continuing to extend content extensions for non-Safari use cases, by allowing re-writing rules. That would have the added benefit of also supporting sub-resources.

If we must, for some reason, have a UI side delegate that transforms navigation requests, I believe there should be a discussion of if / how these could be integrated into the policy delegation notions. While I still consider it to an easy way to unintentionally hurt performance, I believe it would be slightly cleaner≥