Refactor ArcAndroidManagementChecker.

chrome/browser/chromeos/arc/policy/arc_android_management_checker.cc

Index: chrome/browser/chromeos/arc/policy/arc_android_management_checker.cc
diff --git a/chrome/browser/chromeos/arc/policy/arc_android_management_checker.cc b/chrome/browser/chromeos/arc/policy/arc_android_management_checker.cc
index 1d50dc68d67bad37b488b9e1eaed8d2c50a3d6c6..a645f53bb2d06d5cc89be4cca167a6fcb326ba20 100644
--- a/chrome/browser/chromeos/arc/policy/arc_android_management_checker.cc
+++ b/chrome/browser/chromeos/arc/policy/arc_android_management_checker.cc
@@ -5,11 +5,14 @@
 #include "chrome/browser/chromeos/arc/policy/arc_android_management_checker.h"
 
 #include "base/bind.h"
+#include "base/callback_helpers.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/browser_process_platform_part.h"
-#include "chrome/browser/chromeos/arc/policy/arc_android_management_checker_delegate.h"
+#include "chrome/browser/chromeos/arc/policy/arc_policy_util.h"
 #include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
+#include "chrome/browser/profiles/profile.h"
+#include "components/policy/core/browser/browser_policy_connector.h"
 #include "components/policy/core/common/cloud/device_management_service.h"
 #include "components/signin/core/browser/profile_oauth2_token_service.h"
 
@@ -29,27 +32,20 @@ policy::DeviceManagementService* GetDeviceManagementService() {
 }  // namespace
 
 ArcAndroidManagementChecker::ArcAndroidManagementChecker(
-    ArcAndroidManagementCheckerDelegate* delegate,
+    Profile* profile,
     ProfileOAuth2TokenService* token_service,
     const std::string& account_id,
-    bool background_mode)
-    : delegate_(delegate),
+    bool retry_on_error)
+    : profile_(profile),
       token_service_(token_service),
       account_id_(account_id),
-      background_mode_(background_mode),
+      retry_on_error_(retry_on_error),
       retry_time_ms_(kRetryTimeMinMs),
       android_management_client_(GetDeviceManagementService(),
                                  g_browser_process->system_request_context(),
                                  account_id,
                                  token_service),
-      weak_ptr_factory_(this) {
-  if (token_service_->RefreshTokenIsAvailable(account_id_)) {
-    StartCheck();
-  } else {
-    DCHECK(background_mode_);
-    token_service_->AddObserver(this);
-  }
-}
+      weak_ptr_factory_(this) {}
 
 ArcAndroidManagementChecker::~ArcAndroidManagementChecker() {
   token_service_->RemoveObserver(this);
@@ -60,6 +56,35 @@ void ArcAndroidManagementChecker::StartClient() {
   GetDeviceManagementService()->ScheduleInitialization(0);
 }
 
+void ArcAndroidManagementChecker::StartCheck(const CheckCallback& callback) {
+  DCHECK(callback_.is_null());

[khmel, 2016/10/25 14:55:20]

Now your logic is incorrect, before all checks were started in CTOR and this
guarantied that you cannot reactivate the check. With this StartCheck this is no
longer true. Based on this structure you may call StartCheck many times. 
Potentially you may have android_management_client_.StartCheckAndroidManagement
already started before next call of StartCheck. 

Assume you are existing earlier on line 67.
In this case you have to stop all previous check. Otherwise check may be
triggered in wrong time for another check.

I understand that in reality this should never happen however based on code it
is not clear.

[hidehiko, 2016/10/25 16:15:20]

This is common pattern used in Chrome, AFAIK.

Actually, starting the check looks not a part of initialization of the checker.
Considering that even fail-able initialization is discouraged to do in the ctor,
I think we should not start it in the ctor.

cf) https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors

"Ensuring not calling StartCheck while there is inflight operation" is the
caller's responsibility. Theoretically we can have more complicated mechanism to
support such cases, but I cannot find any benefit on it for this specific case,
because it is not used.
Sorry, I couldn't get your point. Are you worried about the case that a client
wants to start another StartCheck while there is inflight operation?
Then, it is supported. Remove the instance, create another instance and call
StartCheck().
Removing the instance cancels inflight operations.
Both the responsibility of the caller (guarantee at most one inflight
operation), and the cancel behavior are explicitly documented in StartCheck().

[khmel, 2016/10/25 16:18:03]

Acknowledged.

+
+  // Do not send requests for Chrome OS managed users, nor for well-known
+  // consumer domains.
+  if (policy_util::IsAccountManaged(profile_) ||
+      policy::BrowserPolicyConnector::IsNonEnterpriseUser(
+          profile_->GetProfileUserName())) {
+    callback.Run(policy::AndroidManagementClient::Result::RESULT_UNMANAGED);
+    return;
+  }
+
+  callback_ = callback;
+  EnsureRefreshTokenLoaded();
+}
+
+void ArcAndroidManagementChecker::EnsureRefreshTokenLoaded() {
+  if (token_service_->RefreshTokenIsAvailable(account_id_)) {
+    // If the refresh token is already available, just start the management
+    // check immediately.
+    StartCheckInternal();
+    return;
+  }
+
+  // Set the observer to the token service so the callback will be called
+  // when the token is loaded.
+  token_service_->AddObserver(this);
+}
+
 void ArcAndroidManagementChecker::OnRefreshTokenAvailable(
     const std::string& account_id) {
   if (account_id != account_id_)
@@ -69,14 +94,16 @@ void ArcAndroidManagementChecker::OnRefreshTokenAvailable(
 
 void ArcAndroidManagementChecker::OnRefreshTokensLoaded() {
   token_service_->RemoveObserver(this);
-  StartCheck();
+  StartCheckInternal();
 }
 
-void ArcAndroidManagementChecker::StartCheck() {
+void ArcAndroidManagementChecker::StartCheckInternal() {
+  DCHECK(!callback_.is_null());
+
   if (!token_service_->RefreshTokenIsAvailable(account_id_)) {
     VLOG(2) << "No refresh token is available for android management check.";
-    OnAndroidManagementChecked(
-        policy::AndroidManagementClient::Result::RESULT_ERROR);
+    base::ResetAndReturn(&callback_)
+        .Run(policy::AndroidManagementClient::Result::RESULT_ERROR);
     return;
   }
 
@@ -86,39 +113,30 @@ void ArcAndroidManagementChecker::StartCheck() {
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
-void ArcAndroidManagementChecker::ScheduleCheck() {
-  DCHECK(background_mode_);
-
-  VLOG(2) << "Schedule next android management  check in " << retry_time_ms_
-          << " ms.";
-
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE, base::Bind(&ArcAndroidManagementChecker::StartCheck,
-                            weak_ptr_factory_.GetWeakPtr()),
-      base::TimeDelta::FromMilliseconds(retry_time_ms_));
-  retry_time_ms_ *= 2;
-  if (retry_time_ms_ > kRetryTimeMaxMs)
-    retry_time_ms_ = kRetryTimeMaxMs;
-}
-
-void ArcAndroidManagementChecker::DispatchResult(
-    policy::AndroidManagementClient::Result result) {
-  DCHECK(delegate_);
-  delegate_->OnAndroidManagementChecked(result);
-}
-
 void ArcAndroidManagementChecker::OnAndroidManagementChecked(
     policy::AndroidManagementClient::Result result) {
+  DCHECK(!callback_.is_null());
   VLOG(2) << "Android management check done " << result << ".";
-  if (background_mode_ &&
+  if (retry_on_error_ &&
       result == policy::AndroidManagementClient::Result::RESULT_ERROR) {
-    ScheduleCheck();
+    ScheduleRetry();
     return;
   }
 
-  base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::Bind(&ArcAndroidManagementChecker::DispatchResult,
-                            weak_ptr_factory_.GetWeakPtr(), result));
+  base::ResetAndReturn(&callback_).Run(result);
+}
+
+void ArcAndroidManagementChecker::ScheduleRetry() {
+  DCHECK(retry_on_error_);
+  DCHECK(!callback_.is_null());
+  VLOG(2) << "Schedule next android management check in " << retry_time_ms_
+          << " ms.";
+
+  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE, base::Bind(&ArcAndroidManagementChecker::StartCheckInternal,
+                            weak_ptr_factory_.GetWeakPtr()),
+      base::TimeDelta::FromMilliseconds(retry_time_ms_));
+  retry_time_ms_ = std::min(retry_time_ms_ * 2, kRetryTimeMaxMs);
 }
 
 }  // namespace arc