[subresource_filter] Add the Content Settings UI for Android

[subresource_filter] Add the Content Settings UI for Android

BUG=689487
Review-Url: https://codereview.chromium.org/2785913002
Cr-Commit-Position: refs/heads/master@{#462151}
Committed: https://chromium.googlesource.com/chromium/src/+/3f46d56fe6658eb13d6b96b31fab2233b43d856f

[Charlie Harrison, 2017-03-30 03:34:04]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 03:34:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 03:38:11]

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

[commit-bot: I haz the power, 2017-03-30 03:38:11]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Charlie Harrison, 2017-03-30 12:22:17]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 12:22:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 12:35:33]

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

[commit-bot: I haz the power, 2017-03-30 12:35:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie Harrison, 2017-03-30 12:43:23]

Description was changed from

==========
[subresource_filter] Add the Content Settings UI for Android

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

to

==========
[subresource_filter] Add the Content Settings UI for Android

BUG=689487
==========

[Charlie Harrison, 2017-03-30 12:43:37]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 12:43:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 13:28:44]

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

[commit-bot: I haz the power, 2017-03-30 13:28:45]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2017-03-30 13:31:16]

csharrison@chromium.org changed reviewers:
+ melandory@chromium.org

[Charlie Harrison, 2017-03-30 13:31:17]

Tanja: FYI

I'll send this out for broader review once I collect some screenshots.

[Charlie Harrison, 2017-03-30 13:32:24]

+subresource-filter-reviews@

[melandory, 2017-03-30 13:39:58]

We probably should guard the UI with SafeBrowsingSubresourceFilterExperimentalUI
feature. Otherwise we'll give the inconsistent experience.

https://codereview.chromium.org/2785913002/diff/40001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2785913002/diff/40001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:2797: Subresource Filter
We already have this string in chrome/app/generated_resources.grd

[Charlie Harrison, 2017-03-30 17:49:33]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 17:49:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2017-03-30 17:54:30]

csharrison@chromium.org changed reviewers:
+ twellington@chromium.org

[Charlie Harrison, 2017-03-30 17:54:31]

melandory: Thanks, I've guarded this with ChromeFeatureList based on the
experimental UI. As we discussed offline I renamed the android string IDS so
there is no collision. 

twellington: Would you PTAL at everything? The linked bug has screenshots of
this work. Note that the strings are placeholders, the team just wants to get
the engineering work out of the way before we launch.

I'm not so familiar with this code and in particular the code in
SingleWebsitePreferences.java was a pain to get working.

Also, is there a simple way to access strings in
chrome/app/generated_resources.grd? In this patch I've duplicated it in an
android specific string.

[lgarron, 2017-03-30 18:00:42]

How is this going to be exposed to the user?
It seems this is being exposed like a permission, but I don't see permissions
people involved in the bugs.

One of the relevant bugs shows "Subresource filter" in Android Page Info in a
screenshot, but that probably won't make sense to most users.

[Theresa, 2017-03-30 18:22:29]

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

[Theresa, 2017-03-30 18:22:30]

+finnur@ is more familiar with site settings.

[commit-bot: I haz the power, 2017-03-30 18:33:11]

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

[commit-bot: I haz the power, 2017-03-30 18:33:15]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2017-03-30 19:09:01]

lgarron please see crbug.com/689992 which has links to the full UI mocks. These
strings are just placeholders and are guarded behind a flag so users won't
actually see "Subresource Filter"

[lgarron, 2017-03-30 19:13:11]

On 2017/03/30 at 19:09:01, csharrison wrote:
> lgarron please see crbug.com/689992 which has links to the full UI mocks.
These strings are just placeholders and are guarded behind a flag so users won't
actually see "Subresource Filter"

Sounds good; thanks for the details!

[Finnur, 2017-03-31 10:04:58]

General comments first...

I find this setting to be confusing. I don't think it is clear to the user at
least from any of the pictures (in the bug) what this actually does.

And these look more like screenshots from your implementation than mocks from a
designer. Is that a fair assessment? I'm wondering if anyone from UI has had a
say in this?

[Finnur, 2017-03-31 10:22:35]

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/res...
File chrome/android/java/res/xml/single_website_preferences.xml (right):

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/res...
chrome/android/java/res/xml/single_website_preferences.xml:50:
<org.chromium.chrome.browser.preferences.ChromeBaseListPreference
On line 33 there is a link to a bug that discusses the order of these
permissions. 

The agreed upon order is here...
https://docs.google.com/spreadsheets/d/1bVRBeEcJ3YSJNMKq9U9pm-dGFtVRDryzfojoD...
... and I suggest you talk to emilyschechter and/or Chris (palmer@) about where
exactly to place this (and have them update the doc so we know).

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/res...
File chrome/android/java/res/xml/site_settings_preferences.xml (right):

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/res...
chrome/android/java/res/xml/site_settings_preferences.xml:45:
android:key="subresource_filter" />
Same comment here (about the order)

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java
(right):

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java:155:
new ResourceItem(R.drawable.permission_popups,
This is the wrong icon to use. You should consult with UI which icon is correct
for this (they'll know where to pull it from -- likely Google's material design
icon library).

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java:158:
ContentSetting.BLOCK, 0, 0));
These blocks are alphabetically ordered based on the ContentSettingsType. Move
to line 151 please.

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsCategory.java
(right):

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsCategory.java:140:
}
Move block to line 130 (alphabetical order).

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsCategory.java:187:
}
Move to line 182.

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsCategory.java:298:
}
Move this block to line 286.

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferences.java
(right):

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferences.java:124:
getPreferenceScreen().removePreference(findPreference(SUBRESOURCE_FILTER_KEY));
Move up one line.

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferences.java:138:
}
I don't think this is necessary. Isn't this taken care of by line 169-171?

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:2796: <message
name="IDS_SUBRESOURCE_FILTER_HEADER_ANDROID" desc="Header for the subresource
filter setting on the Content Settings dialog.">
The _ANDROID suffix is redundant. This file and these strings are Android only.

Also, this belongs next to ids_popup_permission_title and all those higher up in
this file. There's also a CHAR_LIMIT that is needed for translators (you'll see
what I mean once you move this up).

[Charlie Harrison, 2017-03-31 13:21:42]

Thanks for the review, finnur. I've tried to address most of your comments.

In the linked bug I've added palmer (cc'd here too) to the discussion on setting
ordering, and given a bit more context for this change.

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/res...
File chrome/android/java/res/xml/single_website_preferences.xml (right):

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/res...
chrome/android/java/res/xml/single_website_preferences.xml:50:
<org.chromium.chrome.browser.preferences.ChromeBaseListPreference
On 2017/03/31 10:22:34, Finnur wrote:
> On line 33 there is a link to a bug that discusses the order of these
> permissions. 
> 
> The agreed upon order is here...
>
https://docs.google.com/spreadsheets/d/1bVRBeEcJ3YSJNMKq9U9pm-dGFtVRDryzfojoD...
> ... and I suggest you talk to emilyschechter and/or Chris (palmer@) about
where
> exactly to place this (and have them update the doc so we know).

I've left these as-is for now, but added palmer to the linked bug for thoughts.

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java
(right):

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java:155:
new ResourceItem(R.drawable.permission_popups,
On 2017/03/31 10:22:34, Finnur wrote:
> This is the wrong icon to use. You should consult with UI which icon is
correct
> for this (they'll know where to pull it from -- likely Google's material
design
> icon library).

Is there a way we can just use an existing icon as a placeholder? The current
icon for launch is not quite ready yet.

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java:158:
ContentSetting.BLOCK, 0, 0));
On 2017/03/31 10:22:34, Finnur wrote:
> These blocks are alphabetically ordered based on the ContentSettingsType. Move
> to line 151 please.

Done.

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsCategory.java
(right):

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsCategory.java:140:
}
On 2017/03/31 10:22:34, Finnur wrote:
> Move block to line 130 (alphabetical order).

Done.

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsCategory.java:187:
}
On 2017/03/31 10:22:34, Finnur wrote:
> Move to line 182.

Done.

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsCategory.java:298:
}
On 2017/03/31 10:22:34, Finnur wrote:
> Move this block to line 286.

Done. Just the showSubresourceFilterSites method right?

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferences.java
(right):

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferences.java:124:
getPreferenceScreen().removePreference(findPreference(SUBRESOURCE_FILTER_KEY));
On 2017/03/31 10:22:34, Finnur wrote:
> Move up one line.

Done.

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferences.java:138:
}
On 2017/03/31 10:22:34, Finnur wrote:
> I don't think this is necessary. Isn't this taken care of by line 169-171?

I'm not 100% certain why, but without this line we just get an empty row in the
site settings page.

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:2796: <message
name="IDS_SUBRESOURCE_FILTER_HEADER_ANDROID" desc="Header for the subresource
filter setting on the Content Settings dialog.">
On 2017/03/31 10:22:34, Finnur wrote:
> The _ANDROID suffix is redundant. This file and these strings are Android
only.
> 
> Also, this belongs next to ids_popup_permission_title and all those higher up
in
> this file. There's also a CHAR_LIMIT that is needed for translators (you'll
see
> what I mean once you move this up).

I changed it to have the ANDROID suffix because we have the exact same string in
chrome/app/generated_resources.grd. A few comments ago I asked what the best
practice in this case is. I added the suffix because melandory was concerned
that duplicate IDS names could cause issues.

I've moved it and changed the name to IDS_SUBRESOURCE_FILTER_PERMISSION_TITLE.

[Finnur, 2017-03-31 13:47:24]

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/res...
File chrome/android/java/res/xml/single_website_preferences.xml (right):

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/res...
chrome/android/java/res/xml/single_website_preferences.xml:50:
<org.chromium.chrome.browser.preferences.ChromeBaseListPreference
Since this is most-privacy-sensitive-first ordering, I would move it to the
bottom of the list while this is being decided. Or, we can wait for the final
ordering decision.

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java
(right):

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java:155:
new ResourceItem(R.drawable.permission_popups,
That's what you are doing now, isn't it? There are no pre-defined placeholders,
if that's what you mean.

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsCategory.java
(right):

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsCategory.java:298:
}
Yeah. Asthetically, I'd say the subresourceFilterCategoryEnabled function should
be elsewhere, so that it is obvious where the showFooSites functions end, but
since it is only temporary I didn't bother mentioning it.

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferences.java
(right):

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferences.java:138:
}
That seems like something you should investigate. It would indicate that the new
setting isn't specified the way the others are, because they don't need to be
manually removed. I would compare it to one of the pre-existing permission,
say... popups and see if you are defining everything the same way.

https://codereview.chromium.org/2785913002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java
(right):

https://codereview.chromium.org/2785913002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java:152:
// they are ready.
You might end up using these exact strings and this is probably the wrong place
to document that they are placeholders. The icon, however, is surely wrong, so
I'd just say:

// TODO(csharrison): Swap out the placeholder icon once it is ready.

https://codereview.chromium.org/2785913002/diff/80001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2785913002/diff/80001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:648: Subresource Filter
nit: Should the f in filter be capitalized? UI should be able to give guidance.

[Charlie Harrison, 2017-03-31 14:56:45]

csharrison@chromium.org changed reviewers:
+ srahim@chromium.org

[Charlie Harrison, 2017-03-31 14:56:46]

Many thanks for the prompt reviews, finnur.

+srahim this is the CL which adds the content setting UI for subresource
filtering for Android. We don't have strings ready but would you look at the use
of placeholder strings?

There is also a question below about header capitalization.

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/res...
File chrome/android/java/res/xml/single_website_preferences.xml (right):

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/res...
chrome/android/java/res/xml/single_website_preferences.xml:50:
<org.chromium.chrome.browser.preferences.ChromeBaseListPreference
On 2017/03/31 13:47:23, Finnur wrote:
> Since this is most-privacy-sensitive-first ordering, I would move it to the
> bottom of the list while this is being decided. Or, we can wait for the final
> ordering decision.

Done.

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java
(right):

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java:155:
new ResourceItem(R.drawable.permission_popups,
On 2017/03/31 13:47:23, Finnur wrote:
> That's what you are doing now, isn't it? There are no pre-defined
placeholders,
> if that's what you mean. 

Yeah, that was my intention with reusing the popup icon.

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferences.java
(right):

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferences.java:138:
}
On 2017/03/31 13:47:23, Finnur wrote:
> That seems like something you should investigate. It would indicate that the
new
> setting isn't specified the way the others are, because they don't need to be
> manually removed. I would compare it to one of the pre-existing permission,
> say... popups and see if you are defining everything the same way.

My understanding is that onCreate calls addPreferencesFromResource which
initializes the preference screen with everything in
site_settings_preferences.xml.

Therefore, since we don't want our preference showing up without the flag I
think we have to remove it from the preference screen. I'm not sure other
preferences like popups have this requirement. They just always show up (unless
we're in the media sub menu).

If we only have the check below, the preference is still in the preference
screen, it just never has its title or click listener initialized.

https://codereview.chromium.org/2785913002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java
(right):

https://codereview.chromium.org/2785913002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/ContentSettingsResources.java:152:
// they are ready.
On 2017/03/31 13:47:23, Finnur wrote:
> You might end up using these exact strings and this is probably the wrong
place
> to document that they are placeholders. The icon, however, is surely wrong, so
> I'd just say:
> 
> // TODO(csharrison): Swap out the placeholder icon once it is ready.

Done.

https://codereview.chromium.org/2785913002/diff/80001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2785913002/diff/80001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:648: Subresource Filter
On 2017/03/31 13:47:23, Finnur wrote:
> nit: Should the f in filter be capitalized? UI should be able to give
guidance.

+srahim what do you think? Note that this is just a placeholder title until we
finalize the launch strings.

[Finnur, 2017-04-03 10:36:17]

LGTM, with the nits below addressed and an understanding that UI should be
involved before this is enabled for a wider audience (to sort out the strings
and icon).

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferences.java
(right):

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferences.java:138:
}
Hmm... maybe they should all be hidden to begin with and show on demand. 

But fair enough, I'm OK with this as is as long as
subresourceFilterCategoryEnabled is temporary and this call will eventually be
removed during that cleanup. At most a TODO(csharrison) is needed.

https://codereview.chromium.org/2785913002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferences.java
(right):

https://codereview.chromium.org/2785913002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferences.java:123:
getPreferenceScreen().removePreference(findPreference(SUBRESOURCE_FILTER_KEY));
Oops, sorry. My bad. Meant to say: Move to line 122.

[Charlie Harrison, 2017-04-03 14:01:47]

csharrison@chromium.org changed reviewers:
+ engedy@chromium.org, tedchoc@chromium.org

[Charlie Harrison, 2017-04-03 14:01:48]

Thanks finnur!

+engedy mainly for FYI. Do you mind using a placeholder icon for this while its
behind an experimental flag?

+tedchoc would you PTAL for OWNERS at
 java/res
 java/strings
 pref_service_bridge
 page_info_popup_android

Feel free to take a look at the preference implementation too, which finnur has
reviewed.

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferences.java
(right):

https://codereview.chromium.org/2785913002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferences.java:138:
}
On 2017/04/03 10:36:17, Finnur wrote:
> Hmm... maybe they should all be hidden to begin with and show on demand. 
> 
> But fair enough, I'm OK with this as is as long as
> subresourceFilterCategoryEnabled is temporary and this call will eventually be
> removed during that cleanup. At most a TODO(csharrison) is needed.

Added a TODO.

https://codereview.chromium.org/2785913002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferences.java
(right):

https://codereview.chromium.org/2785913002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SiteSettingsPreferences.java:123:
getPreferenceScreen().removePreference(findPreference(SUBRESOURCE_FILTER_KEY));
On 2017/04/03 10:36:17, Finnur wrote:
> Oops, sorry. My bad. Meant to say: Move to line 122.

My bad too, alphabetizing fail :)

[engedy, 2017-04-04 13:51:21]

Using placeholder icons as long as this is behind a feature sounds perfectly
reasonable to me.

FWIW LGTM % question: How are we going to control the visibility of the
subresource filter content setting entry based on whether the visited site
matches activation lists?

[Charlie Harrison, 2017-04-04 14:30:12]

On 2017/04/04 13:51:21, engedy wrote:
> Using placeholder icons as long as this is behind a feature sounds perfectly
> reasonable to me.
> 
> FWIW LGTM % question: How are we going to control the visibility of the
> subresource filter content setting entry based on whether the visited site
> matches activation lists?

We discussed this a little offline.

I think this is solvable with careful use of content settings, and our new
website setting storing metadata. Currently we don't do anything smart though,
but we're behind a flag so I think it should be OK.

[engedy, 2017-04-04 18:05:05]

On 2017/04/04 14:30:12, Charlie Harrison wrote:
> On 2017/04/04 13:51:21, engedy wrote:
> > Using placeholder icons as long as this is behind a feature sounds perfectly
> > reasonable to me.
> > 
> > FWIW LGTM % question: How are we going to control the visibility of the
> > subresource filter content setting entry based on whether the visited site
> > matches activation lists?
> 
> We discussed this a little offline.
> 
> I think this is solvable with careful use of content settings, and our new
> website setting storing metadata. Currently we don't do anything smart though,
> but we're behind a flag so I think it should be OK.

Sounds good.

[Ted C, 2017-04-05 17:15:54]

lgtm

https://codereview.chromium.org/2785913002/diff/120001/chrome/browser/ui/andr...
File chrome/browser/ui/android/page_info/page_info_popup_android.cc (right):

https://codereview.chromium.org/2785913002/diff/120001/chrome/browser/ui/andr...
chrome/browser/ui/android/page_info/page_info_popup_android.cc:120:
permissions_to_display.push_back(CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER);
this doesn't need to be conditionally included based on the UI feature right?  I
believe we only show items in the popup if they deviate from the default and in
this case you wouldn't have been able to change it from the default without the
ui turned on so I suspect this is ok.

[Charlie Harrison, 2017-04-05 17:25:17]

Thanks everyone. Sending to CQ

https://codereview.chromium.org/2785913002/diff/120001/chrome/browser/ui/andr...
File chrome/browser/ui/android/page_info/page_info_popup_android.cc (right):

https://codereview.chromium.org/2785913002/diff/120001/chrome/browser/ui/andr...
chrome/browser/ui/android/page_info/page_info_popup_android.cc:120:
permissions_to_display.push_back(CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER);
On 2017/04/05 17:15:54, Ted C wrote:
> this doesn't need to be conditionally included based on the UI feature right? 
I
> believe we only show items in the popup if they deviate from the default and
in
> this case you wouldn't have been able to change it from the default without
the
> ui turned on so I suspect this is ok.

Yep this is correct.

[Charlie Harrison, 2017-04-05 17:26:59]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2017-04-05 17:26:59]

The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org
Link to the patchset: https://codereview.chromium.org/2785913002/#ps120001
(title: "nits (trybots prev)")

[commit-bot: I haz the power, 2017-04-05 17:27:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 18:19:39]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491413219068830,
"parent_rev": "45d9003c7a48506caccc60d6a628a4951ed0ae50", "commit_rev":
"3f46d56fe6658eb13d6b96b31fab2233b43d856f"}

[commit-bot: I haz the power, 2017-04-05 18:20:28]

Description was changed from

==========
[subresource_filter] Add the Content Settings UI for Android

BUG=689487
==========

to

==========
[subresource_filter] Add the Content Settings UI for Android

BUG=689487

Review-Url: https://codereview.chromium.org/2785913002
Cr-Commit-Position: refs/heads/master@{#462151}
Committed:
https://chromium.googlesource.com/chromium/src/+/3f46d56fe6658eb13d6b96b31fab...
==========

[commit-bot: I haz the power, 2017-04-05 18:20:30]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/3f46d56fe6658eb13d6b96b31fab...