[Extensions] Remove unused parts of the preferencesPrivate API

[Extensions] Remove unused parts of the preferencesPrivate API

easyUnlockProximityRequired might still be used by someone, but it looks like
the rest of this API can be removed.

BUG=593166
TBR=erg@chromium.org (profiles/ pref change reviewed by gab@)

Committed: https://crrev.com/76ea109e501ad804a4539f61028515d7ff9b7e07
Cr-Commit-Position: refs/heads/master@{#381278}

[Devlin, 2016-03-09 22:00:51]

rdevlin.cronin@chromium.org changed reviewers:
+ asargent@chromium.org

[asargent_no_longer_on_chrome, 2016-03-09 23:07:44]

https://codereview.chromium.org/1776373002/diff/1/chrome/common/extensions/ap...
File chrome/common/extensions/api/preferences_private.json (left):

https://codereview.chromium.org/1776373002/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/preferences_private.json:19: "value":
["googlegeolocationaccess.enabled", {"type":"boolean"}],
I think you also want to remove "googlegeolocationaccess.enabled" from
PreferenceWhitelist in
chrome/browser/extensions/api/preference/chrome_direct_setting_api.cc

(I happened to run across this file because I was trying to understand how the
heck these property read/writes work, and finally went back to the CL adding
easyUnlockProximityRequired and noticed it being added there).

[Devlin, 2016-03-10 19:08:41]

Note also that I removed the histogram updates from this CL since it emits a
presubmit warning (since it's "editing" an existing entry), and will probably
have to be notry'd.  I'll do that in a followup CL with no other code changes to
minimize the risk.  (There's a bug about fixing the histogram checker around
somewhere, but I've never quite had time to get around to it...)

https://codereview.chromium.org/1776373002/diff/1/chrome/common/extensions/ap...
File chrome/common/extensions/api/preferences_private.json (left):

https://codereview.chromium.org/1776373002/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/preferences_private.json:19: "value":
["googlegeolocationaccess.enabled", {"type":"boolean"}],
On 2016/03/09 23:07:44, Antony Sargent wrote:
> I think you also want to remove "googlegeolocationaccess.enabled" from
> PreferenceWhitelist in
> chrome/browser/extensions/api/preference/chrome_direct_setting_api.cc
> 
> (I happened to run across this file because I was trying to understand how the
> heck these property read/writes work, and finally went back to the CL adding
> easyUnlockProximityRequired and noticed it being added there). 

Good call!  It also looks like that was the only time we used that pref, so we
can remove it entirely. Do you recall if we have to push out a pref deletion
before just removing the entry here or if the service knows to delete any
unknown prefs?

[asargent_no_longer_on_chrome, 2016-03-10 22:35:28]

> Good call!  It also looks like that was the only time we used that pref, so we
> can remove it entirely. Do you recall if we have to push out a pref deletion
> before just removing the entry here or if the service knows to delete any
> unknown prefs?

I don't know off the top of my head. Want me to hold off looking at the latest
patchset until you can incorporate that (or determine it's not needed)?

[Devlin, 2016-03-10 22:47:54]

On 2016/03/10 22:35:28, Antony Sargent wrote:
> > Good call!  It also looks like that was the only time we used that pref, so
we
> > can remove it entirely. Do you recall if we have to push out a pref deletion
> > before just removing the entry here or if the service knows to delete any
> > unknown prefs?
> 
> I don't know off the top of my head. Want me to hold off looking at the latest
> patchset until you can incorporate that (or determine it's not needed)?

I'll have a prefs owner look it over and enlighten us.  I think it's probably
safe for you to review, since anything else will be pref changes (rather than
extension changes).

[Devlin, 2016-03-10 22:49:33]

rdevlin.cronin@chromium.org changed reviewers:
+ gab@chromium.org

[Devlin, 2016-03-10 22:49:34]

+gab@ (a lucky owner of components/prefs) for a prefs question:
If we wanna remove a chrome pref (i.e., one registered in the PrefRegistry), is
it sufficient to just remove the pref entry entirely from the code, or do we
have first submit a patch to clear it for all users and then remove it after
that?

[asargent_no_longer_on_chrome, 2016-03-11 16:56:33]

lgtm

[Devlin, 2016-03-14 17:39:50]

gab@, friendly ping?

[gab, 2016-03-14 18:32:27]

On 2016/03/10 22:49:34, Devlin wrote:
> +gab@ (a lucky owner of components/prefs) for a prefs question:
> If we wanna remove a chrome pref (i.e., one registered in the PrefRegistry),
is
> it sufficient to just remove the pref entry entirely from the code, or do we
> have first submit a patch to clear it for all users and then remove it after
> that?

Add clean up code in this CL under MigrateObsoleteBrowserPrefs() @
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pre...

The flow is for devs to add clean up for their pref in there with a date comment
of when it landed and to also cleanup anything that's more than a year old
behind them at the same time, keeping a sane rotation of cleanup without having
to ping people a year later.

PS: Sorry for delay was OOO Thu/Fri and forgot to mark it in codereview..

[Devlin, 2016-03-14 20:52:32]

On 2016/03/14 18:32:27, gab wrote:
> On 2016/03/10 22:49:34, Devlin wrote:
> > +gab@ (a lucky owner of components/prefs) for a prefs question:
> > If we wanna remove a chrome pref (i.e., one registered in the PrefRegistry),
> is
> > it sufficient to just remove the pref entry entirely from the code, or do we
> > have first submit a patch to clear it for all users and then remove it after
> > that?
> 
> Add clean up code in this CL under MigrateObsoleteBrowserPrefs() @
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pre...
> 
> The flow is for devs to add clean up for their pref in there with a date
comment
> of when it landed and to also cleanup anything that's more than a year old
> behind them at the same time, keeping a sane rotation of cleanup without
having
> to ping people a year later.
> 
> PS: Sorry for delay was OOO Thu/Fri and forgot to mark it in codereview..

Thanks!  Mind taking a look at the latest patch set?

[gab, 2016-03-14 20:57:36]

lgtm w/ comment

https://codereview.chromium.org/1776373002/diff/40001/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/1776373002/diff/40001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:675:
profile_prefs->ClearPref(kGoogleGeolocationAccessEnabled);
From its location in pref_names.cc I gather that this was a Local State pref? If
so it needs to be cleared in MigrateObsoleteBrowserPrefs() above, not in
MigrateObsoleteProfilePrefs() which is for obsolete per-profile Preferences.

[Devlin, 2016-03-14 21:14:32]

Patchset #4 (id:60001) has been deleted

[Devlin, 2016-03-14 21:19:52]

https://codereview.chromium.org/1776373002/diff/40001/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/1776373002/diff/40001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:675:
profile_prefs->ClearPref(kGoogleGeolocationAccessEnabled);
On 2016/03/14 20:57:36, gab wrote:
> From its location in pref_names.cc I gather that this was a Local State pref?
If
> so it needs to be cleared in MigrateObsoleteBrowserPrefs() above, not in
> MigrateObsoleteProfilePrefs() which is for obsolete per-profile Preferences.

Detangling this pref is kind of a pain, but I think it was misplaced in the
pref_names file.  The pref is read at [1] and [2] and is set at [3] and [4], all
of which use the pref service from Profile::GetPrefs().  Does that sound right
to you?

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
[3]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
[4]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

[gab, 2016-03-15 01:20:29]

prefs lgtm++, thanks

https://codereview.chromium.org/1776373002/diff/40001/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/1776373002/diff/40001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:675:
profile_prefs->ClearPref(kGoogleGeolocationAccessEnabled);
On 2016/03/14 21:19:52, Devlin wrote:
> On 2016/03/14 20:57:36, gab wrote:
> > From its location in pref_names.cc I gather that this was a Local State
pref?
> If
> > so it needs to be cleared in MigrateObsoleteBrowserPrefs() above, not in
> > MigrateObsoleteProfilePrefs() which is for obsolete per-profile Preferences.
> 
> Detangling this pref is kind of a pain, but I think it was misplaced in the
> pref_names file.  The pref is read at [1] and [2] and is set at [3] and [4],
all
> of which use the pref service from Profile::GetPrefs().  Does that sound right
> to you?
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
> [2]
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
> [3]
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
> [4]
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

Ah ok, that sounds right then, had only based my reading on its position in the
file (TBH that file is a humongous mess and trusting it was incorrect of me in
the first place!)

[Devlin, 2016-03-15 17:10:58]

Description was changed from

==========
[Extensions] Remove unused parts of the preferencesPrivate API

easyUnlockProximityRequired might still be used by someone, but it looks like
the rest of this API can be removed.

BUG=593166
==========

to

==========
[Extensions] Remove unused parts of the preferencesPrivate API

easyUnlockProximityRequired might still be used by someone, but it looks like
the rest of this API can be removed.

BUG=593166
TBR=erg@chromium.org (profiles/ pref change reviewed by gab@)
==========

[Devlin, 2016-03-15 17:11:11]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2016-03-15 17:11:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
asargent@chromium.org
Link to the patchset: https://codereview.chromium.org/1776373002/#ps80001
(title: "Really clean up prefs")

[commit-bot: I haz the power, 2016-03-15 17:11:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1776373002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1776373002/80001

[commit-bot: I haz the power, 2016-03-15 18:03:53]

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

[commit-bot: I haz the power, 2016-03-15 18:03:54]

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

[Devlin, 2016-03-15 18:58:05]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2016-03-15 18:58:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1776373002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1776373002/80001

[commit-bot: I haz the power, 2016-03-15 19:20:14]

Description was changed from

==========
[Extensions] Remove unused parts of the preferencesPrivate API

easyUnlockProximityRequired might still be used by someone, but it looks like
the rest of this API can be removed.

BUG=593166
TBR=erg@chromium.org (profiles/ pref change reviewed by gab@)
==========

to

==========
[Extensions] Remove unused parts of the preferencesPrivate API

easyUnlockProximityRequired might still be used by someone, but it looks like
the rest of this API can be removed.

BUG=593166
TBR=erg@chromium.org (profiles/ pref change reviewed by gab@)
==========

[commit-bot: I haz the power, 2016-03-15 19:20:15]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-03-15 19:21:27]

Description was changed from

==========
[Extensions] Remove unused parts of the preferencesPrivate API

easyUnlockProximityRequired might still be used by someone, but it looks like
the rest of this API can be removed.

BUG=593166
TBR=erg@chromium.org (profiles/ pref change reviewed by gab@)
==========

to

==========
[Extensions] Remove unused parts of the preferencesPrivate API

easyUnlockProximityRequired might still be used by someone, but it looks like
the rest of this API can be removed.

BUG=593166
TBR=erg@chromium.org (profiles/ pref change reviewed by gab@)

Committed: https://crrev.com/76ea109e501ad804a4539f61028515d7ff9b7e07
Cr-Commit-Position: refs/heads/master@{#381278}
==========

[commit-bot: I haz the power, 2016-03-15 19:21:29]

Patchset 4 (id:??) landed as
https://crrev.com/76ea109e501ad804a4539f61028515d7ff9b7e07
Cr-Commit-Position: refs/heads/master@{#381278}