Remove old default search preferences.

Remove old default search preferences.

After https://bugs.chromium.org/p/chromium/issues/detail?id=365762
chromium code support two sets of preferences for default search -
obsolete and new. Old preferences are migrated to new prefs on start
since M36. Its time to delete old prefs set and migration logic.

BUG=676801
R=pkasting@chromium.org,gab@chromium.org

Review-Url: https://codereview.chromium.org/2598033004
Cr-Commit-Position: refs/heads/master@{#442569}
Committed: https://chromium.googlesource.com/chromium/src/+/1be376e34def3865b168d419e415ee4b6df63f6e

[Alexander Yashkin, 2016-12-23 12:03:23]

@gab - PTAL at prefs changes
@pkasting - PTAL at search_engines changes

https://codereview.chromium.org/2598033004/diff/20001/components/user_prefs/t...
File components/user_prefs/tracked/pref_hash_filter.cc (right):

https://codereview.chromium.org/2598033004/diff/20001/components/user_prefs/t...
components/user_prefs/tracked/pref_hash_filter.cc:44:
"default_search_provider.search_url",
@gab
I have added deprecated tracked prefs here to cleanup, yet I am not sure if its
needed.
This function removes deprecated prefs by calling:

 pref_store_contents->Remove(key, NULL);
 hash_store_transaction->ClearHash(key);

In migration logic old preferences were always cleared from prefs after
migrating to new set.
Can I simply remove them without cleanup?

[gab, 2016-12-23 17:26:26]

https://codereview.chromium.org/2598033004/diff/20001/chrome/browser/prefs/ch...
File chrome/browser/prefs/chrome_pref_service_factory.cc (right):

https://codereview.chromium.org/2598033004/diff/20001/chrome/browser/prefs/ch...
chrome/browser/prefs/chrome_pref_service_factory.cc:151: 8,
prefs::kDefaultSearchProviderSearchURL,
If they're no longer migrated, why do they still need to be protected? Feels we
should outright delete kDefaultSearchProviderSearchURL (we usually have a
cleanup phase in browser_prefs.cc::MigrateUserPrefs() for a year but it's
unecessary here given the preceding migration having the same effect -- i.e.
ClearDefaultSearchProviderFromLegacyPrefs()).

https://codereview.chromium.org/2598033004/diff/20001/components/user_prefs/t...
File components/user_prefs/tracked/pref_hash_filter.cc (right):

https://codereview.chromium.org/2598033004/diff/20001/components/user_prefs/t...
components/user_prefs/tracked/pref_hash_filter.cc:42:
"sync.remaining_rollback_tries",
Remove the above entries (since people tend to forget we do a rolling cleanup
where the next person cleans the previous person's entries :)).

https://codereview.chromium.org/2598033004/diff/20001/components/user_prefs/t...
components/user_prefs/tracked/pref_hash_filter.cc:43: // TODO(a-v-y): Remove
after M59.
"Remove in M60+." (i.e. we like to leave the cleanup for a good 3+ milestones)

https://codereview.chromium.org/2598033004/diff/20001/components/user_prefs/t...
components/user_prefs/tracked/pref_hash_filter.cc:44:
"default_search_provider.search_url",
On 2016/12/23 12:03:23, Alexander Yashkin wrote:
> @gab
> I have added deprecated tracked prefs here to cleanup, yet I am not sure if
its
> needed.
> This function removes deprecated prefs by calling:
> 
>  pref_store_contents->Remove(key, NULL);
>  hash_store_transaction->ClearHash(key);
> 
> In migration logic old preferences were always cleared from prefs after
> migrating to new set.
> Can I simply remove them without cleanup?

You can remove the prefs' constants themselves (and their entry in
kTrackedPrefs). But you will still need this to cleanup up the MAC associated
with an (hopefully by now) null pref.

[Alexander Yashkin, 2016-12-23 20:28:14]

https://codereview.chromium.org/2598033004/diff/20001/chrome/browser/prefs/ch...
File chrome/browser/prefs/chrome_pref_service_factory.cc (right):

https://codereview.chromium.org/2598033004/diff/20001/chrome/browser/prefs/ch...
chrome/browser/prefs/chrome_pref_service_factory.cc:151: 8,
prefs::kDefaultSearchProviderSearchURL,
On 2016/12/23 at 17:26:26, gab (HOHOHO) wrote:
> If they're no longer migrated, why do they still need to be protected? Feels
we should outright delete kDefaultSearchProviderSearchURL (we usually have a
cleanup phase in browser_prefs.cc::MigrateUserPrefs() for a year but it's
unecessary here given the preceding migration having the same effect -- i.e.
ClearDefaultSearchProviderFromLegacyPrefs()).

Done

https://codereview.chromium.org/2598033004/diff/20001/components/user_prefs/t...
File components/user_prefs/tracked/pref_hash_filter.cc (right):

https://codereview.chromium.org/2598033004/diff/20001/components/user_prefs/t...
components/user_prefs/tracked/pref_hash_filter.cc:42:
"sync.remaining_rollback_tries",
On 2016/12/23 at 17:26:26, gab (HOHOHO) wrote:
> Remove the above entries (since people tend to forget we do a rolling cleanup
where the next person cleans the previous person's entries :)).

Done

https://codereview.chromium.org/2598033004/diff/20001/components/user_prefs/t...
components/user_prefs/tracked/pref_hash_filter.cc:43: // TODO(a-v-y): Remove
after M59.
On 2016/12/23 at 17:26:26, gab (HOHOHO) wrote:
> "Remove in M60+." (i.e. we like to leave the cleanup for a good 3+ milestones)

Done

https://codereview.chromium.org/2598033004/diff/20001/components/user_prefs/t...
components/user_prefs/tracked/pref_hash_filter.cc:44:
"default_search_provider.search_url",
On 2016/12/23 at 17:26:26, gab (HOHOHO) wrote:
> On 2016/12/23 12:03:23, Alexander Yashkin wrote:
> > @gab
> > I have added deprecated tracked prefs here to cleanup, yet I am not sure if
its
> > needed.
> > This function removes deprecated prefs by calling:
> > 
> >  pref_store_contents->Remove(key, NULL);
> >  hash_store_transaction->ClearHash(key);
> > 
> > In migration logic old preferences were always cleared from prefs after
> > migrating to new set.
> > Can I simply remove them without cleanup?
> 
> You can remove the prefs' constants themselves (and their entry in
kTrackedPrefs). But you will still need this to cleanup up the MAC associated
with an (hopefully by now) null pref.

Done

[gab, 2017-01-05 19:35:27]

lgtm, beware that this may break many people using the old prefs in
master_preferences (i.e. it's been deprecated for a while but the migration
still made old master_preferences "work"). Worth searching on Google to find
documentation that still refers to the old pref names.

CC georgesak/blumberg as FYI for enterprise and admins that may hit this

https://codereview.chromium.org/2598033004/diff/60001/components/user_prefs/t...
File components/user_prefs/tracked/pref_hash_filter.cc (right):

https://codereview.chromium.org/2598033004/diff/60001/components/user_prefs/t...
components/user_prefs/tracked/pref_hash_filter.cc:40:
"default_search_provider.keyword"
nit: trailing comma as was the case previously

[Peter Kasting, 2017-01-06 00:09:56]

https://codereview.chromium.org/2598033004/diff/60001/components/search_engin...
File components/search_engines/default_search_policy_handler.cc (right):

https://codereview.chromium.org/2598033004/diff/60001/components/search_engin...
components/search_engines/default_search_policy_handler.cc:14: #include
"base/strings/string_util.h"
Can this #include or any others now be removed?

https://codereview.chromium.org/2598033004/diff/60001/components/search_engin...
components/search_engines/default_search_policy_handler.cc:62: const
PolicyToPreferenceMapEntry kDefaultSearchPolicyMap[] = {
Will we have more entries in this map in the future?  If we don't know the
answer is yes, should we simplify this and the code that uses it within the
class by hardcoding the handling of this more?

https://codereview.chromium.org/2598033004/diff/60001/components/search_engin...
components/search_engines/default_search_policy_handler.cc:129: if (policy_name
!= key::kDefaultSearchProviderEnabled &&
Can this ever succeed anymore?

[Alexander Yashkin, 2017-01-09 06:11:38]

@gab, I searched for master_preferences documentation and found only 
https://www.chromium.org/administrators/configuring-other-preferences
and default search options are not mentioned there.

https://codereview.chromium.org/2598033004/diff/60001/components/search_engin...
File components/search_engines/default_search_policy_handler.cc (right):

https://codereview.chromium.org/2598033004/diff/60001/components/search_engin...
components/search_engines/default_search_policy_handler.cc:14: #include
"base/strings/string_util.h"
On 2017/01/06 00:09:56, Peter Kasting (backlogged) wrote:
> Can this #include or any others now be removed?

Removed string_util.h.

https://codereview.chromium.org/2598033004/diff/60001/components/search_engin...
components/search_engines/default_search_policy_handler.cc:62: const
PolicyToPreferenceMapEntry kDefaultSearchPolicyMap[] = {
On 2017/01/06 00:09:56, Peter Kasting (backlogged) wrote:
> Will we have more entries in this map in the future?  If we don't know the
> answer is yes, should we simplify this and the code that uses it within the
> class by hardcoding the handling of this more?

Removed kDefaultSearchPolicyMap and handlers_ from this class, because they were
used only to check value types for policies. 
Added  kDefaultSearchProviderEnabled to kDefaultSearchPolicyDataMap.

https://codereview.chromium.org/2598033004/diff/60001/components/search_engin...
components/search_engines/default_search_policy_handler.cc:129: if (policy_name
!= key::kDefaultSearchProviderEnabled &&
On 2017/01/06 00:09:56, Peter Kasting (backlogged) wrote:
> Can this ever succeed anymore?

I think after removing entries from kDefaultSearchPolicyMap I broke check of
policies value types.
I tried to fix it, added unittest.

https://codereview.chromium.org/2598033004/diff/60001/components/user_prefs/t...
File components/user_prefs/tracked/pref_hash_filter.cc (right):

https://codereview.chromium.org/2598033004/diff/60001/components/user_prefs/t...
components/user_prefs/tracked/pref_hash_filter.cc:40:
"default_search_provider.keyword"
On 2017/01/05 at 19:35:27, gab wrote:
> nit: trailing comma as was the case previously

Done

[Peter Kasting, 2017-01-09 22:00:06]

LGTM

git cl format can help prevent errors like the indentation issues.

https://codereview.chromium.org/2598033004/diff/80001/components/search_engin...
File components/search_engines/default_search_policy_handler.cc (right):

https://codereview.chromium.org/2598033004/diff/80001/components/search_engin...
components/search_engines/default_search_policy_handler.cc:154: // above.
Nit: Shorter and avoids restating the code: "kDefaultSearchProviderEnabled has
already been handled."

https://codereview.chromium.org/2598033004/diff/80001/components/search_engin...
components/search_engines/default_search_policy_handler.cc:155: if (policy_name
== key::kDefaultSearchProviderEnabled) continue;
Nit: Put "continue" on a separate line (Google style guide allows your style,
but Chromium does not generally use it)

https://codereview.chromium.org/2598033004/diff/80001/components/search_engin...
components/search_engines/default_search_policy_handler.cc:216: auto policy_name
= policy_map_entry.policy_name;
Nit: Can just inline into the one use below

https://codereview.chromium.org/2598033004/diff/80001/components/search_engin...
components/search_engines/default_search_policy_handler.cc:218: // Check value
type for every policy.
Nit: Not sure what this comment buys over the one above

https://codereview.chromium.org/2598033004/diff/80001/components/search_engin...
components/search_engines/default_search_policy_handler.cc:222:
base::Value::GetTypeName(value_type));
Nit: Bad indentation

https://codereview.chromium.org/2598033004/diff/80001/components/search_engin...
components/search_engines/default_search_policy_handler.cc:223: all_ok &= false;
Nit: Just use =

https://codereview.chromium.org/2598033004/diff/80001/components/search_engin...
File components/search_engines/default_search_policy_handler.h (right):

https://codereview.chromium.org/2598033004/diff/80001/components/search_engin...
components/search_engines/default_search_policy_handler.h:29: // Calls
|CheckPolicySettings()| on each of the handlers in |handlers_|
Nit: Comment needs updating

https://codereview.chromium.org/2598033004/diff/80001/components/search_engin...
File components/search_engines/default_search_policy_handler_unittest.cc
(right):

https://codereview.chromium.org/2598033004/diff/80001/components/search_engin...
components/search_engines/default_search_policy_handler_unittest.cc:183:
POLICY_SCOPE_USER, POLICY_SOURCE_CLOUD,
Nit: Indentation

https://codereview.chromium.org/2598033004/diff/80001/components/search_engin...
components/search_engines/default_search_policy_handler_unittest.cc:192:
POLICY_SCOPE_USER, POLICY_SOURCE_CLOUD,
Nit: Indentation

[Alexander Yashkin, 2017-01-10 05:01:44]

https://codereview.chromium.org/2598033004/diff/80001/components/search_engin...
File components/search_engines/default_search_policy_handler.cc (right):

https://codereview.chromium.org/2598033004/diff/80001/components/search_engin...
components/search_engines/default_search_policy_handler.cc:154: // above.
On 2017/01/09 22:00:05, Peter Kasting wrote:
> Nit: Shorter and avoids restating the code: "kDefaultSearchProviderEnabled has
> already been handled."

Done.

https://codereview.chromium.org/2598033004/diff/80001/components/search_engin...
components/search_engines/default_search_policy_handler.cc:155: if (policy_name
== key::kDefaultSearchProviderEnabled) continue;
On 2017/01/09 22:00:05, Peter Kasting wrote:
> Nit: Put "continue" on a separate line (Google style guide allows your style,
> but Chromium does not generally use it)

Done.

https://codereview.chromium.org/2598033004/diff/80001/components/search_engin...
components/search_engines/default_search_policy_handler.cc:216: auto policy_name
= policy_map_entry.policy_name;
On 2017/01/09 22:00:06, Peter Kasting wrote:
> Nit: Can just inline into the one use below

Done.

https://codereview.chromium.org/2598033004/diff/80001/components/search_engin...
components/search_engines/default_search_policy_handler.cc:218: // Check value
type for every policy.
On 2017/01/09 22:00:05, Peter Kasting wrote:
> Nit: Not sure what this comment buys over the one above

Removed

https://codereview.chromium.org/2598033004/diff/80001/components/search_engin...
components/search_engines/default_search_policy_handler.cc:222:
base::Value::GetTypeName(value_type));
On 2017/01/09 22:00:05, Peter Kasting wrote:
> Nit: Bad indentation

Fixed

https://codereview.chromium.org/2598033004/diff/80001/components/search_engin...
components/search_engines/default_search_policy_handler.cc:223: all_ok &= false;
On 2017/01/09 22:00:05, Peter Kasting wrote:
> Nit: Just use =

Done.

https://codereview.chromium.org/2598033004/diff/80001/components/search_engin...
File components/search_engines/default_search_policy_handler.h (right):

https://codereview.chromium.org/2598033004/diff/80001/components/search_engin...
components/search_engines/default_search_policy_handler.h:29: // Calls
|CheckPolicySettings()| on each of the handlers in |handlers_|
On 2017/01/09 22:00:06, Peter Kasting wrote:
> Nit: Comment needs updating

Done.

https://codereview.chromium.org/2598033004/diff/80001/components/search_engin...
File components/search_engines/default_search_policy_handler_unittest.cc
(right):

https://codereview.chromium.org/2598033004/diff/80001/components/search_engin...
components/search_engines/default_search_policy_handler_unittest.cc:183:
POLICY_SCOPE_USER, POLICY_SOURCE_CLOUD,
On 2017/01/09 22:00:06, Peter Kasting wrote:
> Nit: Indentation

Done.

https://codereview.chromium.org/2598033004/diff/80001/components/search_engin...
components/search_engines/default_search_policy_handler_unittest.cc:192:
POLICY_SCOPE_USER, POLICY_SOURCE_CLOUD,
On 2017/01/09 22:00:06, Peter Kasting wrote:
> Nit: Indentation

Done.

[Alexander Yashkin, 2017-01-10 05:45:28]

On 2017/01/09 at 22:00:06, pkasting wrote:
> LGTM
> 
> git cl format can help prevent errors like the indentation issues.
Always forget to apply git cl format, applied.

[Alexander Yashkin, 2017-01-10 05:48:00]

The CQ bit was checked by a-v-y@yandex-team.ru

[Alexander Yashkin, 2017-01-10 05:48:01]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2598033004/#ps100001
(title: "Fixed after review, round 2")

[commit-bot: I haz the power, 2017-01-10 05:48:56]

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-01-10 06:02:24]

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

[commit-bot: I haz the power, 2017-01-10 06:02:25]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Alexander Yashkin, 2017-01-10 06:15:03]

a-v-y@yandex-team.ru changed reviewers:
+ jochen@chromium.org, sdefresne@chromium.org

[Alexander Yashkin, 2017-01-10 06:17:45]

On 2017/01/10 at 06:02:25, commit-bot wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

@sdefresne, PTAL at ios prefs changes
@jochen, PTAL at histograms.xml

[sdefresne, 2017-01-10 09:35:10]

ios/ lgtm

[jochen (gone - plz use gerrit), 2017-01-10 09:45:31]

i can only approve UseCounter additions. Please ask somebody from
tools/metrics/OWNERS for a review

[Alexander Yashkin, 2017-01-10 10:24:42]

On 2017/01/10 at 09:45:31, jochen wrote:
> i can only approve UseCounter additions. Please ask somebody from
tools/metrics/OWNERS for a review

@mpearson@chromium.org, PTAL at histograms.xml

[Alexander Yashkin, 2017-01-10 11:29:33]

a-v-y@yandex-team.ru changed reviewers:
+ mpearson@chromium.org
- jochen@chromium.org

[Alexander Yashkin, 2017-01-10 12:08:30]

The CQ bit was checked by a-v-y@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 12:08:43]

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-01-10 13:10:22]

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

[commit-bot: I haz the power, 2017-01-10 13:10:23]

Dry run: This issue passed the CQ dry run.

[Alexander Yashkin, 2017-01-10 13:18:20]

The CQ bit was checked by a-v-y@yandex-team.ru

[commit-bot: I haz the power, 2017-01-10 13:18:37]

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-01-10 13:22:16]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484054300438190,
"parent_rev": "6b2ff207c6fdf78d401bd13b417a31040a44dc61", "commit_rev":
"1be376e34def3865b168d419e415ee4b6df63f6e"}

[commit-bot: I haz the power, 2017-01-10 13:22:44]

Description was changed from

==========
Remove old default search preferences.

After https://bugs.chromium.org/p/chromium/issues/detail?id=365762
chromium code support two sets of preferences for default search -
obsolete and new. Old preferences are migrated to new prefs on start
since M36. Its time to delete old prefs set and migration logic.

BUG=676801

R=pkasting@chromium.org,gab@chromium.org
==========

to

==========
Remove old default search preferences.

After https://bugs.chromium.org/p/chromium/issues/detail?id=365762
chromium code support two sets of preferences for default search -
obsolete and new. Old preferences are migrated to new prefs on start
since M36. Its time to delete old prefs set and migration logic.

BUG=676801

R=pkasting@chromium.org,gab@chromium.org

Review-Url: https://codereview.chromium.org/2598033004
Cr-Commit-Position: refs/heads/master@{#442569}
Committed:
https://chromium.googlesource.com/chromium/src/+/1be376e34def3865b168d419e415...
==========

[commit-bot: I haz the power, 2017-01-10 13:22:46]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/1be376e34def3865b168d419e415...

[Mark P, 2017-01-10 19:57:41]

histograms.xml lgtm