Add SPI for sending WebsiteSettings to WebProcess during navigation

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Add SPI for sending WebsiteSettings to WebProcess during navigation

[Alex Christensen]

Created attachment 296383 [details]
Patch

[Alex Christensen]

Created attachment 296385 [details]
Patch

[Alex Christensen]

Created attachment 296396 [details]
Patch

[Anders Carlsson]

Comment on attachment 296396 [details]
Patch

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

> Source/WebCore/loader/DocumentLoader.h:235
> +    std::optional<bool> userContentExtensionsDisabled() const { return m_userContentExtensionsDisabled; }
> +    void setUserContentExtensionsDisabled(std::optional<bool> disabled) { m_userContentExtensionsDisabled = disabled; }

Using std::optional<bool> is pretty confusing, it seems like you should be able to just have it be a boolean (and using enabled by default seems easier to understand too).

> Source/WebKit2/UIProcess/API/Cocoa/_WKWebsitePolicies.h:33
> +@property (nonatomic) BOOL contentBlockersDisabled;

Again, I think this should be contentBlockersEnabled.

[Alex Christensen]

Created attachment 296449 [details]
Patch

[Alex Christensen]

Created attachment 296470 [details]
Patch

[Anders Carlsson]

Comment on attachment 296470 [details]
Patch

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

> Source/WebKit2/UIProcess/API/APIWebsitePolicies.h:37
> +    explicit WebsitePolicies() = default;
> +    virtual ~WebsitePolicies() = default;

I'd put these out of line so we won't end up emitting a vtable wherever we use RefPtr<WebsitePolicies>.

> Source/WebKit2/UIProcess/API/APIWebsitePolicies.h:45
> +    

Extra newline here.

> Source/WebKit2/UIProcess/API/Cocoa/WKNavigationDelegatePrivate.h:68
> +- (void)_webView:(WKWebView *)webView decidePolicyForNavigationAction:(WKNavigationAction *)navigationAction decisionHandler:(void (^)(WKNavigationActionPolicy, _WKWebsitePolicies *))decisionHandler WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));

It's a little ugly that we now have an extra parameter here that only makes sense for calling it with WKNavigationActionPolicyAllow.

I think if we want to make this API, we should use an object instead, similar to WebPolicyListener (or whatever it's called)..

> Source/WebKit2/UIProcess/API/Cocoa/_WKWebsitePoliciesInternal.h:44
> +    @package

@package shouldn't be indented.

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:819
> +    // Only setUserContentExtensionsEnabled if it hasn't already been disabled by reloading without content blockers.
> +    if (documentLoader->userContentExtensionsEnabled())
> +        documentLoader->setUserContentExtensionsEnabled(websitePolicies.contentBlockersEnabled);

Does that mean that it's impossible to implement "reload _with_ content blockers"?

[Alex Christensen]

Comment on attachment 296470 [details]
Patch

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

>> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:819
>> +        documentLoader->setUserContentExtensionsEnabled(websitePolicies.contentBlockersEnabled);
> 
> Does that mean that it's impossible to implement "reload _with_ content blockers"?

yes.  we would definitely need optional bools for that.

[Alex Christensen]

Comment on attachment 296470 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/Cocoa/WKNavigationDelegatePrivate.h:68
>> +- (void)_webView:(WKWebView *)webView decidePolicyForNavigationAction:(WKNavigationAction *)navigationAction decisionHandler:(void (^)(WKNavigationActionPolicy, _WKWebsitePolicies *))decisionHandler WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> It's a little ugly that we now have an extra parameter here that only makes sense for calling it with WKNavigationActionPolicyAllow.
> 
> I think if we want to make this API, we should use an object instead, similar to WebPolicyListener (or whatever it's called)..

Ok, I'll land this as-is.  If we make it API, we would need to rename it anyways.  The _ is the only difference between this and an existing selector's name.

[Alex Christensen]

http://trac.webkit.org/changeset/209558