Clean up old legacy preference migration logic.

Clean up old legacy preference migration logic.

BUG=372547
Committed: https://crrev.com/fd4f68a33222c654f233256a39b2e55e09caa739
Cr-Commit-Position: refs/heads/master@{#405562}

[proberge, 2016-06-30 20:53:43]

Description was changed from

==========
Clean up old legacy preference migration logic.

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

to

==========
Clean up old legacy preference migration logic.

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

[proberge, 2016-06-30 20:53:43]

proberge@chromium.org changed reviewers:
+ gab@chromium.org, jwd@chromium.org

[proberge, 2016-06-30 20:54:19]

Sorry @gab for sending you another large CL, feel free to re-assign one of them
if you're overloaded.

[proberge, 2016-06-30 21:04:35]

On 2016/06/30 20:54:19, proberge wrote:
> Sorry @gab for sending you another large CL, feel free to re-assign one of
them
> if you're overloaded.

There's some special logic for the default search engine here which mentions
legacy prefs:
https://cs.chromium.org/chromium/src/components/search_engines/default_search...
Should that be cleaned up as well?

[proberge, 2016-06-30 21:04:37]

On 2016/06/30 20:54:19, proberge wrote:
> Sorry @gab for sending you another large CL, feel free to re-assign one of
them
> if you're overloaded.

There's some special logic for the default search engine here which mentions
legacy prefs:
https://cs.chromium.org/chromium/src/components/search_engines/default_search...
Should that be cleaned up as well?

[gab, 2016-07-04 14:56:36]

Top-level comments, review in progress:

Associate this CL with http://crbug.com/372547 and mark as Fixed when it lands
(and might as well take ownership of issue for cleanup phase while you're at
it).

On 2016/06/30 21:04:37, proberge wrote:
> On 2016/06/30 20:54:19, proberge wrote:
> > Sorry @gab for sending you another large CL, feel free to re-assign one of
> them
> > if you're overloaded.
> 
> There's some special logic for the default search engine here which mentions
> legacy prefs:
>
https://cs.chromium.org/chromium/src/components/search_engines/default_search...
> Should that be cleaned up as well?

That's from ~2 years ago so I'd say yes (in another CL), double-check with Cait
to make sure (she implemented that).

[proberge, 2016-07-04 14:57:23]

Description was changed from

==========
Clean up old legacy preference migration logic.

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

to

==========
Clean up old legacy preference migration logic.

BUG=372547
==========

[gab, 2016-07-04 15:21:46]

lgtm w/ one more deletion, thanks :-)!

https://codereview.chromium.org/2111253002/diff/20001/chrome/browser/prefs/pr...
File chrome/browser/prefs/profile_pref_store_manager.cc (left):

https://codereview.chromium.org/2111253002/diff/20001/chrome/browser/prefs/pr...
chrome/browser/prefs/profile_pref_store_manager.cc:205:
pref_hash_store_impl->set_legacy_hash_store_contents(
Also rm set_legacy_hash_store_contents() (was only caller).

https://codereview.chromium.org/2111253002/diff/20001/components/user_prefs/t...
File components/user_prefs/tracked/pref_service_hash_store_contents.h (left):

https://codereview.chromium.org/2111253002/diff/20001/components/user_prefs/t...
components/user_prefs/tracked/pref_service_hash_store_contents.h:23: class
PrefServiceHashStoreContents : public HashStoreContents {
This leaves DictionaryHashStoreContents as the only implementation of
HashStoreContents. Could be worth considering un-generalizing in an eventual
follow-up.

[proberge, 2016-07-04 20:31:42]

https://codereview.chromium.org/2111253002/diff/20001/chrome/browser/prefs/pr...
File chrome/browser/prefs/profile_pref_store_manager.cc (left):

https://codereview.chromium.org/2111253002/diff/20001/chrome/browser/prefs/pr...
chrome/browser/prefs/profile_pref_store_manager.cc:205:
pref_hash_store_impl->set_legacy_hash_store_contents(
On 2016/07/04 15:21:46, gab wrote:
> Also rm set_legacy_hash_store_contents() (was only caller).

Done. I think there's some logic that could be cleaned up in
pref_hash_calculator as well, but I'll leave that to you.

https://codereview.chromium.org/2111253002/diff/20001/components/user_prefs/t...
File components/user_prefs/tracked/pref_service_hash_store_contents.h (left):

https://codereview.chromium.org/2111253002/diff/20001/components/user_prefs/t...
components/user_prefs/tracked/pref_service_hash_store_contents.h:23: class
PrefServiceHashStoreContents : public HashStoreContents {
On 2016/07/04 15:21:46, gab wrote:
> This leaves DictionaryHashStoreContents as the only implementation of
> HashStoreContents. Could be worth considering un-generalizing in an eventual
> follow-up.

Acknowledged.

[gab, 2016-07-04 20:46:45]

https://codereview.chromium.org/2111253002/diff/20001/chrome/browser/prefs/pr...
File chrome/browser/prefs/profile_pref_store_manager.cc (left):

https://codereview.chromium.org/2111253002/diff/20001/chrome/browser/prefs/pr...
chrome/browser/prefs/profile_pref_store_manager.cc:205:
pref_hash_store_impl->set_legacy_hash_store_contents(
On 2016/07/04 20:31:42, proberge wrote:
> On 2016/07/04 15:21:46, gab wrote:
> > Also rm set_legacy_hash_store_contents() (was only caller).
> 
> Done. I think there's some logic that could be cleaned up in
> pref_hash_calculator as well, but I'll leave that to you.

Yes there is, we were going to switch the device ID being used to one that
doesn't depend on RLZ at some point (ask alito/robertshield), device_id_win.cc
actually has all the logic but it isn't hooked up to anything but its own
unittest...

That is still a worthwhile effort IMO and I was hoping cleaning up the old
device ID migration support could be bundled with that.

[proberge, 2016-07-07 13:55:39]

@jwd can you approve for histograms.xml?

[gab, 2016-07-07 15:03:13]

https://codereview.chromium.org/2111253002/diff/60001/components/user_prefs/t...
File components/user_prefs/tracked/pref_service_hash_store_contents.cc (left):

https://codereview.chromium.org/2111253002/diff/60001/components/user_prefs/t...
components/user_prefs/tracked/pref_service_hash_store_contents.cc:86:
std::string PrefServiceHashStoreContents::hash_store_id() const {
Reading some code further in your other CLs I'm realizing that hash_store_id()
was only meant to support a deprecated way to calculate the MACs (it returns ""
for DictionaryHashStoreContents). So it can be removed :-).

[jwd, 2016-07-07 19:51:13]

lgtm

[proberge, 2016-07-07 21:37:18]

https://codereview.chromium.org/2111253002/diff/60001/components/user_prefs/t...
File components/user_prefs/tracked/pref_service_hash_store_contents.cc (left):

https://codereview.chromium.org/2111253002/diff/60001/components/user_prefs/t...
components/user_prefs/tracked/pref_service_hash_store_contents.cc:86:
std::string PrefServiceHashStoreContents::hash_store_id() const {
On 2016/07/07 15:03:13, gab wrote:
> Reading some code further in your other CLs I'm realizing that hash_store_id()
> was only meant to support a deprecated way to calculate the MACs (it returns
""
> for DictionaryHashStoreContents). So it can be removed :-).

Done. PTAL.

[gab, 2016-07-07 23:17:11]

lgtm, thanks

[proberge, 2016-07-08 13:27:18]

The CQ bit was checked by proberge@chromium.org

[proberge, 2016-07-08 13:27:18]

The patchset sent to the CQ was uploaded after l-g-t-m from jwd@chromium.org
Link to the patchset: https://codereview.chromium.org/2111253002/#ps80001
(title: "Remove hash_store_id method with no remaining useful implementation")

[commit-bot: I haz the power, 2016-07-08 13:28:21]

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, 2016-07-08 13:39:05]

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

[commit-bot: I haz the power, 2016-07-08 13:39:06]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[proberge, 2016-07-11 21:05:38]

Removed unused hash store id from a unit test. Note that I changed clients
between patch set 5 and 6, which makes the inter-patch diff look a bit weird.

[gab, 2016-07-12 00:30:58]

On 2016/07/11 21:05:38, proberge wrote:
> Removed unused hash store id from a unit test. Note that I changed clients
> between patch set 5 and 6, which makes the inter-patch diff look a bit weird.

Still lgtm

[proberge, 2016-07-12 17:16:33]

The CQ bit was checked by proberge@chromium.org

[proberge, 2016-07-12 17:16:34]

The patchset sent to the CQ was uploaded after l-g-t-m from jwd@chromium.org
Link to the patchset: https://codereview.chromium.org/2111253002/#ps100001
(title: "Remove now unused kHashStoreId in unit test")

[commit-bot: I haz the power, 2016-07-12 17:17:49]

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, 2016-07-12 18:38:08]

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

[commit-bot: I haz the power, 2016-07-12 18:38:09]

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_...)

[proberge, 2016-07-12 19:46:10]

The CQ bit was checked by proberge@chromium.org

[commit-bot: I haz the power, 2016-07-12 19:47:20]

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, 2016-07-12 22:16:15]

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

[commit-bot: I haz the power, 2016-07-12 22:16:16]

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_...)

[proberge, 2016-07-14 18:00:09]

The CQ bit was checked by proberge@chromium.org

[commit-bot: I haz the power, 2016-07-14 18:01:02]

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, 2016-07-14 20:52:03]

Description was changed from

==========
Clean up old legacy preference migration logic.

BUG=372547
==========

to

==========
Clean up old legacy preference migration logic.

BUG=372547
==========

[commit-bot: I haz the power, 2016-07-14 20:52:04]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-07-14 20:52:08]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-14 20:55:40]

Description was changed from

==========
Clean up old legacy preference migration logic.

BUG=372547
==========

to

==========
Clean up old legacy preference migration logic.

BUG=372547

Committed: https://crrev.com/fd4f68a33222c654f233256a39b2e55e09caa739
Cr-Commit-Position: refs/heads/master@{#405562}
==========

[commit-bot: I haz the power, 2016-07-14 20:55:42]

Patchset 6 (id:??) landed as
https://crrev.com/fd4f68a33222c654f233256a39b2e55e09caa739
Cr-Commit-Position: refs/heads/master@{#405562}