Switch Cookie to use ContentSettingException instead of CookieInfo on Android

Switch Cookie to use ContentSettingException instead of CookieInfo on Android

We are about to start syncing cookie exceptions to Android. After that allowing
users to add exceptions or toggle existing exceptions will cause crashes. It is
because for now CookieInfo only support set settings for urls.

In this CL, CookieInfo is replaced with ContentSettingException, similar to
JavaScript, to support patterns. This will make sure that users can manually
add an exception and toggle it on Android.

BUG=598909
Committed: https://crrev.com/9c6944d530898d73c02325c0a5c03a257dd7f208
Cr-Commit-Position: refs/heads/master@{#389979}

[lshang, 2016-04-07 06:09:58]

Description was changed from

==========
Switch Cookie to use ContentSettingException instead of CookieInfo on Android


change cookies to use contentSettingExceptions

BUG=
==========

to

==========
Switch Cookie to use ContentSettingException instead of CookieInfo on Android

We are about to start syncing cookie exceptions to Android. After that allowing 
users to add exceptions or toggle existing exceptions will cause crashes. It is 
because for now CookieInfo only support set settings for urls.

In this CL, CookieInfo is replaced with ContentSettingException, similar to 
JavaScript, to support patterns. This will make sure that users can manually 
add an exception and toggle it on Android. 

BUG=598909
==========

[lshang, 2016-04-07 06:16:02]

lshang@chromium.org changed reviewers:
+ raymes@chromium.org

[lshang, 2016-04-07 06:16:03]

PTAL thanks!

[raymes, 2016-04-11 07:43:53]

Looks good I don't know this code so well :)

https://codereview.chromium.org/1864163005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
(left):

https://codereview.chromium.org/1864163005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:210:
}
How come we remove this?

[lshang, 2016-04-11 08:17:07]

lshang@chromium.org changed reviewers:
+ finnur@chromium.org, tsergeant@chromium.org

[lshang, 2016-04-11 08:17:08]

+finnur +tsergeant for code review

The only issue of changing Cookies to use ContentSettingException is that cookie
settings will not show up in the single website setting list. Javascript has the
same problem. They will show in all sites settings and single category settings.

[Finnur, 2016-04-11 09:48:49]

https://codereview.chromium.org/1864163005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
(left):

https://codereview.chromium.org/1864163005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:210:
}
Yeah, this doesn't look right.

https://codereview.chromium.org/1864163005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
(right):

https://codereview.chromium.org/1864163005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:74:
public static final String PREF_COOKIES_PERMISSION = "cookies_permission_list";
> The only issue of changing Cookies to use ContentSettingException is that
cookie
> settings will not show up in the single website setting list. Javascript has
the
> same problem.

It seems clear that Cookies and JavaScript are expected to show, so I think it
would be best if you debug this to find out why it is no longer appearing.

https://codereview.chromium.org/1864163005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:265:
// TODO(lshang): Merge in CookieException? It becomes to use patterns.
nit: 'It becomes to use' is not grammatically correct.

https://codereview.chromium.org/1864163005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePermissionsFetcher.java
(right):

https://codereview.chromium.org/1864163005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePermissionsFetcher.java:87:
queue.add(new PermissionsAvailableCallbackRunner());
nit: Can you add a new-line before this, to separate?

[lshang, 2016-04-19 05:20:50]

https://codereview.chromium.org/1864163005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
(left):

https://codereview.chromium.org/1864163005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:210:
}
On 2016/04/11 09:48:49, Finnur wrote:
> Yeah, this doesn't look right.

Yeah, this is why javascript and cookies settings don't show up in the single
website preference list. Please see details below:-)

https://codereview.chromium.org/1864163005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
(right):

https://codereview.chromium.org/1864163005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:74:
public static final String PREF_COOKIES_PERMISSION = "cookies_permission_list";
On 2016/04/11 09:48:49, Finnur wrote:
> > The only issue of changing Cookies to use ContentSettingException is that
> cookie
> > settings will not show up in the single website setting list. Javascript has
> the
> > same problem.
> 
> It seems clear that Cookies and JavaScript are expected to show, so I think it
> would be best if you debug this to find out why it is no longer appearing.

Cookies, Popups and JavaScript don't show in the single website preferences
because they are not merged into the website's ContentSettingExceptions. As in
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav....
Only PermissionInfo with the same origin get merged. For
ContentSettingException, it has a pattern stored as a string, but it's hard to
match a website's host or origin. (I did an experiment to merge in JavaScript
exceptions and tried to match exception's pattern with the website's origin or
host, and found that they didn't always work. e.g. blocking JavaScript for
[*.]facebook.com doesn't apply to https://m.facebook.com because their host is
different). 

I also found that, when we click on the green lock of the website's url bar,
there will be a WebsiteSettingsPopup showing some of the website's site
settings. JavaScript and Cookies settings will show here because all of the
settings are fetched from
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...,
which calls HostContentSettingsMap::GetWebsiteSetting() of this url and shows
site settings different from default ones. I think SingleWebsitePreferences and
WebsiteSettingsPopup are doing the same thing but they do it via two different
code path.

I prefer to first fix this cookie crash now and it might OK to not showing
popup, javascript and cookie exceptions on the single website preference (they
are displayed in all sites settings). But I'd like to hear you thought Finnur:-)

https://codereview.chromium.org/1864163005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:265:
// TODO(lshang): Merge in CookieException? It becomes to use patterns.
On 2016/04/11 09:48:49, Finnur wrote:
> nit: 'It becomes to use' is not grammatically correct.

Done.

https://codereview.chromium.org/1864163005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePermissionsFetcher.java
(right):

https://codereview.chromium.org/1864163005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePermissionsFetcher.java:87:
queue.add(new PermissionsAvailableCallbackRunner());
On 2016/04/11 09:48:49, Finnur wrote:
> nit: Can you add a new-line before this, to separate?

Done.

[Finnur, 2016-04-19 10:11:04]

finnur@chromium.org changed reviewers:
+ mvanouwerkerk@chromium.org

[Finnur, 2016-04-19 10:12:22]

finnur@chromium.org changed reviewers:
+ emilyschechter@google.com, rolfe@chromium.org

[Finnur, 2016-04-19 10:24:32]

https://codereview.chromium.org/1864163005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
(right):

https://codereview.chromium.org/1864163005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:74:
public static final String PREF_COOKIES_PERMISSION = "cookies_permission_list";
Thanks for looking into this.

Hmm... This is tricky. Probably best to involve a PM (Emily) and a UX designer
(rolfe@) in this discussion. And probably mvanouwerkerk@ as well, the Master
Origin Merger. :)

As a user I'd expect to be able to click on an origin to view its detail, such
as https://m.facebook.com (note the 'm') and see that JavaScript is blocked due
to a [*.]facebook.com pattern rule. Problem is, what does allowing JavaScript
then mean? Does it mean JavaScript is now allowed on http[s]://www.facebook.com
as well (note: 'www')?

How does it work in the WebsiteSettingsPopup?

Unfortunately, I have more UX questions than answers at this point:
Should we warn that allowing JavaScript will mean allowing it for all of
Facebook?
Should we simply show that JavaScript is disallowed, but not let the user change
it (except through the JavaScript pattern)? Then we'd need to show both the
domain and the pattern under All Sites.
 
Also, somewhat related: What if this origin falls under a more complex pattern
than simply [*.]facebook.com? Is that going to make things messy,
implementation-wise?

[raymes, 2016-04-20 01:31:18]

Thanks Finnur! I think Liu is just trying to make cookies work without crashing
in this CL (and making it work in the same way as JS seems reasonable). This is
important because we're going to start syncing cookie settings to mobile soon
and we don't want the browser to crash. Do you think we could try to land a
patch to fix that issue specifically (maybe there is an alternative fix if this
one doesn't seem appropriate)?

I also definitely agree that there are some larger concerns about how patterns
should work in the UX (among other issues). We're doing some work in this area
and it would be great to meet up and discuss more about how things should work.
I can set up a meeting if you like? Thanks!

[Finnur, 2016-04-20 11:31:42]

Happy to meet and I have no problem with not blocking this CL on this issue.

But, there's an outstanding question whether the removal of the few lines of
Cookies code in the mergePermissionInfoForTopLevelOrigin function is the right
course of action and I don't trust myself to judge that fully. I think
mvanouwerkerk@ should bless that part specifically, since he wrote that part (I
believe). If he gives the green light, then that's enough for me.

[Finnur, 2016-04-20 11:31:53]

One nit.

https://codereview.chromium.org/1864163005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
(right):

https://codereview.chromium.org/1864163005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:265:
// TODO(lshang): Merge in CookieException? It becomes to use patterns.
s/starts to/will/

[raymes, 2016-04-21 04:25:12]

Thanks Finnur, I'll set up a meeting soon :)

On Wed, 20 Apr 2016 at 21:31 <finnur@chromium.org> wrote:

> One nit.
>
>
>
>
>
https://codereview.chromium.org/1864163005/diff/40001/chrome/android/java/src...
> File
>
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
> (right):
>
>
>
https://codereview.chromium.org/1864163005/diff/40001/chrome/android/java/src...
>
>
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:265:
> // TODO(lshang): Merge in CookieException? It becomes to use patterns.
> s/starts to/will/
>
> https://codereview.chromium.org/1864163005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Michael van Ouwerkerk, 2016-04-21 09:07:54]

lgtm for fixing a potential crash

I do think we'll need to follow up and rationalize the single origin prefs
screen. As for UX, patterns are difficult to edit on mobile, and I suspect that
editing would not be used frequently, like, maybe less 0.1% of users. Do we have
data on this? But I can see how displaying and clearing patterns is quite
important.

https://codereview.chromium.org/1864163005/diff/80001/chrome/browser/android/...
File chrome/browser/android/preferences/website_preference_bridge.cc (right):

https://codereview.chromium.org/1864163005/diff/80001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:18: #include
"chrome/browser/content_settings/cookie_settings_factory.h"
nit: I don't think you need this include anymore.

https://codereview.chromium.org/1864163005/diff/80001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:27: #include
"components/content_settings/core/browser/cookie_settings.h"
nit: I don't think you need this include anymore.

[lshang, 2016-04-26 07:09:15]

lshang@chromium.org changed reviewers:
+ bauerb@chromium.org

[lshang, 2016-04-26 07:09:16]

Thanks Michael!

+bauerb for OWNER's approval.

https://codereview.chromium.org/1864163005/diff/80001/chrome/browser/android/...
File chrome/browser/android/preferences/website_preference_bridge.cc (right):

https://codereview.chromium.org/1864163005/diff/80001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:18: #include
"chrome/browser/content_settings/cookie_settings_factory.h"
On 2016/04/21 09:07:54, Michael van Ouwerkerk wrote:
> nit: I don't think you need this include anymore.

Done.

https://codereview.chromium.org/1864163005/diff/80001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:27: #include
"components/content_settings/core/browser/cookie_settings.h"
On 2016/04/21 09:07:54, Michael van Ouwerkerk wrote:
> nit: I don't think you need this include anymore.

Done.

[Bernhard Bauer, 2016-04-26 09:34:09]

LGTM with a suggestion for a followup cleanup:

https://codereview.chromium.org/1864163005/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePermissionsFetcher.java
(right):

https://codereview.chromium.org/1864163005/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePermissionsFetcher.java:262:
public void run() {
The only difference to e.g. JavaScriptExceptionInfoFetcher is the method that is
called to set the exception, but everything else (including the type of the
exception) is the same, at least for a handful of content types. If we added a
setException(ContentSettingsType, ContentSettingException) method, we might be
able to get rid of a lot of duplicated code here.

[lshang, 2016-04-27 03:17:33]

Thanks all!

https://codereview.chromium.org/1864163005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
(right):

https://codereview.chromium.org/1864163005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:265:
// TODO(lshang): Merge in CookieException? It becomes to use patterns.
On 2016/04/20 11:31:53, Finnur wrote:
> s/starts to/will/

Done.

[lshang, 2016-04-27 03:17:44]

The CQ bit was checked by lshang@chromium.org

[lshang, 2016-04-27 03:17:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
mvanouwerkerk@chromium.org, bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1864163005/#ps140001
(title: "one missing comments to be addressed")

[commit-bot: I haz the power, 2016-04-27 03:17:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1864163005/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1864163005/140001

[commit-bot: I haz the power, 2016-04-27 03:22:48]

Description was changed from

==========
Switch Cookie to use ContentSettingException instead of CookieInfo on Android

We are about to start syncing cookie exceptions to Android. After that allowing 
users to add exceptions or toggle existing exceptions will cause crashes. It is 
because for now CookieInfo only support set settings for urls.

In this CL, CookieInfo is replaced with ContentSettingException, similar to 
JavaScript, to support patterns. This will make sure that users can manually 
add an exception and toggle it on Android. 

BUG=598909
==========

to

==========
Switch Cookie to use ContentSettingException instead of CookieInfo on Android

We are about to start syncing cookie exceptions to Android. After that allowing 
users to add exceptions or toggle existing exceptions will cause crashes. It is 
because for now CookieInfo only support set settings for urls.

In this CL, CookieInfo is replaced with ContentSettingException, similar to 
JavaScript, to support patterns. This will make sure that users can manually 
add an exception and toggle it on Android. 

BUG=598909
==========

[commit-bot: I haz the power, 2016-04-27 03:22:49]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-04-27 03:24:26]

Description was changed from

==========
Switch Cookie to use ContentSettingException instead of CookieInfo on Android

We are about to start syncing cookie exceptions to Android. After that allowing 
users to add exceptions or toggle existing exceptions will cause crashes. It is 
because for now CookieInfo only support set settings for urls.

In this CL, CookieInfo is replaced with ContentSettingException, similar to 
JavaScript, to support patterns. This will make sure that users can manually 
add an exception and toggle it on Android. 

BUG=598909
==========

to

==========
Switch Cookie to use ContentSettingException instead of CookieInfo on Android

We are about to start syncing cookie exceptions to Android. After that allowing
users to add exceptions or toggle existing exceptions will cause crashes. It is
because for now CookieInfo only support set settings for urls.

In this CL, CookieInfo is replaced with ContentSettingException, similar to
JavaScript, to support patterns. This will make sure that users can manually
add an exception and toggle it on Android.

BUG=598909

Committed: https://crrev.com/9c6944d530898d73c02325c0a5c03a257dd7f208
Cr-Commit-Position: refs/heads/master@{#389979}
==========

[commit-bot: I haz the power, 2016-04-27 03:24:27]

Patchset 8 (id:??) landed as
https://crrev.com/9c6944d530898d73c02325c0a5c03a257dd7f208
Cr-Commit-Position: refs/heads/master@{#389979}