Bug 89545

Summary: [Chromium] Missing setCookieEnabled accessor in WebView
Product: WebKit Reporter: Garret Kelly <gdk>
Component: WebKit APIAssignee: Garret Kelly <gdk>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, eric.carlson, feature-media-reviews, fishd, jamesr, jochen, mihaip, tkent+wkapi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Garret Kelly 2012-06-19 20:07:01 PDT
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.
Comment 1 Garret Kelly 2012-06-19 20:18:12 PDT
Created attachment 148492 [details]
Patch
Comment 2 WebKit Review Bot 2012-06-19 20:21:08 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 WebKit Review Bot 2012-06-19 20:21:31 PDT
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.
Comment 4 Adam Barth 2012-06-19 20:35:50 PDT
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)?
Comment 5 jochen 2012-06-20 01:01:06 PDT
What is this supposed to be used for? Maybe it should be an addition to the WebPermissionClient?
Comment 6 Garret Kelly 2012-06-20 08:20:44 PDT
(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.
Comment 7 Adam Barth 2012-06-20 10:20:26 PDT
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...
Comment 8 Garret Kelly 2012-06-20 10:22:18 PDT
Created attachment 148593 [details]
Patch
Comment 9 Mihai Parparita 2012-06-20 10:24:00 PDT
(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.
Comment 10 Adam Barth 2012-06-20 10:45:54 PDT
> 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.
Comment 11 Adam Barth 2012-06-20 14:20:34 PDT
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.
Comment 12 jochen 2012-06-20 15:18:25 PDT
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)
Comment 13 WebKit Review Bot 2012-06-20 16:50:09 PDT
Comment on attachment 148593 [details]
Patch

Clearing flags on attachment: 148593

Committed r120885: <http://trac.webkit.org/changeset/120885>
Comment 14 WebKit Review Bot 2012-06-20 16:50:15 PDT
All reviewed patches have been landed.  Closing bug.