Bug 165517 - Add SPI for sending WebsiteSettings to WebProcess during navigation
Summary: Add SPI for sending WebsiteSettings to WebProcess during navigation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-12-06 23:01 PST by Alex Christensen
Modified: 2016-12-16 23:47 PST (History)
3 users (show)

See Also:


Attachments
Patch (103.67 KB, patch)
2016-12-07 00:43 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (104.49 KB, patch)
2016-12-07 01:11 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (103.67 KB, patch)
2016-12-07 10:19 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (86.38 KB, patch)
2016-12-07 17:24 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (86.37 KB, patch)
2016-12-07 20:14 PST, Alex Christensen
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2016-12-06 23:01:27 PST
Add SPI for sending WebsiteSettings to WebProcess during navigation
Comment 1 Alex Christensen 2016-12-07 00:43:18 PST
Created attachment 296383 [details]
Patch
Comment 2 Alex Christensen 2016-12-07 01:11:45 PST
Created attachment 296385 [details]
Patch
Comment 3 Alex Christensen 2016-12-07 10:19:51 PST
Created attachment 296396 [details]
Patch
Comment 4 Anders Carlsson 2016-12-07 14:02:58 PST
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.
Comment 5 Alex Christensen 2016-12-07 17:24:55 PST
Created attachment 296449 [details]
Patch
Comment 6 Alex Christensen 2016-12-07 20:14:42 PST
Created attachment 296470 [details]
Patch
Comment 7 Anders Carlsson 2016-12-08 12:28:30 PST
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"?
Comment 8 Alex Christensen 2016-12-08 12:42:15 PST
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.
Comment 9 Alex Christensen 2016-12-08 12:45:34 PST
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.
Comment 10 Alex Christensen 2016-12-08 12:53:16 PST
http://trac.webkit.org/changeset/209558