Move website settings fetching from WebsitePreferences to new WebsitePermissionFetcher.

Move website settings fetching from WebsitePreferences to new WebsitePermissionFetcher.

Use the fetcher to populate the single website preferences screen if it is given only a url string.

This also enables showing the Notifications settings by default.

Based on Sasha's old downstream patch.

BUG=430345
Committed: https://crrev.com/ac1a3823f82e90136e7800d6f77a08534f9afbd1
Cr-Commit-Position: refs/heads/master@{#317125}

[Michael van Ouwerkerk, 2015-02-17 11:59:19]

mvanouwerkerk@chromium.org changed reviewers:
+ bauerb@chromium.org, sashab@chromium.org

[Michael van Ouwerkerk, 2015-02-17 11:59:49]

Sasha, Bernhard, could you take a look please?

[Bernhard Bauer, 2015-02-17 12:10:50]

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

https://codereview.chromium.org/927763003/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePermissionsFetcher.java:76:
private class TaskQueue extends LinkedList<Task> {// WUT? Maybe composition?
Wut? :)

https://codereview.chromium.org/927763003/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePermissionsFetcher.java:240:
// callback for an in-flight fetch? Would that be good or bad?
Do something about this TODO?

(For example, assert that |mCallback| is null at this point. Or actually, do we
even need to have the callback as a member? Couldn't we just pass it to the
PermissionsAvailableCallbackRunner as a construction argument?)

[Michael van Ouwerkerk, 2015-02-17 14:27:18]

All done. Please take another look.

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

https://codereview.chromium.org/927763003/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePermissionsFetcher.java:76:
private class TaskQueue extends LinkedList<Task> {// WUT? Maybe composition?
On 2015/02/17 12:10:50, Bernhard Bauer wrote:
> Wut? :)

Done.

https://codereview.chromium.org/927763003/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePermissionsFetcher.java:240:
// callback for an in-flight fetch? Would that be good or bad?
On 2015/02/17 12:10:50, Bernhard Bauer wrote:
> Do something about this TODO?
> 
> (For example, assert that |mCallback| is null at this point. Or actually, do
we
> even need to have the callback as a member? Couldn't we just pass it to the
> PermissionsAvailableCallbackRunner as a construction argument?)

I moved the callback to the WebsitePermissionFetcher constructor.

[Bernhard Bauer, 2015-02-17 14:46:43]

lgtm

[Peter Beverloo, 2015-02-17 15:06:46]

peter@chromium.org changed reviewers:
+ peter@chromium.org

[Peter Beverloo, 2015-02-17 15:06:47]

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

https://codereview.chromium.org/927763003/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:118:
fetcher.fetchAllPreferences();
Hm, so we end up fetching *all* data for *all* websites. Could you associate a
bug with the TODO on WebsitePermissionsFetcher.java:47? That's not great,
especially not for power users.

https://codereview.chromium.org/927763003/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:129:
* same origin as |address| (as determined by content settings). If
nit: Odd line breaks.

https://codereview.chromium.org/927763003/diff/120001/chrome/browser/android/...
File chrome/browser/android/preferences/website_address.cc (right):

https://codereview.chromium.org/927763003/diff/120001/chrome/browser/android/...
chrome/browser/android/preferences/website_address.cc:21: return
ContentSettingsPattern::FromURL(gurl1).Matches(gurl2);
We should verify that this in fact works for most of the content settings. The
issue right now is that we're storing them differently, which means that
"https://google.com", "https://google.com:443" and some other combinations are
not always considered the same. This may also impact updating existing content
settings.

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

https://codereview.chromium.org/927763003/diff/120001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:87: }
I think we really need Sasha's review on this bit :-(.

Perhaps it's better to split this up - loading and populating the view separated
from unifying the origins of different content settings.

[sashab, 2015-02-18 05:28:32]

Great work Michael. Good to see this patch starting to happen. :)

I'd also be happy for this to be the first of a few patches, which slowly move
towards native functions like "get all settings for this origin" and "get me a
list of origins".

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

https://codereview.chromium.org/927763003/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:118:
fetcher.fetchAllPreferences();
On 2015/02/17 15:06:46, Peter Beverloo wrote:
> Hm, so we end up fetching *all* data for *all* websites. Could you associate a
> bug with the TODO on WebsitePermissionsFetcher.java:47? That's not great,
> especially not for power users.

+1 to the TODO. Would be great if we could (natively) only fetch the permissions
for a single origin at a time.

https://codereview.chromium.org/927763003/diff/120001/chrome/browser/android/...
File chrome/browser/android/preferences/website_address.cc (right):

https://codereview.chromium.org/927763003/diff/120001/chrome/browser/android/...
chrome/browser/android/preferences/website_address.cc:21: return
ContentSettingsPattern::FromURL(gurl1).Matches(gurl2);
On 2015/02/17 15:06:46, Peter Beverloo wrote:
> We should verify that this in fact works for most of the content settings. The
> issue right now is that we're storing them differently, which means that
> "https://google.com", "https://google.com:443" and some other combinations are
> not always considered the same. This may also impact updating existing content
> settings.

Agreed. Maybe the best thing to do for now is just have the logic in here that
we have in the other part of the code: if its HTTPS and ends in 443, or HTTP and
ends in 80, remove the port.

Alternatively, if you look at ContentSettingsPattern, you could create a matcher
for URL1 that matches on scheme/host/port (guessing the port where it is not
specified) and try to match that to URL2. If it matches then its definitely from
the same origin.

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

https://codereview.chromium.org/927763003/diff/120001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:87: }
On 2015/02/17 15:06:46, Peter Beverloo wrote:
> I think we really need Sasha's review on this bit :-(.
> 
> Perhaps it's better to split this up - loading and populating the view
separated
> from unifying the origins of different content settings.

Look... To be honest, I think this is the best we can do for now. We should also
ask bauerb@ for some feedback on this origin pattern stuff.

The problem is that the All Sites list needs a list of patterns, not a list of
'sites'. But different permissions save with different patterns, and patterns
for the same site (ie https...:443 and http...:80) really should be grouped.

raymes@ is also working on fixing this, basically by ensuring that all
permissions are saved using consistent patterns. So ideally, in the future, this
fix would no longer be needed since all permissions will share the same pattern.

Long story short: go for it. This is much better than having two entries
(http...(no port) and http...:80) in the list for every site with a permission.

[Michael van Ouwerkerk, 2015-02-18 19:45:26]

New patchsets have been uploaded after l-g-t-m from bauerb@chromium.org

[Michael van Ouwerkerk, 2015-02-18 19:57:40]

Thanks for the comments.

I found that without any form of merging, the single site screen was nearly
useless, and very confusing. It would display only one permission at a time,
regardless of how many were set and fetched for the site. Which one was
displayed was _random_ thanks to HashSet.

So, this patch merges preferences for fetched sites matching the given origin
into a single site, and all of the information is displayed.

We're getting really close to branch point, so your review is welcome. I'm happy
to take approvals-with-nits :-)

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

https://codereview.chromium.org/927763003/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:118:
fetcher.fetchAllPreferences();
On 2015/02/17 15:06:46, Peter Beverloo wrote:
> Hm, so we end up fetching *all* data for *all* websites. Could you associate a
> bug with the TODO on WebsitePermissionsFetcher.java:47? That's not great,
> especially not for power users.

Done.

https://codereview.chromium.org/927763003/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:118:
fetcher.fetchAllPreferences();
On 2015/02/18 05:28:32, sasha_b wrote:
> On 2015/02/17 15:06:46, Peter Beverloo wrote:
> > Hm, so we end up fetching *all* data for *all* websites. Could you associate
a
> > bug with the TODO on WebsitePermissionsFetcher.java:47? That's not great,
> > especially not for power users.
> 
> +1 to the TODO. Would be great if we could (natively) only fetch the
permissions
> for a single origin at a time.

Done.

https://codereview.chromium.org/927763003/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:129:
* same origin as |address| (as determined by content settings). If
On 2015/02/17 15:06:46, Peter Beverloo wrote:
> nit: Odd line breaks.

Done.

https://codereview.chromium.org/927763003/diff/120001/chrome/browser/android/...
File chrome/browser/android/preferences/website_address.cc (right):

https://codereview.chromium.org/927763003/diff/120001/chrome/browser/android/...
chrome/browser/android/preferences/website_address.cc:21: return
ContentSettingsPattern::FromURL(gurl1).Matches(gurl2);
On 2015/02/17 15:06:46, Peter Beverloo wrote:
> We should verify that this in fact works for most of the content settings. The
> issue right now is that we're storing them differently, which means that
> "https://google.com", "https://google.com:443" and some other combinations are
> not always considered the same. This may also impact updating existing content
> settings.

Verified that this works for the single site screen's purposes, using this page
as a test:
http://dontcallmedom.github.io/web-permissions-req/tests/all.html

https://codereview.chromium.org/927763003/diff/120001/chrome/browser/android/...
chrome/browser/android/preferences/website_address.cc:21: return
ContentSettingsPattern::FromURL(gurl1).Matches(gurl2);
On 2015/02/18 05:28:32, sasha_b wrote:
> On 2015/02/17 15:06:46, Peter Beverloo wrote:
> > We should verify that this in fact works for most of the content settings.
The
> > issue right now is that we're storing them differently, which means that
> > "https://google.com", "https://google.com:443" and some other combinations
are
> > not always considered the same. This may also impact updating existing
content
> > settings.
> 
> Agreed. Maybe the best thing to do for now is just have the logic in here that
> we have in the other part of the code: if its HTTPS and ends in 443, or HTTP
and
> ends in 80, remove the port.
> 
> Alternatively, if you look at ContentSettingsPattern, you could create a
matcher
> for URL1 that matches on scheme/host/port (guessing the port where it is not
> specified) and try to match that to URL2. If it matches then its definitely
from
> the same origin.

Acknowledged.

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

https://codereview.chromium.org/927763003/diff/120001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:87: }
On 2015/02/18 05:28:32, sasha_b wrote:
> On 2015/02/17 15:06:46, Peter Beverloo wrote:
> > I think we really need Sasha's review on this bit :-(.
> > 
> > Perhaps it's better to split this up - loading and populating the view
> separated
> > from unifying the origins of different content settings.
> 
> Look... To be honest, I think this is the best we can do for now. We should
also
> ask bauerb@ for some feedback on this origin pattern stuff.
> 
> The problem is that the All Sites list needs a list of patterns, not a list of
> 'sites'. But different permissions save with different patterns, and patterns
> for the same site (ie https...:443 and http...:80) really should be grouped.
> 
> raymes@ is also working on fixing this, basically by ensuring that all
> permissions are saved using consistent patterns. So ideally, in the future,
this
> fix would no longer be needed since all permissions will share the same
pattern.
> 
> Long story short: go for it. This is much better than having two entries
> (http...(no port) and http...:80) in the list for every site with a
permission.

Acknowledged.

https://codereview.chromium.org/927763003/diff/120001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:87: }
On 2015/02/17 15:06:46, Peter Beverloo wrote:
> I think we really need Sasha's review on this bit :-(.
> 
> Perhaps it's better to split this up - loading and populating the view
separated
> from unifying the origins of different content settings.

See Sasha's comments. Bernhard also already reviewed this and gave it the ok.

[Miguel Garcia, 2015-02-18 21:40:45]

miguelg@chromium.org changed reviewers:
+ miguelg@chromium.org

[Miguel Garcia, 2015-02-18 21:40:46]

Thanks a lot for getting this in shape. I will LGTM first thing in the morning
once we discuss some of the proposed changes.

After branch point we probably want to look at this again and try to refactor it
a bit.

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

https://codereview.chromium.org/927763003/diff/160001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:74:
private class SingleWebsitePermissionsPopulator
Let's at least add a TODO for some heavy refactoring here.

https://codereview.chromium.org/927763003/diff/160001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:130:
Uri uri = Uri.parse(url);
Would getOriginForDisplay in UrlUtilities.java do the job?

https://codereview.chromium.org/927763003/diff/160001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:147:
* Given an address and a list of sets of websites, returns a new site with the
same origin
quadratic complexity with JNI in each call!.

Complexity wise this is O(number of permissions ^2) right? so it's not so
terrible because the number of possible permissions is not very big, perhaps you
can mention it in a comment?
I commented on the JNI piece on C++

https://codereview.chromium.org/927763003/diff/160001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java
(right):

https://codereview.chromium.org/927763003/diff/160001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java:233:
* Copies preferences info of |other| into |this| if |this| does not have a value
for that info
can you explain why is it ok to discard merges where say mPushNotificationInfo
is not null (but perhaps different?) in this and other?

https://codereview.chromium.org/927763003/diff/160001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java:261:
mStorageInfo.addAll(other.mStorageInfo);
so we are ok with duplicates on mStorageInfo?

https://codereview.chromium.org/927763003/diff/160001/chrome/browser/android/...
File chrome/browser/android/preferences/website_address.cc (right):

https://codereview.chromium.org/927763003/diff/160001/chrome/browser/android/...
chrome/browser/android/preferences/website_address.cc:12: jboolean
HasSameContentSettingsOrigin(JNIEnv* env,
Do we really need to go to C++ to do this? can't we just do it in java using the
URI class? By the time you get here all the complexity of the url class is
reduced to checking a (well defined) origin. and we can compare origins in
java...

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

https://codereview.chromium.org/927763003/diff/160001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:64: // The
string |jorigin| is used to group permissions together in the Site
Do you really need to do this here? Can't you pass the bare origin and then
normalize it in java using UrlUtilities#getOriginForDisplay ?

[Miguel Garcia, 2015-02-18 21:50:48]

https://codereview.chromium.org/927763003/diff/160001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromiumApplication.java
(right):

https://codereview.chromium.org/927763003/diff/160001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromiumApplication.java:69:
protected void showSingleOriginSettings(String url) {}
What exactly calls this method? I don't see it being called anywhere in either
of the patches. Is that coming in another patch?

I assumed that whatever intent receiver we currently have for the cog would now
call this instead of calling the "all web sites" version but I  don't see that
anywhere.

[Miguel Garcia, 2015-02-18 21:52:51]

Oh and can you add in the description that you are also enabling notification
settings by default in this patch?

[Miguel Garcia, 2015-02-18 22:01:00]

On 2015/02/18 21:52:51, Miguel Garcia wrote:
> Oh and can you add in the description that you are also enabling notification
> settings by default in this patch?

And yeah I realized I have L-g-t-m-e-d already but in spirit I'd still prefer if
we talk about those comments before submitting.

[Michael van Ouwerkerk, 2015-02-19 17:10:22]

New patchsets have been uploaded after l-g-t-m from miguelg@chromium.org

[Michael van Ouwerkerk, 2015-02-19 17:19:35]

Thanks Miguel.

https://codereview.chromium.org/927763003/diff/160001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromiumApplication.java
(right):

https://codereview.chromium.org/927763003/diff/160001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromiumApplication.java:69:
protected void showSingleOriginSettings(String url) {}
On 2015/02/18 21:50:48, Miguel Garcia wrote:
> What exactly calls this method? I don't see it being called anywhere in either
> of the patches. Is that coming in another patch?
> 
> I assumed that whatever intent receiver we currently have for the cog would
now
> call this instead of calling the "all web sites" version but I  don't see that
> anywhere.

Calling this will be in the next CL.

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

https://codereview.chromium.org/927763003/diff/160001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:74:
private class SingleWebsitePermissionsPopulator
On 2015/02/18 21:40:46, Miguel Garcia wrote:
> Let's at least add a TODO for some heavy refactoring here.

Done.

https://codereview.chromium.org/927763003/diff/160001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:130:
Uri uri = Uri.parse(url);
On 2015/02/18 21:40:46, Miguel Garcia wrote:
> Would getOriginForDisplay in UrlUtilities.java do the job?

Done.

https://codereview.chromium.org/927763003/diff/160001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java:147:
* Given an address and a list of sets of websites, returns a new site with the
same origin
On 2015/02/18 21:40:46, Miguel Garcia wrote:
> quadratic complexity with JNI in each call!.
> 
> Complexity wise this is O(number of permissions ^2) right? so it's not so
> terrible because the number of possible permissions is not very big, perhaps
you
> can mention it in a comment?
> I commented on the JNI piece on C++

Done. I've removed the JNI call from it, and added a note about the cost.

https://codereview.chromium.org/927763003/diff/160001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java
(right):

https://codereview.chromium.org/927763003/diff/160001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java:233:
* Copies preferences info of |other| into |this| if |this| does not have a value
for that info
On 2015/02/18 21:40:46, Miguel Garcia wrote:
> can you explain why is it ok to discard merges where say mPushNotificationInfo
> is not null (but perhaps different?) in this and other?

I've moved the relevant merging functionality into SingleWebsitePreferences
because this behavior is only valid there.

https://codereview.chromium.org/927763003/diff/160001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java:261:
mStorageInfo.addAll(other.mStorageInfo);
On 2015/02/18 21:40:46, Miguel Garcia wrote:
> so we are ok with duplicates on mStorageInfo?

It seems to go unused so far, I couldn't get this info to show on the settings
screen. I added a TODO for handling this.

https://codereview.chromium.org/927763003/diff/160001/chrome/browser/android/...
File chrome/browser/android/preferences/website_address.cc (right):

https://codereview.chromium.org/927763003/diff/160001/chrome/browser/android/...
chrome/browser/android/preferences/website_address.cc:12: jboolean
HasSameContentSettingsOrigin(JNIEnv* env,
On 2015/02/18 21:40:46, Miguel Garcia wrote:
> Do we really need to go to C++ to do this? can't we just do it in java using
the
> URI class? By the time you get here all the complexity of the url class is
> reduced to checking a (well defined) origin. and we can compare origins in
> java...

Done.

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

https://codereview.chromium.org/927763003/diff/160001/chrome/browser/android/...
chrome/browser/android/preferences/website_preference_bridge.cc:64: // The
string |jorigin| is used to group permissions together in the Site
On 2015/02/18 21:40:46, Miguel Garcia wrote:
> Do you really need to do this here? Can't you pass the bare origin and then
> normalize it in java using UrlUtilities#getOriginForDisplay ?

I think this can stay for the moment, but I'm happy to try and refactor the
whole thing later to see if this work can be avoided here. There already is a
TODO for doing this, but I'll put it on my name.

[Miguel Garcia, 2015-02-19 19:15:34]

lgtm

this time for real :)

https://codereview.chromium.org/927763003/diff/160001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromiumApplication.java
(right):

https://codereview.chromium.org/927763003/diff/160001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromiumApplication.java:69:
protected void showSingleOriginSettings(String url) {}
On 2015/02/19 17:19:34, Michael van Ouwerkerk wrote:
> On 2015/02/18 21:50:48, Miguel Garcia wrote:
> > What exactly calls this method? I don't see it being called anywhere in
either
> > of the patches. Is that coming in another patch?
> > 
> > I assumed that whatever intent receiver we currently have for the cog would
> now
> > call this instead of calling the "all web sites" version but I  don't see
that
> > anywhere.
> 
> Calling this will be in the next CL.

Nice, with this change it will work in chrome shell

[Michael van Ouwerkerk, 2015-02-19 19:18:00]

Thanks for the reviews! I know this patch has an unusual number of TODOs, and
I'm happy to take suggestions for followups to improve the implementation. I'm
going to land this now to get the basic functionality in before branch cut.

[Michael van Ouwerkerk, 2015-02-19 19:18:05]

The CQ bit was checked by mvanouwerkerk@chromium.org

[commit-bot: I haz the power, 2015-02-19 19:20:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/927763003/180001

[commit-bot: I haz the power, 2015-02-19 19:24:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-19 19:24:11]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Michael van Ouwerkerk, 2015-02-19 19:34:31]

New patchsets have been uploaded after l-g-t-m from miguelg@chromium.org

[Michael van Ouwerkerk, 2015-02-19 19:35:47]

The CQ bit was checked by mvanouwerkerk@chromium.org

[commit-bot: I haz the power, 2015-02-19 19:36:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/927763003/200001

[commit-bot: I haz the power, 2015-02-19 19:41:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-19 19:41:09]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Michael van Ouwerkerk, 2015-02-19 19:59:56]

The CQ bit was checked by mvanouwerkerk@chromium.org

[commit-bot: I haz the power, 2015-02-19 20:00:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/927763003/220001

[commit-bot: I haz the power, 2015-02-19 20:25:39]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-02-19 20:26:29]

Patchset 12 (id:??) landed as
https://crrev.com/ac1a3823f82e90136e7800d6f77a08534f9afbd1
Cr-Commit-Position: refs/heads/master@{#317125}

[Finnur, 2015-03-17 09:33:22]

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

[Finnur, 2015-03-17 09:33:23]

https://codereview.chromium.org/927763003/diff/220001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromiumApplication.java
(right):

https://codereview.chromium.org/927763003/diff/220001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromiumApplication.java:78:
protected void showSingleOriginSettings(String url) {
Belated drive-by: There were no callers of this function added in this CL. Is it
redundant?

[Michael van Ouwerkerk, 2015-04-20 15:48:24]

https://codereview.chromium.org/927763003/diff/220001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromiumApplication.java
(right):

https://codereview.chromium.org/927763003/diff/220001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromiumApplication.java:78:
protected void showSingleOriginSettings(String url) {
On 2015/03/17 09:33:23, Finnur wrote:
> Belated drive-by: There were no callers of this function added in this CL. Is
it
> redundant?

Wow, memory fails me, I'm sorry. I'm sure I had a reason, like it had to be
called or overridden from a downstream subclass (that's why it is protected).
But I can't find a single caller upstream or downstream now. You're probably
right.