ChromeCryptAuthService: only perform device sync once enrollment is complete.

chrome/browser/cryptauth/chrome_cryptauth_service.cc

Index: chrome/browser/cryptauth/chrome_cryptauth_service.cc
diff --git a/chrome/browser/cryptauth/chrome_cryptauth_service.cc b/chrome/browser/cryptauth/chrome_cryptauth_service.cc
index cb91f83f7a8b8bd94c372ba785ffd595f578a532..373985c0dc5a2e031b2571d38cec557781653754 100644
--- a/chrome/browser/cryptauth/chrome_cryptauth_service.cc
+++ b/chrome/browser/cryptauth/chrome_cryptauth_service.cc
@@ -21,7 +21,6 @@
 #include "components/cryptauth/cryptauth_device_manager.h"
 #include "components/cryptauth/cryptauth_enroller.h"
 #include "components/cryptauth/cryptauth_enroller_impl.h"
-#include "components/cryptauth/cryptauth_enrollment_manager.h"
 #include "components/cryptauth/cryptauth_enrollment_utils.h"
 #include "components/cryptauth/cryptauth_gcm_manager_impl.h"
 #include "components/cryptauth/proto/cryptauth_api.pb.h"
@@ -226,8 +225,7 @@ ChromeCryptAuthService::ChromeCryptAuthService(
                  << "waiting before starting CryptAuth managers.";
     token_service_->AddObserver(this);
   } else {
-    enrollment_manager_->Start();
-    device_manager_->Start();
+    PerformEnrollmentAndDeviceSync();
   }
 }
 
@@ -268,11 +266,30 @@ ChromeCryptAuthService::CreateCryptAuthClientFactory() {
   return CreateCryptAuthClientFactoryImpl(profile_);
 }
 
+void ChromeCryptAuthService::OnEnrollmentStarted() {}
+
+void ChromeCryptAuthService::OnEnrollmentFinished(bool success) {

[khorimoto, 2017/05/30 21:38:18]

If !success, the device manager won't actually work, right? Should we be
retrying the enrollment attempt here?

[Tim Song, 2017/05/31 00:04:21]

The enrollment manager should have its own retry logic, so we don't need to
retry the enrollment.

However, I agree that we should check that success == true.

[Ryan Hansberry, 2017/05/31 17:08:09]

Done.

+  device_manager_->Start();
+  enrollment_manager_->RemoveObserver(this);
+}
+
 void ChromeCryptAuthService::OnRefreshTokenAvailable(
     const std::string& account_id) {
+  PA_LOG(ERROR) << "ChromeCAS: OnRefreshTokenAvailable";

[khorimoto, 2017/05/30 21:38:18]

Remove these logs.

[Ryan Hansberry, 2017/05/31 17:08:09]

Oops, removed.

   if (account_id == GetAccountId()) {
+    PA_LOG(ERROR) << "ChromeCAS: OnRefreshTokenAvailable starting managers";
     token_service_->RemoveObserver(this);
-    enrollment_manager_->Start();
-    device_manager_->Start();
+    PerformEnrollmentAndDeviceSync();
   }
 }
+
+void ChromeCryptAuthService::PerformEnrollmentAndDeviceSync() {
+  // If enrollment is not valid, wait for the new enrollment attempt to finish

[khorimoto, 2017/05/30 21:38:18]

nit: This comment should probably be moved to the else block.

[Ryan Hansberry, 2017/05/31 17:08:09]

Done.

+  // before starting CryptAuthDeviceManager. See OnEnrollmentFinished(),
+  if (enrollment_manager_->IsEnrollmentValid())
+    device_manager_->Start();
+  else
+    enrollment_manager_->AddObserver(this);
+
+  enrollment_manager_->Start();

[khorimoto, 2017/05/30 21:38:18]

Do we still need to call Start() here if the enrollment is already valid above?

[Ryan Hansberry, 2017/05/31 17:08:09]

Yes, in order for EnrollmentManager to schedule its next enrollment.

[khorimoto, 2017/05/31 17:53:38]

nit: Thanks for the explanation - please add a comment explaining why we need to
Start() regardless.

[Ryan Hansberry, 2017/05/31 18:14:32]

Done.

+}