Add Permissions to the PageInfo dialog on Android

Add Permissions to the PageInfo dialog on Android

Add permissions dropdowns to the PageInfo dialog on Android,
which alter the content setting for the website currently
being visited. Also add a 'Copy URL' button and a 'Done'
button.

BUG=365528
Committed: https://crrev.com/97894ce12fc78d6f59e63314aefead3857816a28
Cr-Commit-Position: refs/heads/master@{#300658}

[sashab, 2014-10-07 06:22:05]

sashab@chromium.org changed reviewers:
+ tedchoc@chromium.org

[sashab, 2014-10-07 06:25:51]

Hi Ted,

This is still a WIP, but I've got the permissions dropdowns to appear and change
the content settings for the site when selected. For some reason the Spinners
are appearing a bit strange (not displaying an arrow/bottom border) but I'm sure
that's a minor problem I can look at later. Also the buttons currently do
nothing.

If you could just take a general look over the patch and let me know if I'm
heading in the right direction that would be great :)

Thanks,

Sasha

[Finnur, 2014-10-08 09:22:55]

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

[Finnur, 2014-10-08 09:22:56]

https://codereview.chromium.org/620983002/diff/60001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/620983002/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:139:
private int getImageResourceForPermission(int permission) {
This feels like it should be public in a more accessible location, because I'm
going to need exactly this kind of function in my classes and rolling my own
risks going out of sync.

How about Website.java, for example?

https://codereview.chromium.org/620983002/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:164:
return R.drawable.page_info_popups;
I'm wondering if we can avoid renaming these images?

For one, page_info_ (as a prefix) is misleading, because we're both planning to
use this and I'm not using it in an association with page_info. 

Second, renaming makes it hard to see if an image from the master image list has
already been added.

I have already compiled a list of all the icons I was planning to use (with
original names -- I can provide you with it, if you want?). I think it makes
sense to keep them in the public repository (that you're working in), though,
because I think I can access the public repository from the non-public one but
you can't do the reverse.

[sashab, 2014-10-08 22:55:03]

Ted: happy for you to properly review this now. It looks like a large patch
because of all the icons but it's really not that big :D If you want I can move
the icons to another patch, not sure what the policy is on binary files in CLs.

https://codereview.chromium.org/620983002/diff/60001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/620983002/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:139:
private int getImageResourceForPermission(int permission) {
On 2014/10/08 09:22:55, Finnur wrote:
> This feels like it should be public in a more accessible location, because I'm
> going to need exactly this kind of function in my classes and rolling my own
> risks going out of sync.
> 
> How about Website.java, for example?

So that you don't need my patch to start working on WebsiteSettings, and since
this function is currently only needed inside this file, I'm happy to leave this
here for now and for you to write basically a copy of this in WebsiteSettings
that uses your own images. Whoever lands last can combine these into:

getImageResourceForPermission(permission, screen)

where 'screen' is either WebsiteSettings or PageInfo (is it OK to pass around
Java classes as arguments? If not an enum representing each one could probably
work).

I've added a TODO to the top of this class as well to rename it and all its
resources to PageInfo* and page_info_*. That leaves the WebsiteSettings prefix
to you. So that should hopefully clear up some confusion as well :)

Sasha

https://codereview.chromium.org/620983002/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:164:
return R.drawable.page_info_popups;
On 2014/10/08 09:22:55, Finnur wrote:
> I'm wondering if we can avoid renaming these images?
> 
> For one, page_info_ (as a prefix) is misleading, because we're both planning
to
> use this and I'm not using it in an association with page_info. 
> 
> Second, renaming makes it hard to see if an image from the master image list
has
> already been added.
> 
> I have already compiled a list of all the icons I was planning to use (with
> original names -- I can provide you with it, if you want?). I think it makes
> sense to keep them in the public repository (that you're working in), though,
> because I think I can access the public repository from the non-public one but
> you can't do the reverse.
> 

I don't mind where the icons go, but I think we're going to need separate images
for both page_info_ and website_settings_ anyway since they are slightly
different shades of grey. See what resources UI sends you once WebsiteSettings
is UI approved, but for now I'd use icons with a website_settings_* prefix.

We should definitely combine these functions though, even if we end up having
two. Discussed this in the previous comment.

[Ted C, 2014-10-09 01:25:15]

I know in the SVN world you did need to split out and land resources first, but
no idea if that is still the case.

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
File
chrome/android/java/res/drawable/website_settings_permission_spinner_dropdown_item.xml
(right):

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
chrome/android/java/res/drawable/website_settings_permission_spinner_dropdown_item.xml:8:
<CheckedTextView xmlns:android="http://schemas.android.com/apk/res/android"
CheckedTextView?  The mocks do not have any check boxes.

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
File
chrome/android/java/res/drawable/website_settings_permission_spinner_item.xml
(right):

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
chrome/android/java/res/drawable/website_settings_permission_spinner_item.xml:12:
android:layout_height="@dimen/website_settings_popup_permission_spinner"
in general, we typically don't define dimens (and often colors) if they are only
used in a single place.  This applies to the rest of the CL.  Granted, if you
are sharing top/left/bottom of padding of one view, you would also put right
there if it was only used once just for consistency.  Always defining externally
ends up causing a lot of hopping around when reading/updating.

Granted, if all the dimens you are adding are used all over then disregard this.

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
File
chrome/android/java/res/drawable/website_settings_permission_spinner_item_background.xml
(right):

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
chrome/android/java/res/drawable/website_settings_permission_spinner_item_background.xml:8:
<layer-list xmlns:android="http://schemas.android.com/apk/res/android">
The spinner underline isn't consistent with the new look and feel of the android
spinners.  Why are we being different from the rest of the platform here?

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
chrome/android/java/res/drawable/website_settings_permission_spinner_item_background.xml:13:
<solid android:color="@android:color/transparent" />
why is this necessary?

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
File chrome/android/java/res/layout/website_settings.xml (right):

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
chrome/android/java/res/layout/website_settings.xml:27:
android:text="https://google.com/design/material/guidelines"
remove?

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
chrome/android/java/res/layout/website_settings.xml:37: android:text="Your
connection to this site is private, but an attacker could still compromise the
page."
remove this too?

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
chrome/android/java/res/layout/website_settings.xml:42:
android:layout_width="match_parent"
why do you need this?  If you set layout_gravity="end" on the button does it get
right aligned as you expect?

For these padding values, you should be able to set them as
layout_marginTop/Bottom instead of padding to get the same effect as well.

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
chrome/android/java/res/layout/website_settings.xml:56:
android:height="@dimen/website_settings_popup_button_height"
if you just set layout_height="@dimen/website_settings_popup_button_height"
instead, can you remove this?

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
chrome/android/java/res/layout/website_settings.xml:77:
android:paddingEnd="@dimen/website_settings_popup_margin_sides"
These seem to be used only in padding, so you should either rename the dimen to
be padding sides or convert these to use marginStart, marginEnd

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
chrome/android/java/res/layout/website_settings.xml:78:
android:paddingStart="@dimen/website_settings_popup_margin_sides" >
put start above end

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
chrome/android/java/res/layout/website_settings.xml:93:
android:paddingTop="@dimen/website_settings_popup_margin_above_done_button" >
put top above bottom

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
chrome/android/java/res/layout/website_settings.xml:100:
android:layout_marginEnd="@dimen/website_settings_popup_button_external_padding_sides"
use margin instead of external_padding

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
chrome/android/java/res/layout/website_settings.xml:103:
android:paddingEnd="@dimen/website_settings_popup_button_internal_padding_sides"
no need for padding

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
File chrome/android/java/res/layout/website_settings_permission_row.xml (right):

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
chrome/android/java/res/layout/website_settings_permission_row.xml:3: Copyright
(c) 2013 The Chromium Authors. All rights reserved.
no (c) and 2014

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
chrome/android/java/res/layout/website_settings_permission_row.xml:29:
android:text="Automatic downloads"
remove this

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
chrome/android/java/res/layout/website_settings_permission_row.xml:31:
android:textSize="@dimen/website_settings_popup_permission_name"
for text sizes, the dimens should include text_size

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:46:
* WebsiteSettings* and website_settings_*. Do this on the C++ side as well.
For TODO's, align all subsequent lines with the start of the first line
("Rename" in this case).

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:102:
mSiteSettingsButton.setVisibility(View.GONE);
move this up with the mSiteSettingsButton configuration.  And I would spaces
between the setup of mCopyUrlButton, mSiteSettingsButton, and mDoneButton
blocks.

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:121:
// the data we're
this looks like it fit fine before on 100 char lines, so this is a mistake???

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:145:
// TODO(sashab): Check if this needs to be removed - manually
should this be above the return?

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:147:
// not be available on mobile.
put this on the previous line

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:148:
case ContentSettingsType.CONTENT_SETTINGS_TYPE_FULLSCREEN:
We don't use this on mobile.

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:214:
int domainStartIndex = scheme.length();
this should really be schemeEndIndex now

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:215:
int domainEndIndex = domainStartIndex + domain.length() + 3;
Out of curiosity, what about pages like about:blank?  Will the scheme divider be
: or will it be expanded to ://?  Just wondering if 3 isn't flexible enough.

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:246:
LinearLayout permission_row = (LinearLayout)
LayoutInflater.from(mContext).inflate(
always use camelCased names in java

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:265:
ArrayAdapter<String> adapter = new ArrayAdapter<String>(mContext,
Instead of using tags, I think it would be slightly nicer to have a simple class
that was:

private static final class PagePermissionEntry {
    public final String name;
    public final int value;
    public final int type;

    PagePermission(String name, int value, int type);

    public String toString() {
        return name;
    }
}

Then this would be an adapter of those and onItemSelected would just operate on
a single unit.

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:312:
ClipboardManager clipboardManager = (ClipboardManager)
mContext.getSystemService(
Clipboard.java is slightly safer to compensate for some crashier old builds of
android.

(i.e. new Clipboard(mContext).setText(...))

And in most cases I see we are just passing the value for the label as well, so
we might just do that here to be consistent (or you need to pull "url" out as a
constant).

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:316:
Toast.makeText(mContext, R.string.page_info_copy_success_text,
Toast.LENGTH_SHORT)
This toast isn't consistent with the rest of the copy actions we do in chrome
(nor the aosp part of android).  Wondering what the justification is for doing
it here?

Is it because we are not dismissing the popup if they click copy URL?  Should we
be and not using this toast?

[sashab, 2014-10-14 02:53:19]

Thanks Ted; see parallel UI thread for nit UI discussions. Since this is behind
a flag I'm happy to land this and fix up the UI specifics later (pre-launch);
just need something to go to launch committee with, so we can get this landed
and enabled in M40 :)

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
File
chrome/android/java/res/drawable/website_settings_permission_spinner_dropdown_item.xml
(right):

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
chrome/android/java/res/drawable/website_settings_permission_spinner_dropdown_item.xml:8:
<CheckedTextView xmlns:android="http://schemas.android.com/apk/res/android"
On 2014/10/09 01:25:13, Ted C wrote:
> CheckedTextView?  The mocks do not have any check boxes.

Changed to TextView.

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
File
chrome/android/java/res/drawable/website_settings_permission_spinner_item.xml
(right):

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
chrome/android/java/res/drawable/website_settings_permission_spinner_item.xml:12:
android:layout_height="@dimen/website_settings_popup_permission_spinner"
On 2014/10/09 01:25:13, Ted C wrote:
> in general, we typically don't define dimens (and often colors) if they are
only
> used in a single place.  This applies to the rest of the CL.  Granted, if you
> are sharing top/left/bottom of padding of one view, you would also put right
> there if it was only used once just for consistency.  Always defining
externally
> ends up causing a lot of hopping around when reading/updating.
> 
> Granted, if all the dimens you are adding are used all over then disregard
this.

Fixed this everywhere; there were a lot of places where I had defined a
dimension to only use it in one place. Is definitely a lot easier to read now,
and dimens.xml/colors.xml aren't so cluttered :)

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
File
chrome/android/java/res/drawable/website_settings_permission_spinner_item_background.xml
(right):

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
chrome/android/java/res/drawable/website_settings_permission_spinner_item_background.xml:8:
<layer-list xmlns:android="http://schemas.android.com/apk/res/android">
On 2014/10/09 01:25:14, Ted C wrote:
> The spinner underline isn't consistent with the new look and feel of the
android
> spinners.  Why are we being different from the rest of the platform here?

I'm trying to match the mocks, and I'm developing for Android K, where the
Material Design styles haven't been implemented yet. I'm assuming when we reach
Android L we can substitute these spinners with their Material Design
equivalents?

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
chrome/android/java/res/drawable/website_settings_permission_spinner_item_background.xml:13:
<solid android:color="@android:color/transparent" />
On 2014/10/09 01:25:14, Ted C wrote:
> why is this necessary?

Sorry; didn't realise transparent was the default. Removed.

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
File chrome/android/java/res/layout/website_settings.xml (right):

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
chrome/android/java/res/layout/website_settings.xml:27:
android:text="https://google.com/design/material/guidelines"
On 2014/10/09 01:25:14, Ted C wrote:
> remove?

Yup; sorry about these. Done.

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
chrome/android/java/res/layout/website_settings.xml:37: android:text="Your
connection to this site is private, but an attacker could still compromise the
page."
On 2014/10/09 01:25:14, Ted C wrote:
> remove this too?

Done.

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
chrome/android/java/res/layout/website_settings.xml:42:
android:layout_width="match_parent"
On 2014/10/09 01:25:14, Ted C wrote:
> why do you need this?  If you set layout_gravity="end" on the button does it
get
> right aligned as you expect?
> 
> For these padding values, you should be able to set them as
> layout_marginTop/Bottom instead of padding to get the same effect as well.

Yup, layout_gravity works, didn't know you could do this on a single view :)
Done and moved the paddings to margins.

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
chrome/android/java/res/layout/website_settings.xml:56:
android:height="@dimen/website_settings_popup_button_height"
On 2014/10/09 01:25:14, Ted C wrote:
> if you just set layout_height="@dimen/website_settings_popup_button_height"
> instead, can you remove this?

Yup, works :) Done.

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
chrome/android/java/res/layout/website_settings.xml:77:
android:paddingEnd="@dimen/website_settings_popup_margin_sides"
On 2014/10/09 01:25:14, Ted C wrote:
> These seem to be used only in padding, so you should either rename the dimen
to
> be padding sides or convert these to use marginStart, marginEnd

Renamed them to be padding.

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
chrome/android/java/res/layout/website_settings.xml:78:
android:paddingStart="@dimen/website_settings_popup_margin_sides" >
On 2014/10/09 01:25:14, Ted C wrote:
> put start above end

Done.

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
chrome/android/java/res/layout/website_settings.xml:93:
android:paddingTop="@dimen/website_settings_popup_margin_above_done_button" >
On 2014/10/09 01:25:14, Ted C wrote:
> put top above bottom

Done.

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
chrome/android/java/res/layout/website_settings.xml:100:
android:layout_marginEnd="@dimen/website_settings_popup_button_external_padding_sides"
On 2014/10/09 01:25:14, Ted C wrote:
> use margin instead of external_padding

Done.

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
chrome/android/java/res/layout/website_settings.xml:103:
android:paddingEnd="@dimen/website_settings_popup_button_internal_padding_sides"
On 2014/10/09 01:25:14, Ted C wrote:
> no need for padding

I think you mean 'no need for internal'? :) Done.

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
File chrome/android/java/res/layout/website_settings_permission_row.xml (right):

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
chrome/android/java/res/layout/website_settings_permission_row.xml:3: Copyright
(c) 2013 The Chromium Authors. All rights reserved.
On 2014/10/09 01:25:14, Ted C wrote:
> no (c) and 2014

Done.

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
chrome/android/java/res/layout/website_settings_permission_row.xml:29:
android:text="Automatic downloads"
On 2014/10/09 01:25:14, Ted C wrote:
> remove this

Done.

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/res/...
chrome/android/java/res/layout/website_settings_permission_row.xml:31:
android:textSize="@dimen/website_settings_popup_permission_name"
On 2014/10/09 01:25:14, Ted C wrote:
> for text sizes, the dimens should include text_size

Removed this one anyway. And all the other textSize dimens attributes say _size
at the end.

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:46:
* WebsiteSettings* and website_settings_*. Do this on the C++ side as well.
On 2014/10/09 01:25:14, Ted C wrote:
> For TODO's, align all subsequent lines with the start of the first line
> ("Rename" in this case).

Done.

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:102:
mSiteSettingsButton.setVisibility(View.GONE);
On 2014/10/09 01:25:15, Ted C wrote:
> move this up with the mSiteSettingsButton configuration.  And I would spaces
> between the setup of mCopyUrlButton, mSiteSettingsButton, and mDoneButton
> blocks.

Done.

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:121:
// the data we're
On 2014/10/09 01:25:15, Ted C wrote:
> this looks like it fit fine before on 100 char lines, so this is a mistake???

Weird... Done.

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:145:
// TODO(sashab): Check if this needs to be removed - manually
On 2014/10/09 01:25:14, Ted C wrote:
> should this be above the return?

Yes, but I've now actually confirmed that this doesn't exist on mobile. Replaced
this list with only the 6 that we are using, so no comments required.

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:147:
// not be available on mobile.
On 2014/10/09 01:25:14, Ted C wrote:
> put this on the previous line

Done.

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:148:
case ContentSettingsType.CONTENT_SETTINGS_TYPE_FULLSCREEN:
On 2014/10/09 01:25:15, Ted C wrote:
> We don't use this on mobile.

Done.

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:214:
int domainStartIndex = scheme.length();
On 2014/10/09 01:25:14, Ted C wrote:
> this should really be schemeEndIndex now

Done.

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:215:
int domainEndIndex = domainStartIndex + domain.length() + 3;
On 2014/10/09 01:25:15, Ted C wrote:
> Out of curiosity, what about pages like about:blank?  Will the scheme divider
be
> : or will it be expanded to ://?  Just wondering if 3 isn't flexible enough.

Good point. Also confirmed with UI that we actually should be displaying the
whole URL, not just the 'compact' version like I've done here. Revamped this
function to just take a schemeEndIndex and domainEndIndex and color it
accordingly.

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:241:
* settingsNames/settingsValues.
Went through and changed all these comments to be aligned to 100 chars instead
of 80. Not sure why this happened.

Also added JavaDoc-style comments where appropriate.

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:246:
LinearLayout permission_row = (LinearLayout)
LayoutInflater.from(mContext).inflate(
On 2014/10/09 01:25:14, Ted C wrote:
> always use camelCased names in java

Done.

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:265:
ArrayAdapter<String> adapter = new ArrayAdapter<String>(mContext,
On 2014/10/09 01:25:15, Ted C wrote:
> Instead of using tags, I think it would be slightly nicer to have a simple
class
> that was:
> 
> private static final class PagePermissionEntry {
>     public final String name;
>     public final int value;
>     public final int type;
> 
>     PagePermission(String name, int value, int type);
> 
>     public String toString() {
>         return name;
>     }
> }
> 
> Then this would be an adapter of those and onItemSelected would just operate
on
> a single unit.

Nice. Done.

Also changed the place these are set to be purely in Java - agreed with UI that
these will always be fixed options (only Allow and Block), so no need to pass
these parameters (settingsNames/settingsValues) through anymore.

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:312:
ClipboardManager clipboardManager = (ClipboardManager)
mContext.getSystemService(
On 2014/10/09 01:25:14, Ted C wrote:
> Clipboard.java is slightly safer to compensate for some crashier old builds
of
> android.
> 
> (i.e. new Clipboard(mContext).setText(...))
> 
> And in most cases I see we are just passing the value for the label as well,
so
> we might just do that here to be consistent (or you need to pull "url" out as
a
> constant).

Thanks for this; done, and set the label to the value (I can see it done
everywhere else as well).

https://codereview.chromium.org/620983002/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:316:
Toast.makeText(mContext, R.string.page_info_copy_success_text,
Toast.LENGTH_SHORT)
On 2014/10/09 01:25:15, Ted C wrote:
> This toast isn't consistent with the rest of the copy actions we do in chrome
> (nor the aosp part of android).  Wondering what the justification is for doing
> it here?
> 
> Is it because we are not dismissing the popup if they click copy URL?  Should
we
> be and not using this toast?

Removed the toast; it was my own idea (and you're right - it's inconsistent :-)
). Confirming with UI what kind of feedback we want to give the user when they
click 'Copy URL'.

https://codereview.chromium.org/620983002/diff/100001/chrome/android/java/res...
File chrome/android/java/res/layout/website_settings.xml (right):

https://codereview.chromium.org/620983002/diff/100001/chrome/android/java/res...
chrome/android/java/res/layout/website_settings.xml:86: <Button
This button code is duplicated 3 times. Is there a way I can extract out the
common text size/color/padding etc into a custom tag, perhaps? Is there an
accepted pattern for doing this?

[sashab, 2014-10-15 06:13:57]

sashab@chromium.org changed reviewers:
+ jochen@chromium.org

[sashab, 2014-10-15 06:13:57]

jochen@chromium.org: Please review changes in 
- chrome/chrome.gyp
- chrome/BUILD.gn
- components/content_settings/*.

tedchoc@chromium.org: Please review all other changes :)

[sashab, 2014-10-15 08:23:45]

+some CCs who might want to keep tabs on this CL :)

Once this lands, this is considered feature-complete for the PageInfo launch.
There have been some final bugs identified but agreed to be fixed post-release
(details on the bug).

[jochen (gone - plz use gerrit), 2014-10-15 15:28:46]

components and gyp/gn lgtm

[Ted C, 2014-10-16 00:24:25]

https://codereview.chromium.org/620983002/diff/120001/chrome/android/java/res...
File
chrome/android/java/res/drawable/website_settings_permission_spinner_item_background.xml
(right):

https://codereview.chromium.org/620983002/diff/120001/chrome/android/java/res...
chrome/android/java/res/drawable/website_settings_permission_spinner_item_background.xml:16:
<item><bitmap android:gravity="right"
android:src="@drawable/page_info_arrow_down" /></item>
With this, I think the text can draw over the dropdown arrow.

You might want to apply end padding to the spinner to make sure that isn't
possible.

https://codereview.chromium.org/620983002/diff/120001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/620983002/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:335:

In the transition to PageInfoPermissionEntry, we should be using just that here.

i.e.

PageInfoPermissionEntry entry  = (PageInfoPermissionEntry)
parent.getItemAtPosition(pos);
nativeOnPermissionSettingChanged(mNativeWebsiteSettingsPopup, entry.type,
entry.value);

No need for the tags or anything else here (I think)

[sashab, 2014-10-16 05:52:01]

sashab@chromium.org changed reviewers:
+ markusheintz@google.com

[sashab, 2014-10-16 05:52:02]

markusheintz: please review changes to chrome/browser/ui/website_settings/*
(especially changes in website_settings.cc)

tedchoc: please review changes in chrome/browser/ui/android/ and
chrome/android/java :)

https://codereview.chromium.org/620983002/diff/120001/chrome/android/java/res...
File
chrome/android/java/res/drawable/website_settings_permission_spinner_item_background.xml
(right):

https://codereview.chromium.org/620983002/diff/120001/chrome/android/java/res...
chrome/android/java/res/drawable/website_settings_permission_spinner_item_background.xml:16:
<item><bitmap android:gravity="right"
android:src="@drawable/page_info_arrow_down" /></item>
On 2014/10/16 00:24:25, Ted C wrote:
> With this, I think the text can draw over the dropdown arrow.
> 
> You might want to apply end padding to the spinner to make sure that isn't
> possible.

Yup, it's true, thanks for pointing this out :) Added padding, see test:
http://imgur.com/LwzojsW

https://codereview.chromium.org/620983002/diff/120001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/620983002/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:335:

On 2014/10/16 00:24:25, Ted C wrote:
> In the transition to PageInfoPermissionEntry, we should be using just that
here.
> 
> i.e.
> 
> PageInfoPermissionEntry entry  = (PageInfoPermissionEntry)
> parent.getItemAtPosition(pos);
> nativeOnPermissionSettingChanged(mNativeWebsiteSettingsPopup, entry.type,
> entry.value);
> 
> No need for the tags or anything else here (I think)

Nice, thanks :) Done.

[sashab, 2014-10-16 05:52:18]

sashab@chromium.org changed reviewers:
+ markusheintz@chromium.org
- markusheintz@google.com

[markusheintz_, 2014-10-16 16:06:27]

On 2014/10/16 05:52:02, sasha_b wrote:
> markusheintz: please review changes to chrome/browser/ui/website_settings/*
> (especially changes in website_settings.cc)
> 
> tedchoc: please review changes in chrome/browser/ui/android/ and
> chrome/android/java :)
> 
>
https://codereview.chromium.org/620983002/diff/120001/chrome/android/java/res...
> File
>
chrome/android/java/res/drawable/website_settings_permission_spinner_item_background.xml
> (right):
> 
>
https://codereview.chromium.org/620983002/diff/120001/chrome/android/java/res...
>
chrome/android/java/res/drawable/website_settings_permission_spinner_item_background.xml:16:
> <item><bitmap android:gravity="right"
> android:src="@drawable/page_info_arrow_down" /></item>
> On 2014/10/16 00:24:25, Ted C wrote:
> > With this, I think the text can draw over the dropdown arrow.
> > 
> > You might want to apply end padding to the spinner to make sure that isn't
> > possible.
> 
> Yup, it's true, thanks for pointing this out :) Added padding, see test:
> http://imgur.com/LwzojsW
> 
>
https://codereview.chromium.org/620983002/diff/120001/chrome/android/java/src...
> File
> chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
> (right):
> 
>
https://codereview.chromium.org/620983002/diff/120001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:335:
> 
> On 2014/10/16 00:24:25, Ted C wrote:
> > In the transition to PageInfoPermissionEntry, we should be using just that
> here.
> > 
> > i.e.
> > 
> > PageInfoPermissionEntry entry  = (PageInfoPermissionEntry)
> > parent.getItemAtPosition(pos);
> > nativeOnPermissionSettingChanged(mNativeWebsiteSettingsPopup, entry.type,
> > entry.value);
> > 
> > No need for the tags or anything else here (I think)
> 
> Nice, thanks :) Done.

Content settings changes LGTM

[Ted C, 2014-10-16 21:30:46]

Very close...just a few final little comments

https://codereview.chromium.org/620983002/diff/140001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/620983002/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:67:
public String toString() {
this should have an @Override

https://codereview.chromium.org/620983002/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:104:
*            retrieved for the visible entry.
for all your multi line javadoc, the following lines we align with the start of
the sentence above ("The WebContents") here

https://codereview.chromium.org/620983002/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:195:
private void setIsInternalPage(boolean isInternalPage) {
setIsInternalPage seems like a really odd name (especially for something that is
not actually persisted and only used for such a small portion of this flow).

This updates the url and connection information.  Maybe just updatePageDetails
or something is more appropriate (and general enough to not be
updatePageUrlAndConnectionMessageAndSecurityTypeAndTheKitchenSync)

https://codereview.chromium.org/620983002/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:207:
// TODO(sashab): How could this work for RTL languages?
This "should" be fine for RTL as these are two separate sentences, so appending
them with a space between should be ok in both.

With that said, we having something that we are going to upstream (previously
called StringLinkifier, name TBD) that will allow you to specify tokens in your
string i.e. <red></red> and <bold></bold> and replace those with Spans.  For
now, I think you're safe though.

https://codereview.chromium.org/620983002/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:243:
assert false : "Invalid security level specified: " + mSecurityLevel;
if this ever falls into this case, schemeColorId will be -1 and it will crash. 
That is probably ok because it implies a programming oversight, but just
something to be aware of (asserts are only on dev local builds with the flag set
on your device).

https://codereview.chromium.org/620983002/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:375:
public static void show(Context context, Tab currentTab) {
Is getSecurityLevel the only reason you need tab?

If so, I just exposed a method in ToolbarModel directly because there is no need
for Tab to be involved at all:
https://codereview.chromium.org/646943005/

https://codereview.chromium.org/620983002/diff/140001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/620983002/diff/140001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings.cc:350: if
(url.SchemeIs(chrome::kChromeUINativeScheme)) {
I think it should be ok to ugly if def this in the above conditional just to
avoid them diverging unexpectedly.

[sashab, 2014-10-17 00:51:28]

Ptal tedchoc - should we do tablet work in a follow-up CL?

https://codereview.chromium.org/620983002/diff/140001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/620983002/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:67:
public String toString() {
On 2014/10/16 21:30:45, Ted C wrote:
> this should have an @Override

Done.

https://codereview.chromium.org/620983002/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:104:
*            retrieved for the visible entry.
On 2014/10/16 21:30:45, Ted C wrote:
> for all your multi line javadoc, the following lines we align with the start
of
> the sentence above ("The WebContents") here

Fixed in all comments.

https://codereview.chromium.org/620983002/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:195:
private void setIsInternalPage(boolean isInternalPage) {
On 2014/10/16 21:30:45, Ted C wrote:
> setIsInternalPage seems like a really odd name (especially for something that
is
> not actually persisted and only used for such a small portion of this flow).
> 
> This updates the url and connection information.  Maybe just updatePageDetails
> or something is more appropriate (and general enough to not be
> updatePageUrlAndConnectionMessageAndSecurityTypeAndTheKitchenSync)

Yup, agreed :) Done.

https://codereview.chromium.org/620983002/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:207:
// TODO(sashab): How could this work for RTL languages?
On 2014/10/16 21:30:45, Ted C wrote:
> This "should" be fine for RTL as these are two separate sentences, so
appending
> them with a space between should be ok in both.
> 
> With that said, we having something that we are going to upstream (previously
> called StringLinkifier, name TBD) that will allow you to specify tokens in
your
> string i.e. <red></red> and <bold></bold> and replace those with Spans.  For
> now, I think you're safe though.

Great, thanks. Was going to put a TODO for the spans but I figure you'll go
through the codebase looking for them when the time comes.

https://codereview.chromium.org/620983002/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:243:
assert false : "Invalid security level specified: " + mSecurityLevel;
On 2014/10/16 21:30:45, Ted C wrote:
> if this ever falls into this case, schemeColorId will be -1 and it will crash.

> That is probably ok because it implies a programming oversight, but just
> something to be aware of (asserts are only on dev local builds with the flag
set
> on your device).

Oh, I didn't know that the asserts compiled into release. In that case, I'll set
schemeColorId and connectionMessageId to some sensible defaults in this
'default' block.

https://codereview.chromium.org/620983002/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:375:
public static void show(Context context, Tab currentTab) {
On 2014/10/16 21:30:45, Ted C wrote:
> Is getSecurityLevel the only reason you need tab?
> 
> If so, I just exposed a method in ToolbarModel directly because there is no
need
> for Tab to be involved at all:
> https://codereview.chromium.org/646943005/

That new method is awesome :D Nice, now I only need the webContents.

https://codereview.chromium.org/620983002/diff/140001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/620983002/diff/140001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings.cc:350: if
(url.SchemeIs(chrome::kChromeUINativeScheme)) {
On 2014/10/16 21:30:45, Ted C wrote:
> I think it should be ok to ugly if def this in the above conditional just to
> avoid them diverging unexpectedly.

Thought of a cool way to do this :) Done.

[Ted C, 2014-10-17 01:09:03]

lgtm - yeah, let's tackle the tablet stuff in another change.  This one is big
enough :-)

[sashab, 2014-10-17 02:32:29]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-10-17 02:34:14]

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

[commit-bot: I haz the power, 2014-10-17 03:30:54]

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

[commit-bot: I haz the power, 2014-10-17 03:30:56]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[sashab, 2014-10-17 03:32:05]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-10-17 03:32:18]

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

[commit-bot: I haz the power, 2014-10-17 04:28:51]

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

[commit-bot: I haz the power, 2014-10-17 04:28:53]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[sashab, 2014-10-20 04:41:25]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-10-20 04:42:48]

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

[commit-bot: I haz the power, 2014-10-20 05:38:05]

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

[commit-bot: I haz the power, 2014-10-20 05:38:07]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sashab, 2014-10-20 05:51:14]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-10-20 05:51:57]

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

[commit-bot: I haz the power, 2014-10-20 06:57:21]

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

[commit-bot: I haz the power, 2014-10-20 06:57:23]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Finnur, 2014-10-20 09:24:11]

So close! :)

It looks to me like you are getting a NOTREACHED in
LocationIconViewTest.HideOnSecondClick...
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

[Finnur, 2014-10-21 15:49:34]

I think I have a solution to your test failure.

https://codereview.chromium.org/620983002/diff/180001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings_ui.cc (right):

https://codereview.chromium.org/620983002/diff/180001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings_ui.cc:282: break;
I would delete this default: case.

If you do, you'll get a compile error because a case statement is missing for
SITE_IDENTITY_STATUS_INTERNAL_PAGE. When you add that and make a similar fix
later in this file (see below), the interactive test will start passing.

What should the resource_id should be for this new case? Perhaps best if if you
answer that. I used IDR_PAGEINFO_INFO, but maybe IDR_PAGEINFO_GOOD is
appropriate?

https://codereview.chromium.org/620983002/diff/180001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings_ui.cc:315: break;
Also delete this default case. Then you'll find that
SITE_CONNECTION_STATUS_INTERNAL_PAGE is also missing.

[Finnur, 2014-10-21 15:54:19]

... a compile error on Linux (is what I should have said) and future explorers
will get an error there if they add to the enum without updating the switch
statement.

[sashab, 2014-10-22 09:11:08]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-10-22 09:12:22]

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

[sashab, 2014-10-22 09:35:00]

https://codereview.chromium.org/620983002/diff/180001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings_ui.cc (right):

https://codereview.chromium.org/620983002/diff/180001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings_ui.cc:282: break;
On 2014/10/21 15:49:34, Finnur wrote:
> I would delete this default: case.
> 
> If you do, you'll get a compile error because a case statement is missing for
> SITE_IDENTITY_STATUS_INTERNAL_PAGE. When you add that and make a similar fix
> later in this file (see below), the interactive test will start passing.
> 
> What should the resource_id should be for this new case? Perhaps best if if
you
> answer that. I used IDR_PAGEINFO_INFO, but maybe IDR_PAGEINFO_GOOD is
> appropriate?

Added INTERNAL_PAGE to have IDR_PAGEINFO_INFO. The truth is I don't think it
matters, since this code path can't be reached outside of the test (internal
pages on desktop don't have connection info bubbles... yet - they will when we
overhaul the origin info bubble on desktop).

https://codereview.chromium.org/620983002/diff/180001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings_ui.cc:315: break;
On 2014/10/21 15:49:34, Finnur wrote:
> Also delete this default case. Then you'll find that
> SITE_CONNECTION_STATUS_INTERNAL_PAGE is also missing.

Done.

[commit-bot: I haz the power, 2014-10-22 10:08:45]

Committed patchset #19 (id:360001)

[commit-bot: I haz the power, 2014-10-22 10:09:42]

Patchset 19 (id:??) landed as
https://crrev.com/97894ce12fc78d6f59e63314aefead3857816a28
Cr-Commit-Position: refs/heads/master@{#300658}

[Finnur, 2014-10-23 10:36:35]

https://codereview.chromium.org/620983002/diff/360001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/620983002/diff/360001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:183:
case ContentSettingsType.CONTENT_SETTINGS_TYPE_NOTIFICATIONS:
Hmmm... One thing I stumbled across when trying to consolidate icons. We (you
and I) don't seem to agree on what ContentSettingsType governs Push
Notifications. As far as I can tell, it is CONTENT_SETTINGS_TYPE_PUSH_MESSAGING
but you are using CONTENT_SETTINGS_TYPE_NOTIFICATIONS.

I believe mine is correct, because when I go to a test site and allow push
notifications, I query this value to find that decision. What say you? Am I
doing it wrong?