Summary: | [Chromium] Missing setCookieEnabled accessor in WebView | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Garret Kelly <gdk> | ||||||
Component: | WebKit API | Assignee: | 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
Garret Kelly
2012-06-19 20:07:01 PDT
Created attachment 148492 [details]
Patch
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. 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 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? (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. 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... Created attachment 148593 [details]
Patch
(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. > 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 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) Comment on attachment 148593 [details] Patch Clearing flags on attachment: 148593 Committed r120885: <http://trac.webkit.org/changeset/120885> All reviewed patches have been landed. Closing bug. |