Add "Site Settings" option to Clear Browsing Data on Android

Add "Site Settings" option to Clear Browsing Data on Android

Implement site setting removal in BrowsingDataRemoverDelegate
Add preference for site settings checkbox
Add site settings counter
Add site settings ui to CBD dialog on Android

https://screenshot.googleplex.com/FJrhrqVtd6J.png

BUG=681523
Review-Url: https://codereview.chromium.org/2816723002
Cr-Commit-Position: refs/heads/master@{#470270}
Committed: https://chromium.googlesource.com/chromium/src/+/b94383086bbb49474afd397fa447a8b940c84684

[dullweber, 2017-04-12 12:38:33]

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

[commit-bot: I haz the power, 2017-04-12 12:38:46]

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-04-12 13:37:32]

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

[commit-bot: I haz the power, 2017-04-12 13:37:33]

Dry run: This issue passed the CQ dry run.

[dullweber, 2017-04-27 13:50:51]

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

[commit-bot: I haz the power, 2017-04-27 13:51:15]

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-04-27 13:55:25]

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

[commit-bot: I haz the power, 2017-04-27 13:55:26]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[dullweber, 2017-04-28 09:40:02]

dullweber@chromium.org changed reviewers:
+ msramek@chromium.org

[dullweber, 2017-04-28 09:40:03]

Hi Martin,
please review this CL

[dullweber, 2017-04-28 09:43:20]

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

[dullweber, 2017-04-28 09:43:20]

Hi Theresa,
please review the changes in chrome/android. I'm adding an option to delete site
settings to the new CBD dialog.

[Theresa, 2017-04-28 14:56:03]

chrome/android lgtm % nits. Will you please upload screenshots to the bug?

https://codereview.chromium.org/2816723002/diff/40001/chrome/android/java/res...
File chrome/android/java/res/drawable/ic_tv_options_rotated_grey.xml (right):

https://codereview.chromium.org/2816723002/diff/40001/chrome/android/java/res...
chrome/android/java/res/drawable/ic_tv_options_rotated_grey.xml:5: 
This is from ic_tv_options_input_settings, right? Can we use the full name so
that it's easier for folks looking for "ic_tv_options_input_settings" to find in
code search?

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

https://codereview.chromium.org/2816723002/diff/40001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:597: Site settings
This string already exists as IDS_PREFS_SITE_SETTINGS

[dullweber, 2017-05-02 09:43:05]

Description was changed from

==========
Add "Site Settings" option to Clear Browsing Data on Android

Implement site setting removal in BrowsingDataRemoverDelegate
Add preference for site settings checkbox
Add site settings counter
Add site settings ui to CBD dialog on Android

BUG=681523
==========

to

==========
Add "Site Settings" option to Clear Browsing Data on Android

Implement site setting removal in BrowsingDataRemoverDelegate
Add preference for site settings checkbox
Add site settings counter
Add site settings ui to CBD dialog on Android

https://screenshot.googleplex.com/FJrhrqVtd6J.png

BUG=681523
==========

[dullweber, 2017-05-02 09:48:46]

Thanks, I uploaded a screenshot and linked it in the description

https://codereview.chromium.org/2816723002/diff/40001/chrome/android/java/res...
File chrome/android/java/res/drawable/ic_tv_options_rotated_grey.xml (right):

https://codereview.chromium.org/2816723002/diff/40001/chrome/android/java/res...
chrome/android/java/res/drawable/ic_tv_options_rotated_grey.xml:5: 
On 2017/04/28 14:56:03, Theresa wrote:
> This is from ic_tv_options_input_settings, right? Can we use the full name so
> that it's easier for folks looking for "ic_tv_options_input_settings" to find
in
> code search?

This icon is ic_tv_options_input_settings but rotated by 90 degrees (see
screenshot). I renamed it to ic_tv_options_input_settings_rotated_grey.

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

https://codereview.chromium.org/2816723002/diff/40001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:597: Site settings
On 2017/04/28 14:56:03, Theresa wrote:
> This string already exists as IDS_PREFS_SITE_SETTINGS

I removed this string, thanks

[dullweber, 2017-05-02 09:51:42]

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

[dullweber, 2017-05-02 09:51:43]

bauerb@chromium.org: Please review changes in
chrome/browser/android/browsing_data/browsing_data_bridge.cc

[Bernhard Bauer, 2017-05-03 14:29:21]

https://codereview.chromium.org/2816723002/diff/60001/chrome/android/java/res...
File chrome/android/java/res/drawable/ic_tv_options_input_settings_grey.svg
(right):

https://codereview.chromium.org/2816723002/diff/60001/chrome/android/java/res...
chrome/android/java/res/drawable/ic_tv_options_input_settings_grey.svg:1: <?xml
version="1.0" encoding="UTF-8" standalone="no"?>
This resource isn't actually used (and Android doesn't directly support SVG
files as resources anyway).

https://codereview.chromium.org/2816723002/diff/60001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_counter_factory.cc (right):

https://codereview.chromium.org/2816723002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_counter_factory.cc:99: return
base::MakeUnique<browsing_data::SiteSettingsCounter>(
Nit: If the body of an if-statement is more than one line, it requires braces.

https://codereview.chromium.org/2816723002/diff/60001/chrome/browser/content_...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

https://codereview.chromium.org/2816723002/diff/60001/chrome/browser/content_...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1830:
base::PlatformThread::Sleep(TestTimeouts::tiny_timeout());
Ugh, real-time timers. Can't we inject a timer for testing into the
HostContentSettingsMap instead?

[dullweber, 2017-05-03 15:59:13]

Thanks, I removed the wrong svg file and changed the test to inject a
base::Clock.

https://codereview.chromium.org/2816723002/diff/60001/chrome/android/java/res...
File chrome/android/java/res/drawable/ic_tv_options_input_settings_grey.svg
(right):

https://codereview.chromium.org/2816723002/diff/60001/chrome/android/java/res...
chrome/android/java/res/drawable/ic_tv_options_input_settings_grey.svg:1: <?xml
version="1.0" encoding="UTF-8" standalone="no"?>
On 2017/05/03 14:29:21, Bernhard Bauer wrote:
> This resource isn't actually used (and Android doesn't directly support SVG
> files as resources anyway).

Thanks for noticing this file. I should not have been part of the cl

https://codereview.chromium.org/2816723002/diff/60001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_counter_factory.cc (right):

https://codereview.chromium.org/2816723002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_counter_factory.cc:99: return
base::MakeUnique<browsing_data::SiteSettingsCounter>(
On 2017/05/03 14:29:21, Bernhard Bauer wrote:
> Nit: If the body of an if-statement is more than one line, it requires braces.

Done.

https://codereview.chromium.org/2816723002/diff/60001/chrome/browser/content_...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

https://codereview.chromium.org/2816723002/diff/60001/chrome/browser/content_...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1830:
base::PlatformThread::Sleep(TestTimeouts::tiny_timeout());
On 2017/05/03 14:29:21, Bernhard Bauer wrote:
> Ugh, real-time timers. Can't we inject a timer for testing into the
> HostContentSettingsMap instead?

I changed the PrefProvider to use a base::Clock. It is injected from
HostContentSettingsMap. I also updated other tests that were using timeouts to
guarantee a clock difference.

[Bernhard Bauer, 2017-05-03 16:05:39]

Awesome, thanks! LGTM with a pointer-wrangling nit:

https://codereview.chromium.org/2816723002/diff/80001/chrome/browser/content_...
File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
(right):

https://codereview.chromium.org/2816723002/diff/80001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:625:
auto* clock = new base::SimpleTestClock();
Could you create this in a std::unique_ptr with base::MakeUnique, then get a raw
pointer out of it before passing it into the provider? At this point I'm
conditioned to dislike seeing raw "new" statements ;-) In other places too.

[dullweber, 2017-05-03 16:23:00]

https://codereview.chromium.org/2816723002/diff/80001/chrome/browser/content_...
File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
(right):

https://codereview.chromium.org/2816723002/diff/80001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:625:
auto* clock = new base::SimpleTestClock();
On 2017/05/03 16:05:39, Bernhard Bauer wrote:
> Could you create this in a std::unique_ptr with base::MakeUnique, then get a
raw
> pointer out of it before passing it into the provider? At this point I'm
> conditioned to dislike seeing raw "new" statements ;-) In other places too.

Done.

[dullweber, 2017-05-04 09:18:03]

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

[commit-bot: I haz the power, 2017-05-04 09:18:12]

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-05-04 09:55:52]

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

[commit-bot: I haz the power, 2017-05-04 09:55:53]

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

[dullweber, 2017-05-04 10:41:53]

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

[commit-bot: I haz the power, 2017-05-04 10:42:00]

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-05-04 12:34:36]

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

[commit-bot: I haz the power, 2017-05-04 12:34:37]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dullweber, 2017-05-04 13:47:18]

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

[commit-bot: I haz the power, 2017-05-04 13:47:33]

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-05-04 14:35:48]

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

[commit-bot: I haz the power, 2017-05-04 14:35:49]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[msramek, 2017-05-08 10:53:57]

LGTM % nits and a test improvement suggestion.

Awesome that we're finally getting this functionality \o/

https://codereview.chromium.org/2816723002/diff/160001/chrome/browser/browsin...
File chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc
(right):

https://codereview.chromium.org/2816723002/diff/160001/chrome/browser/browsin...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc:716: auto*
registry = content_settings::ContentSettingsRegistry::GetInstance();
nit: const

https://codereview.chromium.org/2816723002/diff/160001/chrome/browser/browsin...
File
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc
(right):

https://codereview.chromium.org/2816723002/diff/160001/chrome/browser/browsin...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc:1594:
TEST_F(ChromeBrowsingDataRemoverDelegateTest, RemoveAllContentSettings) {
The test is named "RemoveAll", but in fact it tests filtering, i.e. removing not
all :)

Let's just call it "RemoveContentSettings", and I recommend testing both
complete and partial deletions:
1. Remove some settings.
2. Test that some settings remained.
3. Then remove all settings.
4. Test that only the wildcard remained.

https://codereview.chromium.org/2816723002/diff/160001/chrome/browser/browsin...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc:1597:
CONTENT_SETTINGS_TYPE_GEOLOCATION, "",
nit: std::string()

https://codereview.chromium.org/2816723002/diff/160001/chrome/browser/browsin...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc:1602:

Let's add one more content type in order to document that this deletes all
content settings, not just geolocation.

https://codereview.chromium.org/2816723002/diff/160001/chrome/browser/browsin...
File chrome/browser/browsing_data/site_settings_counter_unittest.cc (right):

https://codereview.chromium.org/2816723002/diff/160001/chrome/browser/browsin...
chrome/browser/browsing_data/site_settings_counter_unittest.cc:1: // Copyright
(c) 2017 The Chromium Authors. All rights reserved.
nit: We no longer use (c)

https://codereview.chromium.org/2816723002/diff/160001/components/browsing_da...
File components/browsing_data/core/counters/site_settings_counter.cc (right):

https://codereview.chromium.org/2816723002/diff/160001/components/browsing_da...
components/browsing_data/core/counters/site_settings_counter.cc:26:
std::set<ContentSettingsPattern> patterns;
style: Include what you use.

https://codereview.chromium.org/2816723002/diff/160001/components/browsing_da...
File components/browsing_data/core/counters/site_settings_counter.h (right):

https://codereview.chromium.org/2816723002/diff/160001/components/browsing_da...
components/browsing_data/core/counters/site_settings_counter.h:9: #include
<vector>
Not needed.

https://codereview.chromium.org/2816723002/diff/160001/components/browsing_da...
components/browsing_data/core/counters/site_settings_counter.h:28:
scoped_refptr<HostContentSettingsMap> map_;
Do we need to keep this pointer? We don't observe HCSM, just call a method on
it...

[dullweber, 2017-05-08 15:50:24]

Thanks

https://codereview.chromium.org/2816723002/diff/160001/chrome/browser/browsin...
File chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc
(right):

https://codereview.chromium.org/2816723002/diff/160001/chrome/browser/browsin...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc:716: auto*
registry = content_settings::ContentSettingsRegistry::GetInstance();
On 2017/05/08 10:53:56, msramek (recovering) wrote:
> nit: const

Done.

https://codereview.chromium.org/2816723002/diff/160001/chrome/browser/browsin...
File
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc
(right):

https://codereview.chromium.org/2816723002/diff/160001/chrome/browser/browsin...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc:1597:
CONTENT_SETTINGS_TYPE_GEOLOCATION, "",
On 2017/05/08 10:53:56, msramek (recovering) wrote:
> nit: std::string()

Done.

https://codereview.chromium.org/2816723002/diff/160001/chrome/browser/browsin...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc:1602:

On 2017/05/08 10:53:56, msramek (recovering) wrote:
> Let's add one more content type in order to document that this deletes all
> content settings, not just geolocation.

Done.

https://codereview.chromium.org/2816723002/diff/160001/chrome/browser/browsin...
File chrome/browser/browsing_data/site_settings_counter_unittest.cc (right):

https://codereview.chromium.org/2816723002/diff/160001/chrome/browser/browsin...
chrome/browser/browsing_data/site_settings_counter_unittest.cc:1: // Copyright
(c) 2017 The Chromium Authors. All rights reserved.
On 2017/05/08 10:53:56, msramek (recovering) wrote:
> nit: We no longer use (c)

Oh, right

https://codereview.chromium.org/2816723002/diff/160001/components/browsing_da...
File components/browsing_data/core/counters/site_settings_counter.cc (right):

https://codereview.chromium.org/2816723002/diff/160001/components/browsing_da...
components/browsing_data/core/counters/site_settings_counter.cc:26:
std::set<ContentSettingsPattern> patterns;
On 2017/05/08 10:53:56, msramek (recovering) wrote:
> style: Include what you use.

I added includes for <set> and content_settings_pattern.h

https://codereview.chromium.org/2816723002/diff/160001/components/browsing_da...
File components/browsing_data/core/counters/site_settings_counter.h (right):

https://codereview.chromium.org/2816723002/diff/160001/components/browsing_da...
components/browsing_data/core/counters/site_settings_counter.h:28:
scoped_refptr<HostContentSettingsMap> map_;
On 2017/05/08 10:53:56, msramek (recovering) wrote:
> Do we need to keep this pointer? We don't observe HCSM, just call a method on
> it...

I need the pointer because I can't access the HostContentSettingsMapFactory from
components/

[dullweber, 2017-05-08 15:50:34]

The CQ bit was checked by dullweber@chromium.org

[dullweber, 2017-05-08 15:50:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org, bauerb@chromium.org, msramek@chromium.org
Link to the patchset: https://codereview.chromium.org/2816723002/#ps180001
(title: "fix comments")

[commit-bot: I haz the power, 2017-05-08 15:51:01]

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-05-08 15:56:31]

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

[commit-bot: I haz the power, 2017-05-08 15:56:32]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dullweber, 2017-05-08 16:02:16]

The CQ bit was checked by dullweber@chromium.org

[dullweber, 2017-05-08 16:02:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
msramek@chromium.org, twellington@chromium.org, bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2816723002/#ps200001
(title: "fix deps")

[commit-bot: I haz the power, 2017-05-08 16:02:53]

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-05-08 16:19:41]

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

[commit-bot: I haz the power, 2017-05-08 16:19:42]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[dullweber, 2017-05-09 09:07:30]

The CQ bit was checked by dullweber@chromium.org

[commit-bot: I haz the power, 2017-05-09 09:08:11]

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-05-09 09:13:41]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1494320850882210,
"parent_rev": "0cafc95d595689bfde0c550c60482e2d0b67ea43", "commit_rev":
"b94383086bbb49474afd397fa447a8b940c84684"}

[commit-bot: I haz the power, 2017-05-09 09:13:52]

Description was changed from

==========
Add "Site Settings" option to Clear Browsing Data on Android

Implement site setting removal in BrowsingDataRemoverDelegate
Add preference for site settings checkbox
Add site settings counter
Add site settings ui to CBD dialog on Android

https://screenshot.googleplex.com/FJrhrqVtd6J.png

BUG=681523
==========

to

==========
Add "Site Settings" option to Clear Browsing Data on Android

Implement site setting removal in BrowsingDataRemoverDelegate
Add preference for site settings checkbox
Add site settings counter
Add site settings ui to CBD dialog on Android

https://screenshot.googleplex.com/FJrhrqVtd6J.png

BUG=681523

Review-Url: https://codereview.chromium.org/2816723002
Cr-Commit-Position: refs/heads/master@{#470270}
Committed:
https://chromium.googlesource.com/chromium/src/+/b94383086bbb49474afd397fa447...
==========

[commit-bot: I haz the power, 2017-05-09 09:13:55]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/b94383086bbb49474afd397fa447...