Remove kDistroDict from Preferences.

Remove kDistroDict from Preferences.

Extracted from https://codereview.chromium.org/2705113005.

kDistroDict was mapped along with other master_preferences into Preferences
but was never registered to be read at runtime... except for the RLZ
ping delay that is... which is now mapped to a real pref to keep things cleaner.

Also fixes a long standing mistake to send the ping delay as milliseconds instead of seconds..!
https://codereview.chromium.org/2714853002/diff/160001/chrome/browser/chrome_browser_main.cc#newcode1752

BUG=555550
Review-Url: https://codereview.chromium.org/2714853002
Cr-Commit-Position: refs/heads/master@{#454115}
Committed: https://chromium.googlesource.com/chromium/src/+/3ca4a4913a34992aaa78c25a6e304aef8fca340b

[gab, 2017-02-23 22:59:55]

Description was changed from

==========
Extracted from https://codereview.chromium.org/2705113005.

kDistroDict was mapped along with other master_preferences into Preferences
but was never registered to be read at runtime... except for the RLZ
ping delay that is... which is now mapped to a real pref to keep things cleaner.
Remove kDistroDict from Preferences.


BUG=555550
==========

to

==========
Remove kDistroDict from Preferences.

Extracted from https://codereview.chromium.org/2705113005.

kDistroDict was mapped along with other master_preferences into Preferences
but was never registered to be read at runtime... except for the RLZ
ping delay that is... which is now mapped to a real pref to keep things cleaner.

BUG=555550
==========

[gab, 2017-02-23 23:10:26]

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

[commit-bot: I haz the power, 2017-02-23 23:11:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2017-02-23 23:12:37]

Patchset #3 (id:40001) has been deleted

[gab, 2017-02-23 23:12:51]

Patchset #2 (id:20001) has been deleted

[gab, 2017-02-23 23:12:56]

Patchset #1 (id:1) has been deleted

[gab, 2017-02-23 23:23:36]

Patchset #1 (id:60001) has been deleted

[gab, 2017-02-23 23:26:44]

gab@chromium.org changed reviewers:
+ grt@chromium.org

[gab, 2017-02-23 23:26:45]

Greg PTaL, this can land on its own, thanks!

[gab, 2017-02-23 23:28:15]

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

[commit-bot: I haz the power, 2017-02-23 23:28:59]

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-02-23 23:40:07]

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

[commit-bot: I haz the power, 2017-02-23 23:40:08]

Dry run: 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_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[gab, 2017-02-23 23:48:45]

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

[commit-bot: I haz the power, 2017-02-23 23:49:44]

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-02-24 00:03:34]

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

[commit-bot: I haz the power, 2017-02-24 00:03:35]

Dry run: 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_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[gab, 2017-02-24 00:34:55]

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

[gab, 2017-02-24 00:35:10]

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

[commit-bot: I haz the power, 2017-02-24 00:35:23]

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-02-24 00:40:01]

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

[commit-bot: I haz the power, 2017-02-24 00:40:02]

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

[gab, 2017-02-24 01:12:12]

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

[commit-bot: I haz the power, 2017-02-24 01:12:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2017-02-24 01:15:22]

The CQ bit was unchecked by gab@chromium.org

[gab, 2017-02-24 01:15:49]

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

[commit-bot: I haz the power, 2017-02-24 01:16:24]

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-02-24 02:55:04]

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

[commit-bot: I haz the power, 2017-02-24 02:55:05]

Dry run: This issue passed the CQ dry run.

[grt (UTC plus 2), 2017-02-24 07:42:19]

grt@chromium.org changed reviewers:
+ rogerta@chromium.org

[grt (UTC plus 2), 2017-02-24 07:42:21]

a nice cleanup.

+rogerta for 90 vs 90000 question. i didn't look at the history to see if ms is
an accident from a refactor or something.

https://codereview.chromium.org/2714853002/diff/160001/chrome/browser/first_r...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/2714853002/diff/160001/chrome/browser/first_r...
chrome/browser/first_run/first_run.cc:662:
registry->RegisterIntegerPref(prefs::kRlzPingDelay, 90);
this doesn't feel like the right place. rlz isn't specific to first_run. could
you look for a more general place to register the pref? also, i wonder if
local_state isn't a better place for it. maybe outside of scope of this cl, but
how about a comment/todo for later?

https://codereview.chromium.org/2714853002/diff/160001/chrome/browser/first_r...
chrome/browser/first_run/first_run.cc:662:
registry->RegisterIntegerPref(prefs::kRlzPingDelay, 90);
while 90 has been the default, should it really be 90000 (see other comment
about seconds vs milliseconds)? +rogerga to confirm.

https://codereview.chromium.org/2714853002/diff/160001/chrome/browser/prefs/b...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/2714853002/diff/160001/chrome/browser/prefs/b...
chrome/browser/prefs/browser_prefs.cc:753: #if BUILDFLAG(ENABLE_RLZ)
i'm not convinced that the pref should not be migrated if !ENABLE_RLZ. a user
with a post-migrated pref won't lose it if we accidentally ship an rlz-free
build for a day, so why should a user that happens to be migrated via this code
on that day lose it?

https://codereview.chromium.org/2714853002/diff/160001/chrome/common/pref_nam...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/2714853002/diff/160001/chrome/common/pref_nam...
chrome/common/pref_names.cc:193: // Integer. RLZ ping delay in seconds.
i think it's actually milliseconds -- see chrome_browser_main.cc and
user_session_manager.cc.

https://codereview.chromium.org/2714853002/diff/160001/chrome/common/pref_nam...
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/2714853002/diff/160001/chrome/common/pref_nam...
chrome/common/pref_names.h:67: // Integer. RLZ ping delay in seconds.
nit: remove comment for consistency with surrounding code

https://codereview.chromium.org/2714853002/diff/160001/chrome/installer/util/...
File chrome/installer/util/master_preferences.h (left):

https://codereview.chromium.org/2714853002/diff/160001/chrome/installer/util/...
chrome/installer/util/master_preferences.h:46: //      "ping_delay": 40,
isn't this still the proper way to set the ping_delay?

[Roger Tawa OOO till Jul 10th, 2017-02-24 16:02:18]

Thanks for the clean up Gab.  Some comments below, plus discovery of a really
old bug :-)

So just to be clear, this CL should have no user visible changes if people
continue to set the rlz ping delay in the distribution dict (other than the
milliseconds to seconds fix)?  Is that correct?

https://codereview.chromium.org/2714853002/diff/160001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/2714853002/diff/160001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:1752:
base::TimeDelta::FromMilliseconds(abs(ping_delay)),
This should be FromSeconds() and not FromMilliseconds()!

Wow, this has been wrong for >4 years since this commit:

https://chromium.googlesource.com/chromium/src/+/99de6a50b41177fa48b770a9999c...

I'll inform concerned folks, but we should fix the code here.

https://codereview.chromium.org/2714853002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/session/user_session_manager.cc (right):

https://codereview.chromium.org/2714853002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/login/session/user_session_manager.cc:1445:
base::TimeDelta::FromMilliseconds(abs(ping_delay)),
This should be FromSeconds() too.

https://codereview.chromium.org/2714853002/diff/160001/chrome/browser/first_r...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/2714853002/diff/160001/chrome/browser/first_r...
chrome/browser/first_run/first_run.cc:455: master_prefs_path =
base::FilePath(first_run::internal::MasterPrefsPath());
nit: should have curly braces in on this if/else

https://codereview.chromium.org/2714853002/diff/160001/chrome/browser/first_r...
chrome/browser/first_run/first_run.cc:662:
registry->RegisterIntegerPref(prefs::kRlzPingDelay, 90);
On 2017/02/24 07:42:20, grt (UTC plus 1) wrote:
> this doesn't feel like the right place. rlz isn't specific to first_run. could
> you look for a more general place to register the pref? also, i wonder if
> local_state isn't a better place for it. maybe outside of scope of this cl,
but
> how about a comment/todo for later?

Could be moved to c/b/rlz/chrome_rlz_tracker_delegate.cc

https://codereview.chromium.org/2714853002/diff/160001/chrome/browser/first_r...
chrome/browser/first_run/first_run.cc:662:
registry->RegisterIntegerPref(prefs::kRlzPingDelay, 90);
On 2017/02/24 07:42:20, grt (UTC plus 1) wrote:
> while 90 has been the default, should it really be 90000 (see other comment
> about seconds vs milliseconds)? +rogerga to confirm.

See my other comment.  I believe this is a bug that has been around for a long
time.  Since all the docs talk about seconds and existing master prefs use
seconds, lets keep seconds and fix the code to handle it correctly.

https://codereview.chromium.org/2714853002/diff/160001/chrome/browser/prefs/b...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/2714853002/diff/160001/chrome/browser/prefs/b...
chrome/browser/prefs/browser_prefs.cc:753: #if BUILDFLAG(ENABLE_RLZ)
On 2017/02/24 07:42:21, grt (UTC plus 1) wrote:
> i'm not convinced that the pref should not be migrated if !ENABLE_RLZ. a user
> with a post-migrated pref won't lose it if we accidentally ship an rlz-free
> build for a day, so why should a user that happens to be migrated via this
code
> on that day lose it?

What is the chance of that?  It's like saying we accidentally ship a chromium
build instead of a chrome build for a day.  But maybe better safe than sorry. 
Also need to remove ifdefs from master_preferences.cc and its unittest.

https://codereview.chromium.org/2714853002/diff/160001/chrome/common/pref_nam...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/2714853002/diff/160001/chrome/common/pref_nam...
chrome/common/pref_names.cc:193: // Integer. RLZ ping delay in seconds.
On 2017/02/24 07:42:21, grt (UTC plus 1) wrote:
> i think it's actually milliseconds -- see chrome_browser_main.cc and
> user_session_manager.cc.

As mentioned already earlier, let's keep the second definition since there is
documentation that talks about this and existing master_prefs.  Let's change
those two call sites.

https://codereview.chromium.org/2714853002/diff/160001/chrome/installer/util/...
File chrome/installer/util/master_preferences.h (left):

https://codereview.chromium.org/2714853002/diff/160001/chrome/installer/util/...
chrome/installer/util/master_preferences.h:46: //      "ping_delay": 40,
On 2017/02/24 07:42:21, grt (UTC plus 1) wrote:
> isn't this still the proper way to set the ping_delay?

Agreed.  Until the docs are changed, everyone will continue to use this method. 
So we need to make sure it still works all the time.

https://codereview.chromium.org/2714853002/diff/160001/chrome/installer/util/...
File chrome/installer/util/master_preferences_unittest.cc (right):

https://codereview.chromium.org/2714853002/diff/160001/chrome/installer/util/...
chrome/installer/util/master_preferences_unittest.cc:348: #endif  //
BUILDFLAG(ENABLE_RLZ)
Make this its own separate test since it seems weird to test legacy RLZ support
in a test called EnforceLegacyCreateAllShortcutsFalse.

[gab, 2017-02-28 19:46:00]

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

[commit-bot: I haz the power, 2017-02-28 19:46:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2017-02-28 19:53:32]

Patchset #6 (id:180001) has been deleted

[gab, 2017-02-28 19:54:46]

Comments addressed, thanks, PTanL!

https://codereview.chromium.org/2714853002/diff/160001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/2714853002/diff/160001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:1752:
base::TimeDelta::FromMilliseconds(abs(ping_delay)),
On 2017/02/24 16:02:17, Roger Tawa wrote:
> This should be FromSeconds() and not FromMilliseconds()!
> 
> Wow, this has been wrong for >4 years since this commit:
> 
>
https://chromium.googlesource.com/chromium/src/+/99de6a50b41177fa48b770a9999c...
> 
> I'll inform concerned folks, but we should fix the code here.

Done.

https://codereview.chromium.org/2714853002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/session/user_session_manager.cc (right):

https://codereview.chromium.org/2714853002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/login/session/user_session_manager.cc:1445:
base::TimeDelta::FromMilliseconds(abs(ping_delay)),
On 2017/02/24 16:02:17, Roger Tawa wrote:
> This should be FromSeconds() too.

Done.

https://codereview.chromium.org/2714853002/diff/160001/chrome/browser/first_r...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/2714853002/diff/160001/chrome/browser/first_r...
chrome/browser/first_run/first_run.cc:455: master_prefs_path =
base::FilePath(first_run::internal::MasterPrefsPath());
On 2017/02/24 16:02:17, Roger Tawa wrote:
> nit: should have curly braces in on this if/else

Done.

https://codereview.chromium.org/2714853002/diff/160001/chrome/browser/first_r...
chrome/browser/first_run/first_run.cc:662:
registry->RegisterIntegerPref(prefs::kRlzPingDelay, 90);
On 2017/02/24 07:42:20, grt (UTC plus 1) wrote:
> this doesn't feel like the right place. rlz isn't specific to first_run. could
> you look for a more general place to register the pref? also, i wonder if
> local_state isn't a better place for it. maybe outside of scope of this cl,
but
> how about a comment/todo for later?

I think it's intentionally tied to the default profile? Either way, I'm
unwilling to do anything to change that...

https://codereview.chromium.org/2714853002/diff/160001/chrome/browser/first_r...
chrome/browser/first_run/first_run.cc:662:
registry->RegisterIntegerPref(prefs::kRlzPingDelay, 90);
On 2017/02/24 16:02:17, Roger Tawa wrote:
> On 2017/02/24 07:42:20, grt (UTC plus 1) wrote:
> > this doesn't feel like the right place. rlz isn't specific to first_run.
could
> > you look for a more general place to register the pref? also, i wonder if
> > local_state isn't a better place for it. maybe outside of scope of this cl,
> but
> > how about a comment/todo for later?
> 
> Could be moved to c/b/rlz/chrome_rlz_tracker_delegate.cc

Done.

https://codereview.chromium.org/2714853002/diff/160001/chrome/browser/prefs/b...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/2714853002/diff/160001/chrome/browser/prefs/b...
chrome/browser/prefs/browser_prefs.cc:753: #if BUILDFLAG(ENABLE_RLZ)
On 2017/02/24 16:02:17, Roger Tawa wrote:
> On 2017/02/24 07:42:21, grt (UTC plus 1) wrote:
> > i'm not convinced that the pref should not be migrated if !ENABLE_RLZ. a
user
> > with a post-migrated pref won't lose it if we accidentally ship an rlz-free
> > build for a day, so why should a user that happens to be migrated via this
> code
> > on that day lose it?
> 
> What is the chance of that?  It's like saying we accidentally ship a chromium
> build instead of a chrome build for a day.  But maybe better safe than sorry. 
> Also need to remove ifdefs from master_preferences.cc and its unittest.

Agreed that we shouldn't worry about shipping a non-RLZ build (and it's not like
this would be destructive if so either, it would just skip the migration for
that one build, which can't need it if it has RLZ disabled so...).

The reason it's easier to ifdef all is that prefs::kRlzPingDelay is behind
ENABLE_RLZ and so this code has to oblige (not ifdef'ing pref would be uncommon
and would allow non-ifdefed code that relies on RLZ being enabled anyways..).

https://codereview.chromium.org/2714853002/diff/160001/chrome/common/pref_nam...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/2714853002/diff/160001/chrome/common/pref_nam...
chrome/common/pref_names.cc:193: // Integer. RLZ ping delay in seconds.
On 2017/02/24 16:02:18, Roger Tawa wrote:
> On 2017/02/24 07:42:21, grt (UTC plus 1) wrote:
> > i think it's actually milliseconds -- see chrome_browser_main.cc and
> > user_session_manager.cc.
> 
> As mentioned already earlier, let's keep the second definition since there is
> documentation that talks about this and existing master_prefs.  Let's change
> those two call sites.

Acknowledged.

https://codereview.chromium.org/2714853002/diff/160001/chrome/common/pref_nam...
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/2714853002/diff/160001/chrome/common/pref_nam...
chrome/common/pref_names.h:67: // Integer. RLZ ping delay in seconds.
On 2017/02/24 07:42:21, grt (UTC plus 1) wrote:
> nit: remove comment for consistency with surrounding code

Done.

https://codereview.chromium.org/2714853002/diff/160001/chrome/installer/util/...
File chrome/installer/util/master_preferences.h (left):

https://codereview.chromium.org/2714853002/diff/160001/chrome/installer/util/...
chrome/installer/util/master_preferences.h:46: //      "ping_delay": 40,
On 2017/02/24 16:02:18, Roger Tawa wrote:
> On 2017/02/24 07:42:21, grt (UTC plus 1) wrote:
> > isn't this still the proper way to set the ping_delay?
> 
> Agreed.  Until the docs are changed, everyone will continue to use this
method. 
> So we need to make sure it still works all the time.

Good point (and I'm not signing up to update any docs -- just wanted to
straighten the master pref versus runtime Preferences story, I'm fine keeping
this first run mapping virtually forever).

https://codereview.chromium.org/2714853002/diff/160001/chrome/installer/util/...
File chrome/installer/util/master_preferences_unittest.cc (right):

https://codereview.chromium.org/2714853002/diff/160001/chrome/installer/util/...
chrome/installer/util/master_preferences_unittest.cc:348: #endif  //
BUILDFLAG(ENABLE_RLZ)
On 2017/02/24 16:02:18, Roger Tawa wrote:
> Make this its own separate test since it seems weird to test legacy RLZ
support
> in a test called EnforceLegacyCreateAllShortcutsFalse.

Good catch on the test name being outdated with this CL. I'd rather keep it as
one test though since it's testing a single method. Also used in
https://codereview.chromium.org/2705113005/ so it clearly goes beyond shortcuts.

[gab, 2017-02-28 19:55:29]

Description was changed from

==========
Remove kDistroDict from Preferences.

Extracted from https://codereview.chromium.org/2705113005.

kDistroDict was mapped along with other master_preferences into Preferences
but was never registered to be read at runtime... except for the RLZ
ping delay that is... which is now mapped to a real pref to keep things cleaner.

BUG=555550
==========

to

==========
Remove kDistroDict from Preferences.

Extracted from https://codereview.chromium.org/2705113005.

kDistroDict was mapped along with other master_preferences into Preferences
but was never registered to be read at runtime... except for the RLZ
ping delay that is... which is now mapped to a real pref to keep things cleaner.

Also fixes a long standing mistake to send the ping delay as milliseconds
instead of seconds..!

BUG=555550
==========

[gab, 2017-02-28 19:55:49]

Description was changed from

==========
Remove kDistroDict from Preferences.

Extracted from https://codereview.chromium.org/2705113005.

kDistroDict was mapped along with other master_preferences into Preferences
but was never registered to be read at runtime... except for the RLZ
ping delay that is... which is now mapped to a real pref to keep things cleaner.

Also fixes a long standing mistake to send the ping delay as milliseconds
instead of seconds..!

BUG=555550
==========

to

==========
Remove kDistroDict from Preferences.

Extracted from https://codereview.chromium.org/2705113005.

kDistroDict was mapped along with other master_preferences into Preferences
but was never registered to be read at runtime... except for the RLZ
ping delay that is... which is now mapped to a real pref to keep things cleaner.

Also fixes a long standing mistake to send the ping delay as milliseconds
instead of seconds..!
https://codereview.chromium.org/2714853002/diff/160001/chrome/browser/chrome_...

BUG=555550
==========

[commit-bot: I haz the power, 2017-02-28 19:59:30]

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

[commit-bot: I haz the power, 2017-02-28 19:59:31]

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

[grt (UTC plus 2), 2017-02-28 20:09:07]

lgtm. the change in behavior makes me a little squeamish, but i can't imagine it
affecting us.

https://codereview.chromium.org/2714853002/diff/160001/chrome/browser/prefs/b...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/2714853002/diff/160001/chrome/browser/prefs/b...
chrome/browser/prefs/browser_prefs.cc:753: #if BUILDFLAG(ENABLE_RLZ)
On 2017/02/28 19:54:45, gab (UTC plus 9 Tokyo) wrote:
> On 2017/02/24 16:02:17, Roger Tawa wrote:
> > On 2017/02/24 07:42:21, grt (UTC plus 1) wrote:
> > > i'm not convinced that the pref should not be migrated if !ENABLE_RLZ. a
> user
> > > with a post-migrated pref won't lose it if we accidentally ship an
rlz-free
> > > build for a day, so why should a user that happens to be migrated via this
> > code
> > > on that day lose it?
> > 
> > What is the chance of that?  It's like saying we accidentally ship a
chromium
> > build instead of a chrome build for a day.  But maybe better safe than
sorry. 
> > Also need to remove ifdefs from master_preferences.cc and its unittest.
> 
> Agreed that we shouldn't worry about shipping a non-RLZ build (and it's not
like
> this would be destructive if so either, it would just skip the migration for
> that one build, which can't need it if it has RLZ disabled so...).

It is destructive -- kDistroDict is cleared on line 762. I agree that it's
difficult to imagine us somehow falling into this trap, but it is a change in
behavior. Prior to this change, ping_delay set via master prefs will be used
if/when a Chrome that uses RLZ picks up prefs created with it. After this
change, ping_delay vanishes during profile load if RLZ isn't supported by
whatever Chrome loads it, and then is gone thereafter.

[gab, 2017-02-28 22:30:41]

https://codereview.chromium.org/2714853002/diff/160001/chrome/browser/prefs/b...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/2714853002/diff/160001/chrome/browser/prefs/b...
chrome/browser/prefs/browser_prefs.cc:753: #if BUILDFLAG(ENABLE_RLZ)
On 2017/02/28 20:09:07, grt (UTC plus 1) wrote:
> On 2017/02/28 19:54:45, gab (UTC plus 9 Tokyo) wrote:
> > On 2017/02/24 16:02:17, Roger Tawa wrote:
> > > On 2017/02/24 07:42:21, grt (UTC plus 1) wrote:
> > > > i'm not convinced that the pref should not be migrated if !ENABLE_RLZ. a
> > user
> > > > with a post-migrated pref won't lose it if we accidentally ship an
> rlz-free
> > > > build for a day, so why should a user that happens to be migrated via
this
> > > code
> > > > on that day lose it?
> > > 
> > > What is the chance of that?  It's like saying we accidentally ship a
> chromium
> > > build instead of a chrome build for a day.  But maybe better safe than
> sorry. 
> > > Also need to remove ifdefs from master_preferences.cc and its unittest.
> > 
> > Agreed that we shouldn't worry about shipping a non-RLZ build (and it's not
> like
> > this would be destructive if so either, it would just skip the migration for
> > that one build, which can't need it if it has RLZ disabled so...).
> 
> It is destructive -- kDistroDict is cleared on line 762. I agree that it's
> difficult to imagine us somehow falling into this trap, but it is a change in
> behavior. Prior to this change, ping_delay set via master prefs will be used
> if/when a Chrome that uses RLZ picks up prefs created with it. After this
> change, ping_delay vanishes during profile load if RLZ isn't supported by
> whatever Chrome loads it, and then is gone thereafter.

Ah right but what you're describing is caused by code in first_run.cc, not here.

I guess if we did ship a build without RLZ, we'd be losing custom ping delays
for users that either (1) go through first run with it or (2) go through one
time migration with it.

That is, it wouldn't affect nearly all users and it would just mean a fallback
to the default value. I'm willing to live with that being the result of skipping
a non RLZ build (which sounds unlikely as rogerta also said).

[gab, 2017-03-01 00:54:43]

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

[commit-bot: I haz the power, 2017-03-01 00:55:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2017-03-01 00:56:55]

gab@chromium.org changed reviewers:
+ alemate@chromium.org, sky@chromium.org

[gab, 2017-03-01 00:56:56]

@alemate for chrome/browser/chromeos/login/session/user_session_manager.cc

@sky for chrome_browser_main.cc and chrome/common/DEPS

Thanks,
Gab

[gab, 2017-03-01 00:57:35]

On 2017/03/01 00:56:56, gab (UTC plus 9 Tokyo) wrote:
> @alemate for chrome/browser/chromeos/login/session/user_session_manager.cc
> 
> @sky for chrome_browser_main.cc and chrome/common/DEPS

and @rogerta for rlz/

> 
> Thanks,
> Gab

[commit-bot: I haz the power, 2017-03-01 03:10:57]

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

[commit-bot: I haz the power, 2017-03-01 03:10:58]

Dry run: This issue passed the CQ dry run.

[sky, 2017-03-01 03:52:09]

https://codereview.chromium.org/2714853002/diff/230001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/2714853002/diff/230001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:1744: int ping_delay =
profile_->GetPrefs()->GetInteger(prefs::kRlzPingDelay);
optional: How about renaming the pref kRlzPingDelaySeconds to make the units
clear?

https://codereview.chromium.org/2714853002/diff/230001/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (right):

https://codereview.chromium.org/2714853002/diff/230001/chrome/common/BUILD.gn...
chrome/common/BUILD.gn:584: "//rlz/features",
Should this be conditional based on rlz being enabled?

[gab, 2017-03-01 04:26:56]

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

[commit-bot: I haz the power, 2017-03-01 04:27:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2017-03-01 04:28:03]

@sky: 1 done, 1 replied. Thanks.

@alemate: chromeos/login side-effects

@rogerta: rlz/

https://codereview.chromium.org/2714853002/diff/230001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/2714853002/diff/230001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:1744: int ping_delay =
profile_->GetPrefs()->GetInteger(prefs::kRlzPingDelay);
On 2017/03/01 03:52:09, sky wrote:
> optional: How about renaming the pref kRlzPingDelaySeconds to make the units
> clear?

Done.

https://codereview.chromium.org/2714853002/diff/230001/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (right):

https://codereview.chromium.org/2714853002/diff/230001/chrome/common/BUILD.gn...
chrome/common/BUILD.gn:584: "//rlz/features",
On 2017/03/01 03:52:09, sky wrote:
> Should this be conditional based on rlz being enabled?

I don't think so? It's there for rlz/features/features.h includes for #if
BUILDFLAG(ENABLE_RLZ) -- just like the other unconditional /features deps above.
We need to be able to resolve the BUILDFLAG whether it's enabled or not I think.

[commit-bot: I haz the power, 2017-03-01 05:42:41]

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

[commit-bot: I haz the power, 2017-03-01 05:42:42]

Dry run: This issue passed the CQ dry run.

[sky, 2017-03-01 16:37:06]

LGTM

https://codereview.chromium.org/2714853002/diff/230001/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (right):

https://codereview.chromium.org/2714853002/diff/230001/chrome/common/BUILD.gn...
chrome/common/BUILD.gn:584: "//rlz/features",
On 2017/03/01 04:28:02, gab (slow - travel and perf) wrote:
> On 2017/03/01 03:52:09, sky wrote:
> > Should this be conditional based on rlz being enabled?
> 
> I don't think so? It's there for rlz/features/features.h includes for #if
> BUILDFLAG(ENABLE_RLZ) -- just like the other unconditional /features deps
above.
> We need to be able to resolve the BUILDFLAG whether it's enabled or not I
think.

I thought it was the whole rlz. That makes sense, thanks.

[Roger Tawa OOO till Jul 10th, 2017-03-01 19:30:46]

lgtm

Thanks Gab.

[Alexander Alekseev, 2017-03-01 21:54:10]

chrome/browser/chromeos/login/session/user_session_manager.cc

lgtm

[gab, 2017-03-02 00:12:56]

The CQ bit was checked by gab@chromium.org

[gab, 2017-03-02 00:12:57]

The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org
Link to the patchset: https://codereview.chromium.org/2714853002/#ps250001
(title: "kRlzPingDelay -> kRlzPingDelaySeconds")

[commit-bot: I haz the power, 2017-03-02 00:13: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, 2017-03-02 00:19:45]

CQ is committing da patch.

Bot data: {"patchset_id": 250001, "attempt_start_ts": 1488413576655940,
"parent_rev": "b448a6cf53d18cd8864c0c0f4b9b3cf1b5aefd92", "commit_rev":
"3ca4a4913a34992aaa78c25a6e304aef8fca340b"}

[commit-bot: I haz the power, 2017-03-02 00:20:22]

Description was changed from

==========
Remove kDistroDict from Preferences.

Extracted from https://codereview.chromium.org/2705113005.

kDistroDict was mapped along with other master_preferences into Preferences
but was never registered to be read at runtime... except for the RLZ
ping delay that is... which is now mapped to a real pref to keep things cleaner.

Also fixes a long standing mistake to send the ping delay as milliseconds
instead of seconds..!
https://codereview.chromium.org/2714853002/diff/160001/chrome/browser/chrome_...

BUG=555550
==========

to

==========
Remove kDistroDict from Preferences.

Extracted from https://codereview.chromium.org/2705113005.

kDistroDict was mapped along with other master_preferences into Preferences
but was never registered to be read at runtime... except for the RLZ
ping delay that is... which is now mapped to a real pref to keep things cleaner.

Also fixes a long standing mistake to send the ping delay as milliseconds
instead of seconds..!
https://codereview.chromium.org/2714853002/diff/160001/chrome/browser/chrome_...

BUG=555550

Review-Url: https://codereview.chromium.org/2714853002
Cr-Commit-Position: refs/heads/master@{#454115}
Committed:
https://chromium.googlesource.com/chromium/src/+/3ca4a4913a34992aaa78c25a6e30...
==========

[commit-bot: I haz the power, 2017-03-02 00:20:24]

Committed patchset #9 (id:250001) as
https://chromium.googlesource.com/chromium/src/+/3ca4a4913a34992aaa78c25a6e30...