Bug 89545 – [Chromium] Missing setCookieEnabled accessor in WebView

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Garret Kelly]

There's currently no way of modifying the cookieEnabled state of a Page, which is leading to a lot of DOM Exception 18s for renderers that haven't disabled cookies when they should have.

[Garret Kelly]

Created attachment 148492 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Attachment 148492 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1
Source/WebKit/chromium/public/WebView.h:490:  The parameter name "enabled" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/WebViewImpl.h:573:  The parameter name "enabled" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 148492 [details]
Patch

This patch is OK, but a better patch would be to move the cookieEnabled setting to the Settings object.  I did a quick look and this isn't used in many places, so it should be easy to move.  Would you be willing to move it so we can put the API in the right place (i.e., WebSettings)?

What is this supposed to be used for? Maybe it should be an addition to the WebPermissionClient?

[Garret Kelly]

(In reply to comment #4)
> (From update of attachment 148492 [details])
> This patch is OK, but a better patch would be to move the cookieEnabled setting to the Settings object.  I did a quick look and this isn't used in many places, so it should be easy to move.  Would you be willing to move it so we can put the API in the right place (i.e., WebSettings)?

Absolutely! I'll update with a revised patch.

[Adam Barth]

For context about jochen's question, we already have a mechanism for disabling cookies via the WebPermissionClient.  I wonder whether that addresses your use case...

[Garret Kelly]

Created attachment 148593 [details]
Patch

[Mihai Parparita]

(In reply to comment #7)
> For context about jochen's question, we already have a mechanism for disabling cookies via the WebPermissionClient.  I wonder whether that addresses your use case...

We do? I'm not seeing any cookie-related methods on it.

In general, is there a guidelines for what should go on WebSettings vs. WebPermissionClient? I see that JavaScript can be disabled by both, globally by WebSettings and in a more fine-grained level by WebPermissionClient. In this case, we'd like all cookies to be disabled for Chrome apps, so WebSettings seems like a better fit.

[Adam Barth]

> We do? I'm not seeing any cookie-related methods on it.

Oh, you're right.  Sorry, the function is on http://trac.webkit.org/browser/trunk/Source/Platform/chromium/public/WebCookieJar.h

> In general, is there a guidelines for what should go on WebSettings vs. WebPermissionClient? I see that JavaScript can be disabled by both, globally by WebSettings and in a more fine-grained level by WebPermissionClient. In this case, we'd like all cookies to be disabled for Chrome apps, so WebSettings seems like a better fit.

We use WebPermissionClient when we need something fine-grained (e.g., depending on which Frame rather than which Page).

It sounds like you want to disable things on a per-page basis, in which case WebSettings does sound appropriate (especially after we move this bool from Page to Settings).

jochen might have further thoughts given that he's worked a bunch on cookie blocking.

[Adam Barth]

Comment on attachment 148593 [details]
Patch

This patch looks good from a technical point of view.  Let's give jochen a chance to comment before landing it.

ok, thanks for the clarification.

the patch looks good.

For the record, the difference of disabling javascript via settings vs. via webpermissionclient is that the former disables all javascript, whereas the latter only disables execution of javascript from the web page (but still allows content scripts injected by an extension etc)

[WebKit Review Bot]

Comment on attachment 148593 [details]
Patch

Clearing flags on attachment: 148593

Committed r120885: <http://trac.webkit.org/changeset/120885>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.