[CrOS Tether] Fill out the Initializer class. Tether will now initialize fully once the flag is ena…

[CrOS Tether] Fill out the Initializer class. Tether will now initialize fully once the flag is enabled.

This CL also reworks a few of the constructors of related classes and modifies tests accordingly.

BUG=672263
Review-Url: https://codereview.chromium.org/2801353002
Cr-Commit-Position: refs/heads/master@{#463868}
Committed: https://chromium.googlesource.com/chromium/src/+/8e3c185effa25c01193d44193fd661d5a88d56fc

[Kyle Horimoto, 2017-04-07 18:34:16]

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

[Kyle Horimoto, 2017-04-07 18:34:17]

stevenjb@ for //chrome/browser change.
hansberry@ for the rest.

[Ryan Hansberry, 2017-04-07 18:53:41]

https://codereview.chromium.org/2801353002/diff/1/chromeos/components/tether/...
File chromeos/components/tether/initializer.cc (right):

https://codereview.chromium.org/2801353002/diff/1/chromeos/components/tether/...
chromeos/components/tether/initializer.cc:59:
device::BluetoothAdapterFactory::GetAdapter(base::Bind(
We unfortunately need one more callback here :) :/

The refresh token may not be available yet, and thus the CryptAuth*Managers
won;t be started yet. So, we need to listen on ProfileOAuth2TokenService and
wait for a OnRefreshTokensLoaded callback. You'll need to make Initializer
implement OAuth2TokenService::Observer and its method OnRefreshTokensLoaded.

You should inject a ProfileOAuth2TokenService into Initialize() (you get one
with ProfileOAuth2TokenServiceFactory::GetForProfile()), and then add instance_
as an observer of the injected token_service.

Once OnRefreshTokensLoaded is called, then you should call
BluetoothAdapterFactory::GetAdapter().

https://codereview.chromium.org/2801353002/diff/1/chromeos/components/tether/...
File chromeos/components/tether/local_device_data_provider.h (right):

https://codereview.chromium.org/2801353002/diff/1/chromeos/components/tether/...
chromeos/components/tether/local_device_data_provider.h:51: const
cryptauth::CryptAuthEnrollmentManager* cryptauth_enrollment_manager_;
Woo so much simpler :D

https://codereview.chromium.org/2801353002/diff/1/chromeos/components/tether/...
File chromeos/components/tether/tether_host_fetcher.cc (right):

https://codereview.chromium.org/2801353002/diff/1/chromeos/components/tether/...
chromeos/components/tether/tether_host_fetcher.cc:45: const std::string&
user_id,
Can you instead use a FakeCryptAuthService and then just use the single param
constructor?

[Ryan Hansberry, 2017-04-07 18:53:41]

https://codereview.chromium.org/2801353002/diff/1/chromeos/components/tether/...
File chromeos/components/tether/initializer.cc (right):

https://codereview.chromium.org/2801353002/diff/1/chromeos/components/tether/...
chromeos/components/tether/initializer.cc:59:
device::BluetoothAdapterFactory::GetAdapter(base::Bind(
We unfortunately need one more callback here :) :/

The refresh token may not be available yet, and thus the CryptAuth*Managers
won;t be started yet. So, we need to listen on ProfileOAuth2TokenService and
wait for a OnRefreshTokensLoaded callback. You'll need to make Initializer
implement OAuth2TokenService::Observer and its method OnRefreshTokensLoaded.

You should inject a ProfileOAuth2TokenService into Initialize() (you get one
with ProfileOAuth2TokenServiceFactory::GetForProfile()), and then add instance_
as an observer of the injected token_service.

Once OnRefreshTokensLoaded is called, then you should call
BluetoothAdapterFactory::GetAdapter().

https://codereview.chromium.org/2801353002/diff/1/chromeos/components/tether/...
File chromeos/components/tether/local_device_data_provider.h (right):

https://codereview.chromium.org/2801353002/diff/1/chromeos/components/tether/...
chromeos/components/tether/local_device_data_provider.h:51: const
cryptauth::CryptAuthEnrollmentManager* cryptauth_enrollment_manager_;
Woo so much simpler :D

https://codereview.chromium.org/2801353002/diff/1/chromeos/components/tether/...
File chromeos/components/tether/tether_host_fetcher.cc (right):

https://codereview.chromium.org/2801353002/diff/1/chromeos/components/tether/...
chromeos/components/tether/tether_host_fetcher.cc:45: const std::string&
user_id,
Can you instead use a FakeCryptAuthService and then just use the single param
constructor?

[Ryan Hansberry, 2017-04-07 23:08:55]

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

https://codereview.chromium.org/2801353002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/user_session_manager.cc:1260:
base::MakeUnique<TetherNotificationPresenter>(), profile->GetPrefs());
tether::TetherNotificationPresenter

https://codereview.chromium.org/2801353002/diff/20001/chromeos/components/tet...
File chromeos/components/tether/tether_host_fetcher.cc (right):

https://codereview.chromium.org/2801353002/diff/20001/chromeos/components/tet...
chromeos/components/tether/tether_host_fetcher.cc:39:
cryptauth_service_->GetCryptAuthEnrollmentManager()
Oops, this should be cryptauth_service. cryptauth_service_ causes a segfault :)

[Kyle Horimoto, 2017-04-07 23:33:09]

khorimoto@chromium.org changed reviewers:
+ msarda@chromium.org

[Kyle Horimoto, 2017-04-07 23:33:11]

+msarda as a reviewer for the added DEP

https://codereview.chromium.org/2801353002/diff/1/chromeos/components/tether/...
File chromeos/components/tether/initializer.cc (right):

https://codereview.chromium.org/2801353002/diff/1/chromeos/components/tether/...
chromeos/components/tether/initializer.cc:59:
device::BluetoothAdapterFactory::GetAdapter(base::Bind(
On 2017/04/07 18:53:40, Ryan Hansberry wrote:
> We unfortunately need one more callback here :) :/
> 
> The refresh token may not be available yet, and thus the CryptAuth*Managers
> won;t be started yet. So, we need to listen on ProfileOAuth2TokenService and
> wait for a OnRefreshTokensLoaded callback. You'll need to make Initializer
> implement OAuth2TokenService::Observer and its method OnRefreshTokensLoaded.
> 
> You should inject a ProfileOAuth2TokenService into Initialize() (you get one
> with ProfileOAuth2TokenServiceFactory::GetForProfile()), and then add
instance_
> as an observer of the injected token_service.
> 
> Once OnRefreshTokensLoaded is called, then you should call
> BluetoothAdapterFactory::GetAdapter().

Done.

https://codereview.chromium.org/2801353002/diff/1/chromeos/components/tether/...
File chromeos/components/tether/local_device_data_provider.h (right):

https://codereview.chromium.org/2801353002/diff/1/chromeos/components/tether/...
chromeos/components/tether/local_device_data_provider.h:51: const
cryptauth::CryptAuthEnrollmentManager* cryptauth_enrollment_manager_;
On 2017/04/07 18:53:40, Ryan Hansberry wrote:
> Woo so much simpler :D

Yep! This refactor is especially awesome for this class.

https://codereview.chromium.org/2801353002/diff/1/chromeos/components/tether/...
File chromeos/components/tether/tether_host_fetcher.cc (right):

https://codereview.chromium.org/2801353002/diff/1/chromeos/components/tether/...
chromeos/components/tether/tether_host_fetcher.cc:45: const std::string&
user_id,
On 2017/04/07 18:53:40, Ryan Hansberry wrote:
> Can you instead use a FakeCryptAuthService and then just use the single param
> constructor?

I'd like to keep it as-is because it makes the test set-up easier.

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

https://codereview.chromium.org/2801353002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/user_session_manager.cc:1260:
base::MakeUnique<TetherNotificationPresenter>(), profile->GetPrefs());
On 2017/04/07 23:08:55, Ryan Hansberry wrote:
> tether::TetherNotificationPresenter

Done.

https://codereview.chromium.org/2801353002/diff/20001/chromeos/components/tet...
File chromeos/components/tether/tether_host_fetcher.cc (right):

https://codereview.chromium.org/2801353002/diff/20001/chromeos/components/tet...
chromeos/components/tether/tether_host_fetcher.cc:39:
cryptauth_service_->GetCryptAuthEnrollmentManager()
On 2017/04/07 23:08:55, Ryan Hansberry wrote:
> Oops, this should be cryptauth_service. cryptauth_service_ causes a segfault
:)

Done.

[Ryan Hansberry, 2017-04-08 00:10:14]

lgtm with nits! :D

https://codereview.chromium.org/2801353002/diff/40001/chromeos/components/tet...
File chromeos/components/tether/initializer.h (right):

https://codereview.chromium.org/2801353002/diff/40001/chromeos/components/tet...
chromeos/components/tether/initializer.h:13: #include
"components/prefs/pref_registry_simple.h"
Remove this.

https://codereview.chromium.org/2801353002/diff/40001/chromeos/components/tet...
chromeos/components/tether/initializer.h:50: static void
RegisterProfilePrefs(PrefRegistrySimple* registry);
remove this.

[Kyle Horimoto, 2017-04-08 00:30:28]

https://codereview.chromium.org/2801353002/diff/40001/chromeos/components/tet...
File chromeos/components/tether/initializer.h (right):

https://codereview.chromium.org/2801353002/diff/40001/chromeos/components/tet...
chromeos/components/tether/initializer.h:13: #include
"components/prefs/pref_registry_simple.h"
On 2017/04/08 00:10:14, Ryan Hansberry wrote:
> Remove this.

Done.

https://codereview.chromium.org/2801353002/diff/40001/chromeos/components/tet...
chromeos/components/tether/initializer.h:50: static void
RegisterProfilePrefs(PrefRegistrySimple* registry);
On 2017/04/08 00:10:14, Ryan Hansberry wrote:
> remove this.

Done.

[Kyle Horimoto, 2017-04-10 17:27:08]

khorimoto@chromium.org changed reviewers:
+ rogerta@chromium.org
- msarda@chromium.org

[Kyle Horimoto, 2017-04-10 17:27:08]

Removed msarda; added rogerta instead.

[Roger Tawa OOO till Jul 10th, 2017-04-10 18:44:19]

signin DEPS lgtm

[stevenjb, 2017-04-10 20:26:59]

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

https://codereview.chromium.org/2801353002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/user_session_manager.cc:1261:
base::MakeUnique<tether::TetherNotificationPresenter>(),
See my comment in https://codereview.chromium.org/2805393002/. Also, this is a
little bit subtle. I would prefer to see:

auto presenter = base::MakeUnique<tether::TetherNotificationPresenter>(
  message_center::MessageCenter::Get(),
  NetworkConnect::Get());

chromeos::tether::Initializer::Initialize(..., std::move(presenter), ...);

Also, please ensure that UserSessionManager is destroyed *before*
NetworkHandler::Shutdown is called here:

https://cs.chromium.org/chromium/src/chrome/browser/chromeos/chrome_browser_m...

*And* before MessageCenter::Shutdown:

https://cs.chromium.org/chromium/src/chrome/browser/browser_process_impl.cc?t...

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
File chromeos/components/tether/host_scanner.h (right):

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
chromeos/components/tether/host_scanner.h:61: NotificationPresenter*
notification_presenter);
Don't do this. Multiple constructors are messy and should generally be avoided.
Also, as mentioned, this obscures lifetime issues.

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
File chromeos/components/tether/initializer.cc (right):

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
chromeos/components/tether/initializer.cc:75: }
nit: invert, early exit, and share common code:

if (not ready) {
  log and add observer;
  return;
}
GetBluetoothAdatper();

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
chromeos/components/tether/initializer.cc:92:
token_service_->RemoveObserver(this);
}

void GetBluetoothAdapter() {

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
chromeos/components/tether/initializer.cc:131:
base::TimeDelta::FromMilliseconds(100),
Move these constants and the comment to the top of the file.

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
File chromeos/components/tether/initializer.h (right):

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
chromeos/components/tether/initializer.h:47: ProfileOAuth2TokenService*
token_service);
As mentioned before, it is important to pass NHS and MC (and any other global
singletons) to this class explicitly to make the lifetime requirements clear.

Also, having an 'Initialize' method on an 'Initialize' class is just mean. Maybe
rename this 'InitSingleton'? (Or rename this class... I know it's namespaced and
all, but for searching I am personally a fan of more descriptive class names,
e.g. tether::TetherController).

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
File chromeos/components/tether/local_device_data_provider.h (right):

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
chromeos/components/tether/local_device_data_provider.h:45:
cryptauth_enrollment_manager);
Avoid this pattern. It's harder to read and check dependencies.

If you are doing this for testing, allow |cryptauth_service| to be null in the
constructor (and initialize the members to nullptr), and add a separate
InitializeForTest method:

provider_ = new LocalDeviceDataProvider(nullptr);
privider_->InitializeForTest(test_device_manager_, test_enrollment_manager_);

and

LocalDeviceDataProvider::LocalDeviceDataProvider(cryptauth_service) {
  if (cryptauth_service)
    Init(cryptauth_service->device..., cryptauth_service->enroll...);
}

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
File chromeos/components/tether/local_device_data_provider_unittest.cc (right):

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
chromeos/components/tether/local_device_data_provider_unittest.cc:53:
cryptauth::FakeCryptAuthGCMManager* fake_cryptauth_gcm_manager)
explicit

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
chromeos/components/tether/local_device_data_provider_unittest.cc:54: :
cryptauth::CryptAuthEnrollmentManager(nullptr,
comment what each of these are e.g.
nullptr /* clock */,

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
File chromeos/components/tether/tether_host_fetcher.h (right):

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
chromeos/components/tether/tether_host_fetcher.h:67:
cryptauth::CryptAuthDeviceManager* device_manager);
one constructor

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
File chromeos/components/tether/wifi_hotspot_connector.h (right):

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
chromeos/components/tether/wifi_hotspot_connector.h:48: NetworkConnect*
network_connect);
One constructor

[Kyle Horimoto, 2017-04-11 01:40:40]

I also added a test for Initializer which just initializes everything and
destroys the object. This will ensure that anything added to Initializer in the
future cannot be destroyed in the wrong order.

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

https://codereview.chromium.org/2801353002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/user_session_manager.cc:1261:
base::MakeUnique<tether::TetherNotificationPresenter>(),
On 2017/04/10 20:26:59, stevenjb wrote:
> See my comment in https://codereview.chromium.org/2805393002/. Also, this is a
> little bit subtle. I would prefer to see:
> 
> auto presenter = base::MakeUnique<tether::TetherNotificationPresenter>(
>   message_center::MessageCenter::Get(),
>   NetworkConnect::Get());
> 
> chromeos::tether::Initializer::Initialize(..., std::move(presenter), ...);
> 
> Also, please ensure that UserSessionManager is destroyed *before*
> NetworkHandler::Shutdown is called here:
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/chrome_browser_m...
> 
> *And* before MessageCenter::Shutdown:
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/browser_process_impl.cc?t...

Done.

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
File chromeos/components/tether/host_scanner.h (right):

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
chromeos/components/tether/host_scanner.h:61: NotificationPresenter*
notification_presenter);
On 2017/04/10 20:26:59, stevenjb wrote:
> Don't do this. Multiple constructors are messy and should generally be
avoided.
> Also, as mentioned, this obscures lifetime issues.

Done.

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
File chromeos/components/tether/initializer.cc (right):

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
chromeos/components/tether/initializer.cc:75: }
On 2017/04/10 20:26:59, stevenjb wrote:
> nit: invert, early exit, and share common code:
> 
> if (not ready) {
>   log and add observer;
>   return;
> }
> GetBluetoothAdatper();

Done.

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
chromeos/components/tether/initializer.cc:92:
token_service_->RemoveObserver(this);
On 2017/04/10 20:26:59, stevenjb wrote:
> }
> 
> void GetBluetoothAdapter() {

Done.

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
chromeos/components/tether/initializer.cc:131:
base::TimeDelta::FromMilliseconds(100),
On 2017/04/10 20:26:59, stevenjb wrote:
> Move these constants and the comment to the top of the file.

Done.

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
File chromeos/components/tether/initializer.h (right):

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
chromeos/components/tether/initializer.h:47: ProfileOAuth2TokenService*
token_service);
On 2017/04/10 20:26:59, stevenjb wrote:
> As mentioned before, it is important to pass NHS and MC (and any other global
> singletons) to this class explicitly to make the lifetime requirements clear.
> 
> Also, having an 'Initialize' method on an 'Initialize' class is just mean.
Maybe
> rename this 'InitSingleton'? (Or rename this class... I know it's namespaced
and
> all, but for searching I am personally a fan of more descriptive class names,
> e.g. tether::TetherController).

Done.

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
File chromeos/components/tether/local_device_data_provider.h (right):

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
chromeos/components/tether/local_device_data_provider.h:45:
cryptauth_enrollment_manager);
On 2017/04/10 20:26:59, stevenjb wrote:
> Avoid this pattern. It's harder to read and check dependencies.
> 
> If you are doing this for testing, allow |cryptauth_service| to be null in the
> constructor (and initialize the members to nullptr), and add a separate
> InitializeForTest method:
> 
> provider_ = new LocalDeviceDataProvider(nullptr);
> privider_->InitializeForTest(test_device_manager_, test_enrollment_manager_);
> 
> and
> 
> LocalDeviceDataProvider::LocalDeviceDataProvider(cryptauth_service) {
>   if (cryptauth_service)
>     Init(cryptauth_service->device..., cryptauth_service->enroll...);
> }

Done.

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
File chromeos/components/tether/local_device_data_provider_unittest.cc (right):

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
chromeos/components/tether/local_device_data_provider_unittest.cc:53:
cryptauth::FakeCryptAuthGCMManager* fake_cryptauth_gcm_manager)
On 2017/04/10 20:26:59, stevenjb wrote:
> explicit

Done.

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
chromeos/components/tether/local_device_data_provider_unittest.cc:54: :
cryptauth::CryptAuthEnrollmentManager(nullptr,
On 2017/04/10 20:26:59, stevenjb wrote:
> comment what each of these are e.g.
> nullptr /* clock */,

Done.

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
File chromeos/components/tether/tether_host_fetcher.h (right):

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
chromeos/components/tether/tether_host_fetcher.h:67:
cryptauth::CryptAuthDeviceManager* device_manager);
On 2017/04/10 20:26:59, stevenjb wrote:
> one constructor

Done.

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
File chromeos/components/tether/wifi_hotspot_connector.h (right):

https://codereview.chromium.org/2801353002/diff/60001/chromeos/components/tet...
chromeos/components/tether/wifi_hotspot_connector.h:48: NetworkConnect*
network_connect);
On 2017/04/10 20:26:59, stevenjb wrote:
> One constructor

Done.

[Ryan Hansberry, 2017-04-11 15:48:40]

lgtm again, with a nit

https://codereview.chromium.org/2801353002/diff/80001/chromeos/components/tet...
File chromeos/components/tether/initializer.h (right):

https://codereview.chromium.org/2801353002/diff/80001/chromeos/components/tet...
chromeos/components/tether/initializer.h:81: void
IntervalErrorCallback(device::BluetoothAdvertisement::ErrorCode status);
Please change the name of this to reflect the new name
OnBluetoothAdapterAdvertisingIntervalSet, e.g.
OnBluetoothAdapterAdvertisingIntervalError

[Kyle Horimoto, 2017-04-11 16:28:14]

https://codereview.chromium.org/2801353002/diff/80001/chromeos/components/tet...
File chromeos/components/tether/initializer.h (right):

https://codereview.chromium.org/2801353002/diff/80001/chromeos/components/tet...
chromeos/components/tether/initializer.h:81: void
IntervalErrorCallback(device::BluetoothAdvertisement::ErrorCode status);
On 2017/04/11 15:48:40, Ryan Hansberry wrote:
> Please change the name of this to reflect the new name
> OnBluetoothAdapterAdvertisingIntervalSet, e.g.
> OnBluetoothAdapterAdvertisingIntervalError

Done.

[stevenjb, 2017-04-11 19:52:01]

Thanks, mostly nits.

https://codereview.chromium.org/2801353002/diff/100001/chromeos/components/te...
File chromeos/components/tether/initializer.cc (right):

https://codereview.chromium.org/2801353002/diff/100001/chromeos/components/te...
chromeos/components/tether/initializer.cc:39: int64_t
kMaxAdvertisingIntervalMilliseconds = 100;
constexpr

https://codereview.chromium.org/2801353002/diff/100001/chromeos/components/te...
chromeos/components/tether/initializer.cc:61: // Tether should only be
initialized once.
Maybe log an error here (or just DCHECK)

https://codereview.chromium.org/2801353002/diff/100001/chromeos/components/te...
chromeos/components/tether/initializer.cc:117: tether_host_fetcher_.reset();
We shouldn't need to explicitly reset these in the destructor, they will be
destroyed in the reverse order they are listed in the class.

https://codereview.chromium.org/2801353002/diff/100001/chromeos/components/te...
File chromeos/components/tether/initializer.h (right):

https://codereview.chromium.org/2801353002/diff/100001/chromeos/components/te...
chromeos/components/tether/initializer.h:48: static void Start(
Ugh, Start is too overloaded. How about just 'Init'? It is at least more
obviously different from Initializer than Initialize?

https://codereview.chromium.org/2801353002/diff/100001/chromeos/components/te...
File chromeos/components/tether/mock_local_device_data_provider.cc (right):

https://codereview.chromium.org/2801353002/diff/100001/chromeos/components/te...
chromeos/components/tether/mock_local_device_data_provider.cc:16: :
LocalDeviceDataProvider(nullptr) {}
nit: /* comment */ what nullptr is

[Kyle Horimoto, 2017-04-11 21:43:20]

https://codereview.chromium.org/2801353002/diff/100001/chromeos/components/te...
File chromeos/components/tether/initializer.cc (right):

https://codereview.chromium.org/2801353002/diff/100001/chromeos/components/te...
chromeos/components/tether/initializer.cc:39: int64_t
kMaxAdvertisingIntervalMilliseconds = 100;
On 2017/04/11 19:52:00, stevenjb wrote:
> constexpr

Done.

https://codereview.chromium.org/2801353002/diff/100001/chromeos/components/te...
chromeos/components/tether/initializer.cc:61: // Tether should only be
initialized once.
On 2017/04/11 19:52:00, stevenjb wrote:
> Maybe log an error here (or just DCHECK)

Done.

https://codereview.chromium.org/2801353002/diff/100001/chromeos/components/te...
chromeos/components/tether/initializer.cc:117: tether_host_fetcher_.reset();
On 2017/04/11 19:52:00, stevenjb wrote:
> We shouldn't need to explicitly reset these in the destructor, they will be
> destroyed in the reverse order they are listed in the class.

Done.

https://codereview.chromium.org/2801353002/diff/100001/chromeos/components/te...
File chromeos/components/tether/initializer.h (right):

https://codereview.chromium.org/2801353002/diff/100001/chromeos/components/te...
chromeos/components/tether/initializer.h:48: static void Start(
On 2017/04/11 19:52:00, stevenjb wrote:
> Ugh, Start is too overloaded. How about just 'Init'? It is at least more
> obviously different from Initializer than Initialize?

Done.

[stevenjb, 2017-04-11 22:30:57]

lgtm

[Kyle Horimoto, 2017-04-11 22:47:42]

The CQ bit was checked by khorimoto@chromium.org

[Kyle Horimoto, 2017-04-11 22:47:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
rogerta@chromium.org, hansberry@chromium.org, stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2801353002/#ps140001
(title: "Fix TetherNotificationPresenter constructor.")

[commit-bot: I haz the power, 2017-04-11 22:48:27]

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-11 23:04:12]

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

[commit-bot: I haz the power, 2017-04-11 23:04:13]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  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_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Kyle Horimoto, 2017-04-11 23:13:45]

The CQ bit was checked by khorimoto@chromium.org

[Kyle Horimoto, 2017-04-11 23:13:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, rogerta@chromium.org, hansberry@chromium.org
Link to the patchset: https://codereview.chromium.org/2801353002/#ps160001
(title: "Added missing dep.")

[commit-bot: I haz the power, 2017-04-11 23:14:22]

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 01:06:36]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1491952425471040,
"parent_rev": "317fc233486ef485d879bd5b05b041ab292ed02a", "commit_rev":
"8e3c185effa25c01193d44193fd661d5a88d56fc"}

[commit-bot: I haz the power, 2017-04-12 01:07:34]

Description was changed from

==========
[CrOS Tether] Fill out the Initializer class. Tether will now initialize fully
once the flag is enabled.

This CL also reworks a few of the constructors of related classes and modifies
tests accordingly.

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

to

==========
[CrOS Tether] Fill out the Initializer class. Tether will now initialize fully
once the flag is enabled.

This CL also reworks a few of the constructors of related classes and modifies
tests accordingly.

BUG=672263

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

[commit-bot: I haz the power, 2017-04-12 01:07:36]

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