Remove last usage functions from HostContentSettingsMap and clean up prefs

Remove last usage functions from HostContentSettingsMap and clean up prefs

The last usage functions are:
1) Unused and have been for over 2.5 years. There is no concrete plan
to use them.
2) Not implemented in a very clean way, e.g.:
- There are 5 functions in HostContentSettingsMap for this functionality
making the interface more complex. It's unclear whether this should even
be in HostContentSettingsMap.
- There is special casing in SetContentSettingCustomScope for some
permissions which shouldn't be there.

This patch a rebase of raymes@'s patch crrev.com/1411653004 with
RegisterPermissionUsage and prefs logic also removed, and code to clean
up the existing saved prefs added.

BUG=691893
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=torne, sergeyu, alokp, nasko, timvolodine

Review-Url: https://codereview.chromium.org/2697473002
Cr-Commit-Position: refs/heads/master@{#451693}
Committed: https://chromium.googlesource.com/chromium/src/+/625ec633bc9592783a2ac98c55e5253189221eed

[Timothy Loh, 2017-02-13 04:24:44]

Description was changed from

==========
Remove last usage functions from HostContentSettingsMap

These functions are:
1) Unused and have been for over 2.5 years. There is no concrete plan
to use them.
2) Not implemented in a very clean way, e.g.:
- There are 5 functions in HostContentSettingsMap for this functionality
making the interface more complex. It's unclear whether this should even
be in HostContentSettingsMap.
- There is special casing in SetContentSettingCustomScope for some
permissions which shouldn't be there.

This patch a rebase of raymes@'s patch crrev.com/1411653004 with the
RegisterPermissionUsage function also removed.

BUG=689799
==========

to

==========
Remove last usage functions from HostContentSettingsMap

These functions are:
1) Unused and have been for over 2.5 years. There is no concrete plan
to use them.
2) Not implemented in a very clean way, e.g.:
- There are 5 functions in HostContentSettingsMap for this functionality
making the interface more complex. It's unclear whether this should even
be in HostContentSettingsMap.
- There is special casing in SetContentSettingCustomScope for some
permissions which shouldn't be there.

This patch a rebase of raymes@'s patch crrev.com/1411653004 with the
RegisterPermissionUsage function also removed.

BUG=689799
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Timothy Loh, 2017-02-13 04:33:10]

timloh@chromium.org changed reviewers:
+ bauerb@chromium.org, raymes@chromium.org

[Timothy Loh, 2017-02-13 04:33:11]

This is a rebase of https://codereview.chromium.org/1411653004 with the
RegisterPermissionUsage function also removed as I understand we no longer plan
to use this either.

[Timothy Loh, 2017-02-13 08:40:36]

On 2017/02/13 04:33:11, Timothy Loh wrote:
> This is a rebase of https://codereview.chromium.org/1411653004 with the
> RegisterPermissionUsage function also removed as I understand we no longer
plan
> to use this either.

Raymes pointed out that I need to also clean up the prefs that were previously
stored, and there is some logic in prefs to support this that needs to be
removed also.

[Bernhard Bauer, 2017-02-13 13:19:58]

This CL looks good so far, but I agree that it would be nice to clean up the
prefs as well.

[Timothy Loh, 2017-02-14 04:06:01]

Description was changed from

==========
Remove last usage functions from HostContentSettingsMap

These functions are:
1) Unused and have been for over 2.5 years. There is no concrete plan
to use them.
2) Not implemented in a very clean way, e.g.:
- There are 5 functions in HostContentSettingsMap for this functionality
making the interface more complex. It's unclear whether this should even
be in HostContentSettingsMap.
- There is special casing in SetContentSettingCustomScope for some
permissions which shouldn't be there.

This patch a rebase of raymes@'s patch crrev.com/1411653004 with the
RegisterPermissionUsage function also removed.

BUG=689799
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove last usage functions from HostContentSettingsMap

These functions are:
1) Unused and have been for over 2.5 years. There is no concrete plan
to use them.
2) Not implemented in a very clean way, e.g.:
- There are 5 functions in HostContentSettingsMap for this functionality
making the interface more complex. It's unclear whether this should even
be in HostContentSettingsMap.
- There is special casing in SetContentSettingCustomScope for some
permissions which shouldn't be there.

This patch a rebase of raymes@'s patch crrev.com/1411653004 with
RegisterPermissionUsage and prefs logic removed, and code to clean up
the existing saved prefs added.

BUG=691893
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Timothy Loh, 2017-02-14 04:21:15]

Description was changed from

==========
Remove last usage functions from HostContentSettingsMap

These functions are:
1) Unused and have been for over 2.5 years. There is no concrete plan
to use them.
2) Not implemented in a very clean way, e.g.:
- There are 5 functions in HostContentSettingsMap for this functionality
making the interface more complex. It's unclear whether this should even
be in HostContentSettingsMap.
- There is special casing in SetContentSettingCustomScope for some
permissions which shouldn't be there.

This patch a rebase of raymes@'s patch crrev.com/1411653004 with
RegisterPermissionUsage and prefs logic removed, and code to clean up
the existing saved prefs added.

BUG=691893
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove last usage functions from HostContentSettingsMap and clean up prefs

The last usage functions are:
1) Unused and have been for over 2.5 years. There is no concrete plan
to use them.
2) Not implemented in a very clean way, e.g.:
- There are 5 functions in HostContentSettingsMap for this functionality
making the interface more complex. It's unclear whether this should even
be in HostContentSettingsMap.
- There is special casing in SetContentSettingCustomScope for some
permissions which shouldn't be there.

This patch a rebase of raymes@'s patch crrev.com/1411653004 with
RegisterPermissionUsage and prefs logic also removed, and code to clean
up the existing saved prefs added.

BUG=691893
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Timothy Loh, 2017-02-14 04:23:16]

In the latest patch set, I removed the logic for setting the last usage prefs
and I added code to clean up the obsolete prefs. I manually verified that it
actually removes entries from the Preferences file.

[Bernhard Bauer, 2017-02-14 13:38:27]

lgtm

https://codereview.chromium.org/2697473002/diff/40001/components/content_sett...
File components/content_settings/core/browser/content_settings_pref_provider.cc
(right):

https://codereview.chromium.org/2697473002/diff/40001/components/content_sett...
components/content_settings/core/browser/content_settings_pref_provider.cc:251:
for (auto key : values_to_clean) {
Use the actual type instead of auto.

[Peter Beverloo, 2017-02-14 13:50:20]

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

[Peter Beverloo, 2017-02-14 13:50:20]

notifications and //content/shell drive-by lgtm

Thanks for the clean up!

[raymes, 2017-02-16 11:01:38]

Thank you so much for this! lg besides a couple of minor comments.

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

https://codereview.chromium.org/2697473002/diff/40001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:184:
"profile.content_settings.exceptions.geolocation";
nit: You should be able to get this from
ContentSettingsRegistry::GetInstance()->Get(type)->website_settings_info()->pref_name();

To be honest it's not really simpler, but it saves redefining the constant.

https://codereview.chromium.org/2697473002/diff/40001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:187:
static const char kObsoleteLastUsed[] = "last_used";
nit: I think static is unneeded here and below

https://codereview.chromium.org/2697473002/diff/40001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:230:
EXPECT_EQ(mic_result_setting, CONTENT_SETTING_ALLOW);
In this part, could we also query the content setting through the public API of
PrefProvider? That would help us to be sure that the mutated values get reparsed
correctly.

[Timothy Loh, 2017-02-17 06:54:42]

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

https://codereview.chromium.org/2697473002/diff/40001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:184:
"profile.content_settings.exceptions.geolocation";
On 2017/02/16 11:01:38, raymes wrote:
> nit: You should be able to get this from
>
ContentSettingsRegistry::GetInstance()->Get(type)->website_settings_info()->pref_name();
> 
> To be honest it's not really simpler, but it saves redefining the constant.

Done.

https://codereview.chromium.org/2697473002/diff/40001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:187:
static const char kObsoleteLastUsed[] = "last_used";
On 2017/02/16 11:01:38, raymes wrote:
> nit: I think static is unneeded here and below

Done.

https://codereview.chromium.org/2697473002/diff/40001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:230:
EXPECT_EQ(mic_result_setting, CONTENT_SETTING_ALLOW);
On 2017/02/16 11:01:38, raymes wrote:
> In this part, could we also query the content setting through the public API
of
> PrefProvider? That would help us to be sure that the mutated values get
reparsed
> correctly.

Done.

https://codereview.chromium.org/2697473002/diff/40001/components/content_sett...
File components/content_settings/core/browser/content_settings_pref_provider.cc
(right):

https://codereview.chromium.org/2697473002/diff/40001/components/content_sett...
components/content_settings/core/browser/content_settings_pref_provider.cc:251:
for (auto key : values_to_clean) {
On 2017/02/14 13:38:27, Bernhard Bauer wrote:
> Use the actual type instead of auto.

Done.

[raymes, 2017-02-19 23:57:54]

lgtm

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

https://codereview.chromium.org/2697473002/diff/60001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:225:
provider.ShutdownOnUIThread();
nit: should we avoid calling this until after we finish using |provider| at the
bottom of this function?

https://codereview.chromium.org/2697473002/diff/60001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:241:
CONTENT_SETTING_ALLOW);
nit: reverse the order of the params so expected is first (actually this applies
to some of the EXPECT_EQ above too).

[Timothy Loh, 2017-02-21 00:14:09]

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

https://codereview.chromium.org/2697473002/diff/60001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:225:
provider.ShutdownOnUIThread();
On 2017/02/19 23:57:54, raymes wrote:
> nit: should we avoid calling this until after we finish using |provider| at
the
> bottom of this function?

Yep, fixed.

https://codereview.chromium.org/2697473002/diff/60001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:241:
CONTENT_SETTING_ALLOW);
On 2017/02/19 23:57:54, raymes wrote:
> nit: reverse the order of the params so expected is first (actually this
applies
> to some of the EXPECT_EQ above too).

Fixed.

[Timothy Loh, 2017-02-21 00:15:58]

Description was changed from

==========
Remove last usage functions from HostContentSettingsMap and clean up prefs

The last usage functions are:
1) Unused and have been for over 2.5 years. There is no concrete plan
to use them.
2) Not implemented in a very clean way, e.g.:
- There are 5 functions in HostContentSettingsMap for this functionality
making the interface more complex. It's unclear whether this should even
be in HostContentSettingsMap.
- There is special casing in SetContentSettingCustomScope for some
permissions which shouldn't be there.

This patch a rebase of raymes@'s patch crrev.com/1411653004 with
RegisterPermissionUsage and prefs logic also removed, and code to clean
up the existing saved prefs added.

BUG=691893
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove last usage functions from HostContentSettingsMap and clean up prefs

The last usage functions are:
1) Unused and have been for over 2.5 years. There is no concrete plan
to use them.
2) Not implemented in a very clean way, e.g.:
- There are 5 functions in HostContentSettingsMap for this functionality
making the interface more complex. It's unclear whether this should even
be in HostContentSettingsMap.
- There is special casing in SetContentSettingCustomScope for some
permissions which shouldn't be there.

This patch a rebase of raymes@'s patch crrev.com/1411653004 with
RegisterPermissionUsage and prefs logic also removed, and code to clean
up the existing saved prefs added.

BUG=691893
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=torne, sergeyu, alokp, nasko, timvolodine
==========

[Timothy Loh, 2017-02-21 00:15:58]

timloh@chromium.org changed reviewers:
+ alokp@chromium.org, nasko@chromium.org, sergeyu@chromium.org,
timvolodine@chromium.org, torne@chromium.org

[Timothy Loh, 2017-02-21 00:16:55]

TBRing a few people:

torne: android_webview
sergeyu: chrome/browser/media/webrtc
alokp: chromecast/browser
nasko: content
timvolodine: device/geolocation

[Timothy Loh, 2017-02-21 00:17:01]

The CQ bit was checked by timloh@chromium.org

[Timothy Loh, 2017-02-21 00:17:02]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
peter@chromium.org, raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/2697473002/#ps80001
(title: "fix up unit test")

[commit-bot: I haz the power, 2017-02-21 00:17:10]

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-02-21 01:27:04]

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

[commit-bot: I haz the power, 2017-02-21 01:27:08]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Timothy Loh, 2017-02-21 02:34:50]

The CQ bit was checked by timloh@chromium.org

[commit-bot: I haz the power, 2017-02-21 02:35:07]

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-02-21 03:33:18]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487644490283430,
"parent_rev": "0c12be20f82f7252da8513c33c89a7a60761d16f", "commit_rev":
"625ec633bc9592783a2ac98c55e5253189221eed"}

[commit-bot: I haz the power, 2017-02-21 03:34:18]

Description was changed from

==========
Remove last usage functions from HostContentSettingsMap and clean up prefs

The last usage functions are:
1) Unused and have been for over 2.5 years. There is no concrete plan
to use them.
2) Not implemented in a very clean way, e.g.:
- There are 5 functions in HostContentSettingsMap for this functionality
making the interface more complex. It's unclear whether this should even
be in HostContentSettingsMap.
- There is special casing in SetContentSettingCustomScope for some
permissions which shouldn't be there.

This patch a rebase of raymes@'s patch crrev.com/1411653004 with
RegisterPermissionUsage and prefs logic also removed, and code to clean
up the existing saved prefs added.

BUG=691893
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=torne, sergeyu, alokp, nasko, timvolodine
==========

to

==========
Remove last usage functions from HostContentSettingsMap and clean up prefs

The last usage functions are:
1) Unused and have been for over 2.5 years. There is no concrete plan
to use them.
2) Not implemented in a very clean way, e.g.:
- There are 5 functions in HostContentSettingsMap for this functionality
making the interface more complex. It's unclear whether this should even
be in HostContentSettingsMap.
- There is special casing in SetContentSettingCustomScope for some
permissions which shouldn't be there.

This patch a rebase of raymes@'s patch crrev.com/1411653004 with
RegisterPermissionUsage and prefs logic also removed, and code to clean
up the existing saved prefs added.

BUG=691893
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=torne, sergeyu, alokp, nasko, timvolodine

Review-Url: https://codereview.chromium.org/2697473002
Cr-Commit-Position: refs/heads/master@{#451693}
Committed:
https://chromium.googlesource.com/chromium/src/+/625ec633bc9592783a2ac98c55e5...
==========

[commit-bot: I haz the power, 2017-02-21 03:34:20]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/625ec633bc9592783a2ac98c55e5...

[Torne, 2017-02-21 11:59:41]

android_webview lgtm

[Torne, 2017-02-21 11:59:41]

android_webview lgtm