Create TetherService, a TetherAllowed pref, and use DependencyManager for RegisterProfilePrefs call…

Create TetherService, a TetherAllowed pref, and use DependencyManager for RegisterProfilePrefs calls.

TetherService dynamically starts and stops Tether when prefs::kTetherAllowed changes value (from policy changing). This is expected from policy settings in Chrome. Please see: https://sites.google.com/a/chromium.org/dev/developers/how-tos/enterprise/adding-new-policies.

This CL also tweaks EasyUnlockServiceFactory and ChromeCryptAuthServiceFactory to call RegisterProfilePrefs on their respective services. These calls are automagically handled by DependencyManager: it ensures that the RegisterProfilePrefs calls are made in the correct order and only once per context.

A subsequent CL will come after this one that actually creates the enterprise policy for Tether, and maps to prefs::kTetherAllowed.

BUG=710174
Review-Url: https://codereview.chromium.org/2819713002
Cr-Commit-Position: refs/heads/master@{#466702}
Committed: https://chromium.googlesource.com/chromium/src/+/8d01f3ac9dad3f4833d868e253989ed88b705d1c

[Ryan Hansberry, 2017-04-13 23:16:14]

Description was changed from

==========
Create TetherService, a TetherAllowed pref, and use DependencyManager for
RegisterProfilePrefs calls.

BUG=710174
==========

to

==========
Create TetherService, a TetherAllowed pref, and use DependencyManager for
RegisterProfilePrefs calls.

TetherService dynamically starts and stops Tether when prefs::kTetherAllowed
changes value (from policy changing). This is expected from policy settings in
Chrome. Please see:
https://sites.google.com/a/chromium.org/dev/developers/how-tos/enterprise/add....

This CL also tweaks EasyUnlockServiceFactory and ChromeCryptAuthServiceFactory
to call RegisterProfilePrefs on their respective services. These calls are
automagically handled by DependencyManager: it ensures that the
RegisterProfilePrefs calls are made in the correct order and only once per
context.

A subsequent CL will come after this one that actually creates the enterprise
policy for Tether, and maps to prefs::kTetherAllowed.

BUG=710174
==========

[Ryan Hansberry, 2017-04-13 23:16:14]

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

[Ryan Hansberry, 2017-04-13 23:18:07]

Description was changed from

==========
Create TetherService, a TetherAllowed pref, and use DependencyManager for
RegisterProfilePrefs calls.

TetherService dynamically starts and stops Tether when prefs::kTetherAllowed
changes value (from policy changing). This is expected from policy settings in
Chrome. Please see:
https://sites.google.com/a/chromium.org/dev/developers/how-tos/enterprise/add....

This CL also tweaks EasyUnlockServiceFactory and ChromeCryptAuthServiceFactory
to call RegisterProfilePrefs on their respective services. These calls are
automagically handled by DependencyManager: it ensures that the
RegisterProfilePrefs calls are made in the correct order and only once per
context.

A subsequent CL will come after this one that actually creates the enterprise
policy for Tether, and maps to prefs::kTetherAllowed.

BUG=710174
==========

to

==========
Create TetherService, a TetherAllowed pref, and use DependencyManager for
RegisterProfilePrefs calls.

TetherService dynamically starts and stops Tether when prefs::kTetherAllowed
changes value (from policy changing). This is expected from policy settings in
Chrome. Please see:
https://sites.google.com/a/chromium.org/dev/developers/how-tos/enterprise/add....

This CL also tweaks EasyUnlockServiceFactory and ChromeCryptAuthServiceFactory
to call RegisterProfilePrefs on their respective services. These calls are
automagically handled by DependencyManager: it ensures that the
RegisterProfilePrefs calls are made in the correct order and only once per
context.

A subsequent CL will come after this one that actually creates the enterprise
policy for Tether, and maps to prefs::kTetherAllowed.

battre@: Adding for browser_prefs.cc. Please let me know if I've done what you
suggested in https://codereview.chromium.org/2805833002/#msg16.
erg@: Adding for chrome_browser_main_extra_parts_profiles.cc.

BUG=710174
==========

[Kyle Horimoto, 2017-04-14 00:15:43]

https://codereview.chromium.org/2819713002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/session/user_session_manager.cc (left):

https://codereview.chromium.org/2819713002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/session/user_session_manager.cc:1947: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(
When will Shutdown() be called now? Are we sure that it's called at the right
time?

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

https://codereview.chromium.org/2819713002/diff/1/chrome/browser/prefs/browse...
chrome/browser/prefs/browser_prefs.cc:622:
chromeos::tether::Initializer::RegisterProfilePrefs(registry);
When is RegisterProfilePrefs() actually called now? It's unclear to me.

https://codereview.chromium.org/2819713002/diff/1/chrome/browser/tether/tethe...
File chrome/browser/tether/tether_service.cc (right):

https://codereview.chromium.org/2819713002/diff/1/chrome/browser/tether/tethe...
chrome/browser/tether/tether_service.cc:86: void TetherService::OnPrefsChanged()
{
What about if tethering is disabled, then reenabled? IsAllowed() will still
return false since shut_down_ == true.

[Kyle Horimoto, 2017-04-14 01:08:27]

https://codereview.chromium.org/2819713002/diff/1/chrome/browser/tether/tethe...
File chrome/browser/tether/tether_service.cc (right):

https://codereview.chromium.org/2819713002/diff/1/chrome/browser/tether/tethe...
chrome/browser/tether/tether_service.cc:71: if
(!profile_->GetPrefs()->GetBoolean(prefs::kTetherAllowed))
Just return profile_->GetPrefs()->GetBoolean(prefs::kTetherAllowed) instead of
using if/else.

https://codereview.chromium.org/2819713002/diff/1/chrome/browser/tether/tethe...
File chrome/browser/tether/tether_service.h (right):

https://codereview.chromium.org/2819713002/diff/1/chrome/browser/tether/tethe...
chrome/browser/tether/tether_service.h:17: // namespace user_prefs {
Remove if unused.

[Ryan Hansberry, 2017-04-14 02:34:10]

https://codereview.chromium.org/2819713002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/session/user_session_manager.cc (left):

https://codereview.chromium.org/2819713002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/session/user_session_manager.cc:1947: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2017/04/14 00:15:43, Kyle Horimoto wrote:
> When will Shutdown() be called now? Are we sure that it's called at the right
> time?

Initializer::Shutdown() is now called when either Tether is disallowed because
of a policy change, or when TetherService::Shutdown() is called (that is called
automagically by Chrome, see [1]). 

Additionally, in the future, Initializer::Shutdown will be called when Tether is
disabled (e.g. the user flips the pref in Settings).

1.
https://cs.chromium.org/chromium/src/components/keyed_service/core/keyed_serv...

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

https://codereview.chromium.org/2819713002/diff/1/chrome/browser/prefs/browse...
chrome/browser/prefs/browser_prefs.cc:622:
chromeos::tether::Initializer::RegisterProfilePrefs(registry);
On 2017/04/14 00:15:43, Kyle Horimoto wrote:
> When is RegisterProfilePrefs() actually called now? It's unclear to me.

When we call DependsOn() in our ServiceFactories, it creates a chain of
dependencies that DependencyManager keeps track of. DependencyManager calls
RegisterProfilePrefs() of *all* services in this dependency chain in the correct
order, and only once per context. The calls to RegisterProfilePrefs() are
essentially kicked off here:
https://cs.chromium.org/chromium/src/components/keyed_service/core/dependency...

https://codereview.chromium.org/2819713002/diff/1/chrome/browser/tether/tethe...
File chrome/browser/tether/tether_service.cc (right):

https://codereview.chromium.org/2819713002/diff/1/chrome/browser/tether/tethe...
chrome/browser/tether/tether_service.cc:71: if
(!profile_->GetPrefs()->GetBoolean(prefs::kTetherAllowed))
On 2017/04/14 01:08:26, Kyle Horimoto wrote:
> Just return profile_->GetPrefs()->GetBoolean(prefs::kTetherAllowed) instead of
> using if/else.

Done.

https://codereview.chromium.org/2819713002/diff/1/chrome/browser/tether/tethe...
chrome/browser/tether/tether_service.cc:86: void TetherService::OnPrefsChanged()
{
On 2017/04/14 00:15:43, Kyle Horimoto wrote:
> What about if tethering is disabled, then reenabled? IsAllowed() will still
> return false since shut_down_ == true.

Not sure I follow -- are you mixing up the Service being shutdown and
Initializer being shutdown? Initializer can be shutdown and re-initialized many
times within the lifetime of this service.

https://codereview.chromium.org/2819713002/diff/1/chrome/browser/tether/tethe...
File chrome/browser/tether/tether_service.h (right):

https://codereview.chromium.org/2819713002/diff/1/chrome/browser/tether/tethe...
chrome/browser/tether/tether_service.h:17: // namespace user_prefs {
On 2017/04/14 01:08:26, Kyle Horimoto wrote:
> Remove if unused.

Done.

[Elliot Glaysher, 2017-04-14 17:34:49]

profiles lgtm

[Kyle Horimoto, 2017-04-14 18:39:20]

https://codereview.chromium.org/2819713002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/session/user_session_manager.cc (left):

https://codereview.chromium.org/2819713002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/session/user_session_manager.cc:1947: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2017/04/14 02:34:10, Ryan Hansberry wrote:
> On 2017/04/14 00:15:43, Kyle Horimoto wrote:
> > When will Shutdown() be called now? Are we sure that it's called at the
right
> > time?
> 
> Initializer::Shutdown() is now called when either Tether is disallowed because
> of a policy change, or when TetherService::Shutdown() is called (that is
called
> automagically by Chrome, see [1]). 
> 
> Additionally, in the future, Initializer::Shutdown will be called when Tether
is
> disabled (e.g. the user flips the pref in Settings).
> 
> 1.
>
https://cs.chromium.org/chromium/src/components/keyed_service/core/keyed_serv...

Have you confirmed that Initializer::Shutdown() will be called before
NetworkHandler::Shutdown() and MessageCenter::Shutdown()? This needs to happen
to ensure that we don't crash when the user logs out.

https://codereview.chromium.org/2819713002/diff/1/chrome/browser/tether/tethe...
File chrome/browser/tether/tether_service.cc (right):

https://codereview.chromium.org/2819713002/diff/1/chrome/browser/tether/tethe...
chrome/browser/tether/tether_service.cc:71: if
(!profile_->GetPrefs()->GetBoolean(prefs::kTetherAllowed))
On 2017/04/14 02:34:10, Ryan Hansberry wrote:
> On 2017/04/14 01:08:26, Kyle Horimoto wrote:
> > Just return profile_->GetPrefs()->GetBoolean(prefs::kTetherAllowed) instead
of
> > using if/else.
> 
> Done.

I think you forgot to upload your changes :)

https://codereview.chromium.org/2819713002/diff/1/chrome/browser/tether/tethe...
chrome/browser/tether/tether_service.cc:86: void TetherService::OnPrefsChanged()
{
On 2017/04/14 02:34:10, Ryan Hansberry wrote:
> On 2017/04/14 00:15:43, Kyle Horimoto wrote:
> > What about if tethering is disabled, then reenabled? IsAllowed() will still
> > return false since shut_down_ == true.
> 
> Not sure I follow -- are you mixing up the Service being shutdown and
> Initializer being shutdown? Initializer can be shutdown and re-initialized
many
> times within the lifetime of this service.

Yep, I was mixing it up. Thanks for the explanation!

[Ryan Hansberry, 2017-04-18 17:59:05]

https://codereview.chromium.org/2819713002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/session/user_session_manager.cc (left):

https://codereview.chromium.org/2819713002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/session/user_session_manager.cc:1947: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2017/04/14 18:39:20, Kyle Horimoto wrote:
> On 2017/04/14 02:34:10, Ryan Hansberry wrote:
> > On 2017/04/14 00:15:43, Kyle Horimoto wrote:
> > > When will Shutdown() be called now? Are we sure that it's called at the
> right
> > > time?
> > 
> > Initializer::Shutdown() is now called when either Tether is disallowed
because
> > of a policy change, or when TetherService::Shutdown() is called (that is
> called
> > automagically by Chrome, see [1]). 
> > 
> > Additionally, in the future, Initializer::Shutdown will be called when
Tether
> is
> > disabled (e.g. the user flips the pref in Settings).
> > 
> > 1.
> >
>
https://cs.chromium.org/chromium/src/components/keyed_service/core/keyed_serv...
> 
> Have you confirmed that Initializer::Shutdown() will be called before
> NetworkHandler::Shutdown() and MessageCenter::Shutdown()? This needs to happen
> to ensure that we don't crash when the user logs out.

Good catch. Added a call to TetherService::Shutdown here.

https://codereview.chromium.org/2819713002/diff/1/chrome/browser/tether/tethe...
File chrome/browser/tether/tether_service.cc (right):

https://codereview.chromium.org/2819713002/diff/1/chrome/browser/tether/tethe...
chrome/browser/tether/tether_service.cc:71: if
(!profile_->GetPrefs()->GetBoolean(prefs::kTetherAllowed))
On 2017/04/14 18:39:20, Kyle Horimoto wrote:
> On 2017/04/14 02:34:10, Ryan Hansberry wrote:
> > On 2017/04/14 01:08:26, Kyle Horimoto wrote:
> > > Just return profile_->GetPrefs()->GetBoolean(prefs::kTetherAllowed)
instead
> of
> > > using if/else.
> > 
> > Done.
> 
> I think you forgot to upload your changes :)

Accidentally uploaded them to the CL depending on this, oops.

[Tim Song, 2017-04-18 19:08:44]

lgtm

[Ryan Hansberry, 2017-04-18 21:59:53]

https://codereview.chromium.org/2819713002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/session/user_session_manager.cc (left):

https://codereview.chromium.org/2819713002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/session/user_session_manager.cc:1947: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2017/04/18 17:59:05, Ryan Hansberry wrote:
> On 2017/04/14 18:39:20, Kyle Horimoto wrote:
> > On 2017/04/14 02:34:10, Ryan Hansberry wrote:
> > > On 2017/04/14 00:15:43, Kyle Horimoto wrote:
> > > > When will Shutdown() be called now? Are we sure that it's called at the
> > right
> > > > time?
> > > 
> > > Initializer::Shutdown() is now called when either Tether is disallowed
> because
> > > of a policy change, or when TetherService::Shutdown() is called (that is
> > called
> > > automagically by Chrome, see [1]). 
> > > 
> > > Additionally, in the future, Initializer::Shutdown will be called when
> Tether
> > is
> > > disabled (e.g. the user flips the pref in Settings).
> > > 
> > > 1.
> > >
> >
>
https://cs.chromium.org/chromium/src/components/keyed_service/core/keyed_serv...
> > 
> > Have you confirmed that Initializer::Shutdown() will be called before
> > NetworkHandler::Shutdown() and MessageCenter::Shutdown()? This needs to
happen
> > to ensure that we don't crash when the user logs out.
> 
> Good catch. Added a call to TetherService::Shutdown here.

Actually, this code was correct the first time. I have verified that
NetworkHandler::Shutdown() and MessageCenter::Shutdown() are called before
TetherService::Shutdown().

[Kyle Horimoto, 2017-04-18 22:03:44]

lgtm

[battre, 2017-04-19 08:43:30]

\o/

LGTM

Thanks.

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/session/user_session_manager.cc (right):

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/user_session_manager.cc:1230: if
(tether_service) {
nit: {} are not necessary.

[Ryan Hansberry, 2017-04-19 17:32:23]

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

[Ryan Hansberry, 2017-04-19 17:45:37]

hansberry@chromium.org changed reviewers:
+ thestig@chromium.org
- stevenjb@chromium.org

[Ryan Hansberry, 2017-04-19 17:46:06]

Added thestig@ for chrome/browser/ review.

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/session/user_session_manager.cc (right):

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/user_session_manager.cc:1230: if
(tether_service) {
On 2017/04/19 08:43:30, battre wrote:
> nit: {} are not necessary.

Done.

[Lei Zhang, 2017-04-19 19:01:43]

Can you remove messages to reviewers from the CL description?

[Lei Zhang, 2017-04-19 19:13:17]

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:3578: "tether/tether_service.cc",
Is this going to build on Windows/Mac/Linux too? You should run some try jobs
when you get closer to be able to submit.

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/session/user_session_manager.cc (right):

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/user_session_manager.cc:1230: if
(tether_service) {
Is this ever false?

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/tether/O...
File chrome/browser/tether/OWNERS (right):

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/tether/O...
chrome/browser/tether/OWNERS:2: hansberry@chromium.org
Do you have a bug component? If so, add a COMPONENT: line? See other OWNERS file
for examples.

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/tether/t...
File chrome/browser/tether/tether_service.cc (right):

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/tether/t...
chrome/browser/tether/tether_service.cc:11: #include
"chrome/browser/browser_process.h"
Not using g_browser_process - not needed?

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/tether/t...
chrome/browser/tether/tether_service.cc:23: #include
"components/proximity_auth/logging/logging.h"
Is this used for anything? If not, remove and remove the DEPS entry?

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/tether/t...
chrome/browser/tether/tether_service.cc:49: // TODO (hansberry): Switch to using
an IsEnabled() method.
no space after TODO.

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/tether/t...
File chrome/browser/tether/tether_service.h (right):

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/tether/t...
chrome/browser/tether/tether_service.h:11: #include "base/memory/weak_ptr.h"
No WeakPtrs here.

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/tether/t...
chrome/browser/tether/tether_service.h:12: #include "build/build_config.h"
Not needed since nothing is being #ifdef'd? Ditto in other tether files.

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/tether/t...
chrome/browser/tether/tether_service.h:17: // namespace user_prefs {
I think you should uncomment this and remove the #include.

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/tether/t...
File chrome/browser/tether/tether_service_factory.cc (right):

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/tether/t...
chrome/browser/tether/tether_service_factory.cc:7: #include
"base/command_line.h"
Not used.

https://codereview.chromium.org/2819713002/diff/60001/chrome/common/pref_name...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/2819713002/diff/60001/chrome/common/pref_name...
chrome/common/pref_names.cc:973: // Whether a user is allowed to use Tether.
Can you mention it's a boolean pref like the surrounding comments?

[Ryan Hansberry, 2017-04-20 16:56:03]

Description was changed from

==========
Create TetherService, a TetherAllowed pref, and use DependencyManager for
RegisterProfilePrefs calls.

TetherService dynamically starts and stops Tether when prefs::kTetherAllowed
changes value (from policy changing). This is expected from policy settings in
Chrome. Please see:
https://sites.google.com/a/chromium.org/dev/developers/how-tos/enterprise/add....

This CL also tweaks EasyUnlockServiceFactory and ChromeCryptAuthServiceFactory
to call RegisterProfilePrefs on their respective services. These calls are
automagically handled by DependencyManager: it ensures that the
RegisterProfilePrefs calls are made in the correct order and only once per
context.

A subsequent CL will come after this one that actually creates the enterprise
policy for Tether, and maps to prefs::kTetherAllowed.

battre@: Adding for browser_prefs.cc. Please let me know if I've done what you
suggested in https://codereview.chromium.org/2805833002/#msg16.
erg@: Adding for chrome_browser_main_extra_parts_profiles.cc.

BUG=710174
==========

to

==========
Create TetherService, a TetherAllowed pref, and use DependencyManager for
RegisterProfilePrefs calls.

TetherService dynamically starts and stops Tether when prefs::kTetherAllowed
changes value (from policy changing). This is expected from policy settings in
Chrome. Please see:
https://sites.google.com/a/chromium.org/dev/developers/how-tos/enterprise/add....

This CL also tweaks EasyUnlockServiceFactory and ChromeCryptAuthServiceFactory
to call RegisterProfilePrefs on their respective services. These calls are
automagically handled by DependencyManager: it ensures that the
RegisterProfilePrefs calls are made in the correct order and only once per
context.

A subsequent CL will come after this one that actually creates the enterprise
policy for Tether, and maps to prefs::kTetherAllowed.

BUG=710174
==========

[Ryan Hansberry, 2017-04-20 17:22:32]

Removed notes to reviewers in CL description.

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:3578: "tether/tether_service.cc",
On 2017/04/19 19:13:17, Lei Zhang wrote:
> Is this going to build on Windows/Mac/Linux too? You should run some try jobs
> when you get closer to be able to submit.

This is only intended to run on ChromeOS.

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/session/user_session_manager.cc (right):

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/user_session_manager.cc:1230: if
(tether_service) {
On 2017/04/19 19:13:17, Lei Zhang wrote:
> Is this ever false?

Nope. Removed if statement.

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/tether/O...
File chrome/browser/tether/OWNERS (right):

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/tether/O...
chrome/browser/tether/OWNERS:2: hansberry@chromium.org
On 2017/04/19 19:13:17, Lei Zhang wrote:
> Do you have a bug component? If so, add a COMPONENT: line? See other OWNERS
file
> for examples.

We don't have a bug component. :/

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/tether/t...
File chrome/browser/tether/tether_service.cc (right):

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/tether/t...
chrome/browser/tether/tether_service.cc:11: #include
"chrome/browser/browser_process.h"
On 2017/04/19 19:13:17, Lei Zhang wrote:
> Not using g_browser_process - not needed?

Done.

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/tether/t...
chrome/browser/tether/tether_service.cc:23: #include
"components/proximity_auth/logging/logging.h"
On 2017/04/19 19:13:17, Lei Zhang wrote:
> Is this used for anything? If not, remove and remove the DEPS entry?

Removed this and the DEPS entry.

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/tether/t...
chrome/browser/tether/tether_service.cc:49: // TODO (hansberry): Switch to using
an IsEnabled() method.
On 2017/04/19 19:13:17, Lei Zhang wrote:
> no space after TODO.

Done.

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/tether/t...
File chrome/browser/tether/tether_service.h (right):

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/tether/t...
chrome/browser/tether/tether_service.h:11: #include "base/memory/weak_ptr.h"
On 2017/04/19 19:13:17, Lei Zhang wrote:
> No WeakPtrs here.

Done.

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/tether/t...
chrome/browser/tether/tether_service.h:12: #include "build/build_config.h"
On 2017/04/19 19:13:17, Lei Zhang wrote:
> Not needed since nothing is being #ifdef'd? Ditto in other tether files.

Removed here and elsewhere.

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/tether/t...
chrome/browser/tether/tether_service.h:17: // namespace user_prefs {
On 2017/04/19 19:13:17, Lei Zhang wrote:
> I think you should uncomment this and remove the #include.

My bad, removed.

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/tether/t...
File chrome/browser/tether/tether_service_factory.cc (right):

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/tether/t...
chrome/browser/tether/tether_service_factory.cc:7: #include
"base/command_line.h"
On 2017/04/19 19:13:17, Lei Zhang wrote:
> Not used.

Done.

https://codereview.chromium.org/2819713002/diff/60001/chrome/common/pref_name...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/2819713002/diff/60001/chrome/common/pref_name...
chrome/common/pref_names.cc:973: // Whether a user is allowed to use Tether.
On 2017/04/19 19:13:17, Lei Zhang wrote:
> Can you mention it's a boolean pref like the surrounding comments?

Done.

[Lei Zhang, 2017-04-20 19:17:14]

Not sure why you are not running tryjobs. I will kick off a run for you.

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:3578: "tether/tether_service.cc",
On 2017/04/20 17:22:32, Ryan Hansberry wrote:
> On 2017/04/19 19:13:17, Lei Zhang wrote:
> > Is this going to build on Windows/Mac/Linux too? You should run some try
jobs
> > when you get closer to be able to submit.
> 
> This is only intended to run on ChromeOS.

So why is not in a ChromeOS only source list? (see line 3199 where this section
starts)

[Lei Zhang, 2017-04-20 19:17:23]

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

[commit-bot: I haz the power, 2017-04-20 19:18:12]

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

[Lei Zhang, 2017-04-20 19:24:30]

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:3578: "tether/tether_service.cc",
On 2017/04/20 17:22:32, Ryan Hansberry wrote:
> On 2017/04/19 19:13:17, Lei Zhang wrote:
> > Is this going to build on Windows/Mac/Linux too? You should run some try
jobs
> > when you get closer to be able to submit.
> 
> This is only intended to run on ChromeOS.

Another question - is this ever going to be used on Windows/Mac/Linux? If not,
you can leave it here, but rename the files to have the _chromeos postfix. e.g.
tether/tether_service_chromeos.cc. Then it won't be built on other platforms.

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/tether/t...
File chrome/browser/tether/tether_service.h (right):

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/tether/t...
chrome/browser/tether/tether_service.h:17: // namespace user_prefs {
On 2017/04/20 17:22:32, Ryan Hansberry wrote:
> On 2017/04/19 19:13:17, Lei Zhang wrote:
> > I think you should uncomment this and remove the #include.
> 
> My bad, removed.

Why not keep the forward decl and move the pref_registry_syncable.h #include
into the .cc file?
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

[Lei Zhang, 2017-04-20 19:26:21]

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/tether/O...
File chrome/browser/tether/OWNERS (right):

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/tether/O...
chrome/browser/tether/OWNERS:2: hansberry@chromium.org
On 2017/04/20 17:22:32, Ryan Hansberry wrote:
> On 2017/04/19 19:13:17, Lei Zhang wrote:
> > Do you have a bug component? If so, add a COMPONENT: line? See other OWNERS
> file
> > for examples.
> 
> We don't have a bug component. :/

Do you plan to? What component will people file bugs under? You can always add
COMPONENT: later.

[commit-bot: I haz the power, 2017-04-20 19:30:28]

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

[commit-bot: I haz the power, 2017-04-20 19:30:30]

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

[Ryan Hansberry, 2017-04-20 22:08:09]

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:3578: "tether/tether_service.cc",
On 2017/04/20 19:24:30, Lei Zhang wrote:
> On 2017/04/20 17:22:32, Ryan Hansberry wrote:
> > On 2017/04/19 19:13:17, Lei Zhang wrote:
> > > Is this going to build on Windows/Mac/Linux too? You should run some try
> jobs
> > > when you get closer to be able to submit.
> > 
> > This is only intended to run on ChromeOS.
> 
> Another question - is this ever going to be used on Windows/Mac/Linux? If not,
> you can leave it here, but rename the files to have the _chromeos postfix.
e.g.
> tether/tether_service_chromeos.cc. Then it won't be built on other platforms.

I've moved these under a is_chromeos source list. Let me know if that's
incorrect (And there are no plans to expand this to platforms other than
ChromeOS).

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/tether/O...
File chrome/browser/tether/OWNERS (right):

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/tether/O...
chrome/browser/tether/OWNERS:2: hansberry@chromium.org
On 2017/04/20 19:26:21, Lei Zhang wrote:
> On 2017/04/20 17:22:32, Ryan Hansberry wrote:
> > On 2017/04/19 19:13:17, Lei Zhang wrote:
> > > Do you have a bug component? If so, add a COMPONENT: line? See other
OWNERS
> > file
> > > for examples.
> > 
> > We don't have a bug component. :/
> 
> Do you plan to? What component will people file bugs under? You can always add
> COMPONENT: later.

Yes, we plan to. I will add a COMPONENT: line when we have one.

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/tether/t...
File chrome/browser/tether/tether_service.h (right):

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/tether/t...
chrome/browser/tether/tether_service.h:17: // namespace user_prefs {
On 2017/04/20 19:24:30, Lei Zhang wrote:
> On 2017/04/20 17:22:32, Ryan Hansberry wrote:
> > On 2017/04/19 19:13:17, Lei Zhang wrote:
> > > I think you should uncomment this and remove the #include.
> > 
> > My bad, removed.
> 
> Why not keep the forward decl and move the pref_registry_syncable.h #include
> into the .cc file?
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

Oops. Fixed.

[Ryan Hansberry, 2017-04-20 22:08:35]

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

[commit-bot: I haz the power, 2017-04-20 22:09:23]

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

[Lei Zhang, 2017-04-20 22:11:43]

Still need to upload a new patch set.

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:3578: "tether/tether_service.cc",
On 2017/04/20 22:08:09, Ryan Hansberry wrote:
> On 2017/04/20 19:24:30, Lei Zhang wrote:
> > On 2017/04/20 17:22:32, Ryan Hansberry wrote:
> > > On 2017/04/19 19:13:17, Lei Zhang wrote:
> > > > Is this going to build on Windows/Mac/Linux too? You should run some try
> > jobs
> > > > when you get closer to be able to submit.
> > > 
> > > This is only intended to run on ChromeOS.
> > 
> > Another question - is this ever going to be used on Windows/Mac/Linux? If
not,
> > you can leave it here, but rename the files to have the _chromeos postfix.
> e.g.
> > tether/tether_service_chromeos.cc. Then it won't be built on other
platforms.
> 
> I've moved these under a is_chromeos source list. Let me know if that's
> incorrect (And there are no plans to expand this to platforms other than
> ChromeOS).

Why not just rename the file so it is immediately obvious that the file is
platform specific?

[Lei Zhang, 2017-04-20 22:13:03]

On 2017/04/20 22:11:43, Lei Zhang wrote:
> Why not just rename the file so it is immediately obvious that the file is
> platform specific?

Another possibility is to move c/b/tether into chrome/browser/chromeos.

[commit-bot: I haz the power, 2017-04-20 22:18:34]

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

[commit-bot: I haz the power, 2017-04-20 22:18:35]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Ryan Hansberry, 2017-04-20 22:26:45]

Sorry, forgot to upload changelist.

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2819713002/diff/60001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:3578: "tether/tether_service.cc",
On 2017/04/20 22:11:43, Lei Zhang wrote:
> On 2017/04/20 22:08:09, Ryan Hansberry wrote:
> > On 2017/04/20 19:24:30, Lei Zhang wrote:
> > > On 2017/04/20 17:22:32, Ryan Hansberry wrote:
> > > > On 2017/04/19 19:13:17, Lei Zhang wrote:
> > > > > Is this going to build on Windows/Mac/Linux too? You should run some
try
> > > jobs
> > > > > when you get closer to be able to submit.
> > > > 
> > > > This is only intended to run on ChromeOS.
> > > 
> > > Another question - is this ever going to be used on Windows/Mac/Linux? If
> not,
> > > you can leave it here, but rename the files to have the _chromeos postfix.
> > e.g.
> > > tether/tether_service_chromeos.cc. Then it won't be built on other
> platforms.
> > 
> > I've moved these under a is_chromeos source list. Let me know if that's
> > incorrect (And there are no plans to expand this to platforms other than
> > ChromeOS).
> 
> Why not just rename the file so it is immediately obvious that the file is
> platform specific?

What's the preferred situation:

1) Place tether_service_* under the non-Android source set it was with the
suffix _chromeos
2) Place tether_service_* under this is_chromeos source set without the suffix
_chromeos
3) Place tether_service_* under this is_chromeos source set and use the suffix
_chromeos
4) Place tether_service_* files under chrome/browser/chromeos

?

[Lei Zhang, 2017-04-20 22:45:12]

On 2017/04/20 22:26:45, Ryan Hansberry wrote:
> What's the preferred situation:
> 
> 1) Place tether_service_* under the non-Android source set it was with the
> suffix _chromeos
> 2) Place tether_service_* under this is_chromeos source set without the suffix
> _chromeos
> 3) Place tether_service_* under this is_chromeos source set and use the suffix
> _chromeos
> 4) Place tether_service_* files under chrome/browser/chromeos

My vote is for (4) if chrome/browser/chromeos/OWNERS is happy with that.

[Ryan Hansberry, 2017-04-20 23:06:49]

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

[Ryan Hansberry, 2017-04-20 23:07:25]

I've now moved the TetherService and its factory under chrome/browser/chromeos.
Steven can you please take a look?

[Lei Zhang, 2017-04-21 04:32:00]

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

[commit-bot: I haz the power, 2017-04-21 04:32:28]

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

[Lei Zhang, 2017-04-21 05:23:25]

lgtm

[commit-bot: I haz the power, 2017-04-21 05:41:52]

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

[commit-bot: I haz the power, 2017-04-21 05:41:54]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2017-04-21 20:09:17]

lgtm w/ one suggestion

https://codereview.chromium.org/2819713002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/tether/tether_service.cc (right):

https://codereview.chromium.org/2819713002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/tether/tether_service.cc:84:
chromeos::switches::kEnableTether)) {
We should just add IsEnabled in this CL to avoid duplicate code.

[Ryan Hansberry, 2017-04-24 17:53:09]

https://codereview.chromium.org/2819713002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/tether/tether_service.cc (right):

https://codereview.chromium.org/2819713002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/tether/tether_service.cc:84:
chromeos::switches::kEnableTether)) {
On 2017/04/21 20:09:17, stevenjb wrote:
> We should just add IsEnabled in this CL to avoid duplicate code.

Done.

[Ryan Hansberry, 2017-04-24 17:53:19]

The CQ bit was checked by hansberry@chromium.org

[Ryan Hansberry, 2017-04-24 17:53:19]

The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org,
tengs@chromium.org, khorimoto@chromium.org, battre@chromium.org,
stevenjb@chromium.org, thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2819713002/#ps160001
(title: "Create an IsEnabled method on TetherService.")

[commit-bot: I haz the power, 2017-04-24 17:54:11]

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-24 18:58:52]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1493056399287510,
"parent_rev": "93bf8a7466ca3d2c6297f1e40e1f5be17c7b006a", "commit_rev":
"8d01f3ac9dad3f4833d868e253989ed88b705d1c"}

[commit-bot: I haz the power, 2017-04-24 18:59:08]

Description was changed from

==========
Create TetherService, a TetherAllowed pref, and use DependencyManager for
RegisterProfilePrefs calls.

TetherService dynamically starts and stops Tether when prefs::kTetherAllowed
changes value (from policy changing). This is expected from policy settings in
Chrome. Please see:
https://sites.google.com/a/chromium.org/dev/developers/how-tos/enterprise/add....

This CL also tweaks EasyUnlockServiceFactory and ChromeCryptAuthServiceFactory
to call RegisterProfilePrefs on their respective services. These calls are
automagically handled by DependencyManager: it ensures that the
RegisterProfilePrefs calls are made in the correct order and only once per
context.

A subsequent CL will come after this one that actually creates the enterprise
policy for Tether, and maps to prefs::kTetherAllowed.

BUG=710174
==========

to

==========
Create TetherService, a TetherAllowed pref, and use DependencyManager for
RegisterProfilePrefs calls.

TetherService dynamically starts and stops Tether when prefs::kTetherAllowed
changes value (from policy changing). This is expected from policy settings in
Chrome. Please see:
https://sites.google.com/a/chromium.org/dev/developers/how-tos/enterprise/add....

This CL also tweaks EasyUnlockServiceFactory and ChromeCryptAuthServiceFactory
to call RegisterProfilePrefs on their respective services. These calls are
automagically handled by DependencyManager: it ensures that the
RegisterProfilePrefs calls are made in the correct order and only once per
context.

A subsequent CL will come after this one that actually creates the enterprise
policy for Tether, and maps to prefs::kTetherAllowed.

BUG=710174

Review-Url: https://codereview.chromium.org/2819713002
Cr-Commit-Position: refs/heads/master@{#466702}
Committed:
https://chromium.googlesource.com/chromium/src/+/8d01f3ac9dad3f4833d868e25398...
==========

[commit-bot: I haz the power, 2017-04-24 18:59:10]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/8d01f3ac9dad3f4833d868e25398...