Migrate Cryptauth*Managers RegisterPrefs to CryptAuthService::RegisterProfilePrefs.

Migrate Cryptauth*Managers RegisterPrefs to CryptAuthService::RegisterProfilePrefs.

BUG=672263
Review-Url: https://codereview.chromium.org/2805833002
Cr-Commit-Position: refs/heads/master@{#464159}
Committed: https://chromium.googlesource.com/chromium/src/+/25db618aea911fe86805019fc2727f025786f310

[Ryan Hansberry, 2017-04-07 04:01:13]

Description was changed from

==========
Migrate Cryptauth*Managers RegisterPrefs to
CryptAuthService::RegisterProfilePrefs.

BUG=672263
==========

to

==========
Migrate Cryptauth*Managers RegisterPrefs to
CryptAuthService::RegisterProfilePrefs.

BUG=672263
==========

[Ryan Hansberry, 2017-04-07 04:01:13]

hansberry@chromium.org changed reviewers:
+ khorimoto@chromium.org, tengs@chromium.org

[Ryan Hansberry, 2017-04-07 04:02:25]

This change is necessary because Tether and EasyUnlock both need
Cryptauth{Device|Enrollment|Gcm}Manager::RegisterPrefs to be called, but only
once.

[Kyle Horimoto, 2017-04-07 16:58:52]

lgtm

https://codereview.chromium.org/2805833002/diff/1/chrome/browser/prefs/browse...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/2805833002/diff/1/chrome/browser/prefs/browse...
chrome/browser/prefs/browser_prefs.cc:76: #include
"components/cryptauth/cryptauth_service.h"
Wrap with #if BUILDFLAG(ENABLE_EXTENSIONS) || defined(OS_CHROMEOS)

[Tim Song, 2017-04-07 17:56:30]

https://codereview.chromium.org/2805833002/diff/1/chrome/browser/prefs/browse...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/2805833002/diff/1/chrome/browser/prefs/browse...
chrome/browser/prefs/browser_prefs.cc:76: #include
"components/cryptauth/cryptauth_service.h"
On 2017/04/07 16:58:52, Kyle Horimoto wrote:
> Wrap with #if BUILDFLAG(ENABLE_EXTENSIONS) || defined(OS_CHROMEOS)

I would just put this under defined(OS_CHROMEOS) for now. This ENABLE_EXTENSION
flow isn't being used.

[Ryan Hansberry, 2017-04-07 18:18:10]

https://codereview.chromium.org/2805833002/diff/1/chrome/browser/prefs/browse...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/2805833002/diff/1/chrome/browser/prefs/browse...
chrome/browser/prefs/browser_prefs.cc:76: #include
"components/cryptauth/cryptauth_service.h"
On 2017/04/07 17:56:30, Tim Song wrote:
> On 2017/04/07 16:58:52, Kyle Horimoto wrote:
> > Wrap with #if BUILDFLAG(ENABLE_EXTENSIONS) || defined(OS_CHROMEOS)
> 
> I would just put this under defined(OS_CHROMEOS) for now. This
ENABLE_EXTENSION
> flow isn't being used.

Are you sure tengs@? If EasyUnlock is not just on ChromeOS, then how will it
work in a non-ChromeOS context without CryptAuthService?

[Ryan Hansberry, 2017-04-07 18:18:49]

hansberry@chromium.org changed reviewers:
+ battre@chromium.org

[Ryan Hansberry, 2017-04-07 18:19:03]

Adding battre@ for browser_prefs.cc.

[Tim Song, 2017-04-07 20:04:45]

https://codereview.chromium.org/2805833002/diff/1/chrome/browser/prefs/browse...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/2805833002/diff/1/chrome/browser/prefs/browse...
chrome/browser/prefs/browser_prefs.cc:76: #include
"components/cryptauth/cryptauth_service.h"
On 2017/04/07 18:18:10, Ryan Hansberry wrote:
> On 2017/04/07 17:56:30, Tim Song wrote:
> > On 2017/04/07 16:58:52, Kyle Horimoto wrote:
> > > Wrap with #if BUILDFLAG(ENABLE_EXTENSIONS) || defined(OS_CHROMEOS)
> > 
> > I would just put this under defined(OS_CHROMEOS) for now. This
> ENABLE_EXTENSION
> > flow isn't being used.
> 
> Are you sure tengs@? If EasyUnlock is not just on ChromeOS, then how will it
> work in a non-ChromeOS context without CryptAuthService?

Please also move EasyUnlockService::RegisterPrefs() to OS_CHROMEOS, also. We no
longer have plans of running this on other platforms at the moment.

[Ryan Hansberry, 2017-04-07 21:00:06]

https://codereview.chromium.org/2805833002/diff/1/chrome/browser/prefs/browse...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/2805833002/diff/1/chrome/browser/prefs/browse...
chrome/browser/prefs/browser_prefs.cc:76: #include
"components/cryptauth/cryptauth_service.h"
On 2017/04/07 20:04:45, Tim Song wrote:
> On 2017/04/07 18:18:10, Ryan Hansberry wrote:
> > On 2017/04/07 17:56:30, Tim Song wrote:
> > > On 2017/04/07 16:58:52, Kyle Horimoto wrote:
> > > > Wrap with #if BUILDFLAG(ENABLE_EXTENSIONS) || defined(OS_CHROMEOS)
> > > 
> > > I would just put this under defined(OS_CHROMEOS) for now. This
> > ENABLE_EXTENSION
> > > flow isn't being used.
> > 
> > Are you sure tengs@? If EasyUnlock is not just on ChromeOS, then how will it
> > work in a non-ChromeOS context without CryptAuthService?
> 
> Please also move EasyUnlockService::RegisterPrefs() to OS_CHROMEOS, also. We
no
> longer have plans of running this on other platforms at the moment.

Done.

[Tim Song, 2017-04-07 22:09:26]

lgtm

[Ryan Hansberry, 2017-04-07 22:12:26]

hansberry@chromium.org changed reviewers:
+ pam@chromium.org
- battre@chromium.org

[Ryan Hansberry, 2017-04-07 22:14:10]

Changing browser_prefs.cc reviewer to pam@ for time zone reasons.

[Ryan Hansberry, 2017-04-07 22:48:37]

hansberry@chromium.org changed reviewers:
+ battre@chromium.org
- pam@chromium.org

[Ryan Hansberry, 2017-04-07 22:48:46]

Changing reviewer for browser_prefs.cc back to battre@.

[battre, 2017-04-09 14:37:18]

LGTM

Note: If all the components were keyed services, you could use
KeyedServiceBaseFactory::RegisterPrefs to register your preferences exactly once
even if some components are used by two other components. But I don't know
enough about Teather, EasyUnlock and Cryptauth to judge whether that makes
sense. But it might help making the dependencies and initialization a bit more
explicit.

[Ryan Hansberry, 2017-04-10 21:28:04]

On 2017/04/09 14:37:18, battre wrote:
> LGTM
> 
> Note: If all the components were keyed services, you could use
> KeyedServiceBaseFactory::RegisterPrefs to register your preferences exactly
once
> even if some components are used by two other components. But I don't know
> enough about Teather, EasyUnlock and Cryptauth to judge whether that makes
> sense. But it might help making the dependencies and initialization a bit more
> explicit.

Noted -- thank you for the heads up! Created crbug.com/710174 to keep track of
this and we will eventually get around to doing this.

[Ryan Hansberry, 2017-04-10 21:28:09]

The CQ bit was checked by hansberry@chromium.org

[Ryan Hansberry, 2017-04-10 21:28:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
khorimoto@chromium.org
Link to the patchset: https://codereview.chromium.org/2805833002/#ps40001
(title: "Move EasyUnlockService to ChromeOS only.")

[commit-bot: I haz the power, 2017-04-10 21:29:15]

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-04-10 23:15:59]

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

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

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

[Ryan Hansberry, 2017-04-11 01:34:38]

Hey tengs@, this change fails a few tests, presumably because we moved
EasyUnlock from under ENABLE_EXTENIONS to OS_CHROMEOS. Tests are crashing on
this line:
https://cs.chromium.org/chromium/src/chrome/browser/signin/easy_unlock_servic...,
see
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.win%2...
for more.

What do you suggest to fix this?

[Ryan Hansberry, 2017-04-11 21:48:57]

On 2017/04/11 01:34:38, Ryan Hansberry wrote:
> Hey tengs@, this change fails a few tests, presumably because we moved
> EasyUnlock from under ENABLE_EXTENIONS to OS_CHROMEOS. Tests are crashing on
> this line:
>
https://cs.chromium.org/chromium/src/chrome/browser/signin/easy_unlock_servic...,
> see
>
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.win%2...
> for more.
> 
> What do you suggest to fix this?

tengs@ and I agreed offline to just revert EasyUnlock behind the flag it
previously was.

[Ryan Hansberry, 2017-04-11 21:50:19]

battre@ do you mind lg'ing for browser_prefs.cc again?

[battre, 2017-04-12 10:46:05]

On 2017/04/11 21:50:19, Ryan Hansberry wrote:
> battre@ do you mind lg'ing for browser_prefs.cc again?

LGTM with comment.

[Ryan Hansberry, 2017-04-12 16:12:33]

The CQ bit was checked by hansberry@chromium.org

[Ryan Hansberry, 2017-04-12 16:12:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
khorimoto@chromium.org, tengs@chromium.org, battre@chromium.org
Link to the patchset: https://codereview.chromium.org/2805833002/#ps80001
(title: "Rebase on master.")

[commit-bot: I haz the power, 2017-04-12 16:13:18]

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-04-12 16:31:56]

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

[commit-bot: I haz the power, 2017-04-12 16:31:57]

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

[Ryan Hansberry, 2017-04-12 18:27:15]

The CQ bit was checked by hansberry@chromium.org

[Ryan Hansberry, 2017-04-12 18:27:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
khorimoto@chromium.org, tengs@chromium.org, battre@chromium.org
Link to the patchset: https://codereview.chromium.org/2805833002/#ps100001
(title: "Fix typo.")

[commit-bot: I haz the power, 2017-04-12 18:28:01]

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

[Ryan Hansberry, 2017-04-12 18:32:45]

The CQ bit was checked by hansberry@chromium.org

[Ryan Hansberry, 2017-04-12 18:32:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
khorimoto@chromium.org, tengs@chromium.org, battre@chromium.org
Link to the patchset: https://codereview.chromium.org/2805833002/#ps120001
(title: "Replace initializer_unittest RegisterPrefs calls with
Initializer::RegisterProfilePrefs.")

[commit-bot: I haz the power, 2017-04-12 18:34: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, 2017-04-12 21:45:39]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1492021965556580,
"parent_rev": "7bd582aedcc8876a3cf9c17145881eb1ff89e74e", "commit_rev":
"25db618aea911fe86805019fc2727f025786f310"}

[commit-bot: I haz the power, 2017-04-12 21:46:40]

Description was changed from

==========
Migrate Cryptauth*Managers RegisterPrefs to
CryptAuthService::RegisterProfilePrefs.

BUG=672263
==========

to

==========
Migrate Cryptauth*Managers RegisterPrefs to
CryptAuthService::RegisterProfilePrefs.

BUG=672263

Review-Url: https://codereview.chromium.org/2805833002
Cr-Commit-Position: refs/heads/master@{#464159}
Committed:
https://chromium.googlesource.com/chromium/src/+/25db618aea911fe86805019fc272...
==========

[commit-bot: I haz the power, 2017-04-12 21:46:41]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/25db618aea911fe86805019fc272...