Turn ArcSupportHost from Observer model to Delegate

chrome/browser/chromeos/arc/arc_session_manager.cc

Index: chrome/browser/chromeos/arc/arc_session_manager.cc
diff --git a/chrome/browser/chromeos/arc/arc_session_manager.cc b/chrome/browser/chromeos/arc/arc_session_manager.cc
index 237e553a6bdf1b34559365d5201af5f83ef2616a..c74a33704d8e63f647cac39123ac03ff37f37875 100644
--- a/chrome/browser/chromeos/arc/arc_session_manager.cc
+++ b/chrome/browser/chromeos/arc/arc_session_manager.cc
@@ -16,6 +16,7 @@
 #include "base/time/time.h"
 #include "chrome/browser/chromeos/arc/arc_auth_context.h"
 #include "chrome/browser/chromeos/arc/arc_auth_notification.h"
+#include "chrome/browser/chromeos/arc/arc_auth_service.h"
 #include "chrome/browser/chromeos/arc/arc_migration_guide_notification.h"
 #include "chrome/browser/chromeos/arc/arc_optin_uma.h"
 #include "chrome/browser/chromeos/arc/arc_support_host.h"
@@ -416,7 +417,7 @@ void ArcSessionManager::SetProfile(Profile* profile) {
       !IsArcKioskMode()) {
     DCHECK(!support_host_);
     support_host_ = base::MakeUnique<ArcSupportHost>(profile_);
-    support_host_->AddObserver(this);
+    support_host_->SetErrorDelegate(this);
   }
 
   DCHECK_EQ(State::NOT_INITIALIZED, state_);
@@ -438,8 +439,8 @@ void ArcSessionManager::Shutdown() {
   enable_requested_ = false;
   ShutdownSession();
   if (support_host_) {
+    support_host_->SetErrorDelegate(nullptr);
     support_host_->Close();
-    support_host_->RemoveObserver(this);
     support_host_.reset();
   }
   context_.reset();
@@ -954,51 +955,28 @@ void ArcSessionManager::MaybeReenableArc() {
   RequestEnableImpl();
 }
 
-void ArcSessionManager::OnWindowClosed() {
-  DCHECK(support_host_);
-  if (terms_of_service_negotiator_) {
-    // In this case, ArcTermsOfServiceNegotiator should handle the case.
-    // Do nothing.
-    return;
-  }
+void ArcSessionManager::OnWindowClosedTermsAccepted() {
   CancelAuthCode();
 }
 
-void ArcSessionManager::OnTermsAgreed(bool is_metrics_enabled,
-                                      bool is_backup_and_restore_enabled,
-                                      bool is_location_service_enabled) {
-  DCHECK(support_host_);
-  DCHECK(terms_of_service_negotiator_);
-  // This should be handled in ArcTermsOfServiceNegotiator. Do nothing here.
-}
-
 void ArcSessionManager::OnRetryClicked() {
   DCHECK(support_host_);
+  DCHECK_EQ(support_host_->ui_page(), ArcSupportHost::UIPage::ERROR);
+  DCHECK(!terms_of_service_negotiator_);
+  DCHECK(!support_host_->HasAuthDelegateForTest());

[Luis Héctor Chávez, 2017/05/30 16:06:27]

you shouldn't use ...ForTest() methods outside of tests. either move this DCHECK
to ArcSupportHost or rename the method to HasAuthDelegate().

[victorhsieh0, 2017/05/30 19:52:42]

Done.

 
   UpdateOptInActionUMA(OptInActionType::RETRY);
 
-  // TODO(hidehiko): Simplify the retry logic.
-  if (terms_of_service_negotiator_) {
-    // Currently Terms of service is shown. ArcTermsOfServiceNegotiator should
-    // handle this.
-  } else if (!profile_->GetPrefs()->GetBoolean(prefs::kArcTermsAccepted)) {
-    MaybeStartTermsOfServiceNegotiation();
-  } else if (support_host_->ui_page() == ArcSupportHost::UIPage::ERROR &&
-             !arc_session_runner_->IsStopped()) {
-    // ERROR_WITH_FEEDBACK is set in OnSignInFailed(). In the case, stopping
-    // ARC was postponed to contain its internal state into the report.
-    // Here, on retry, stop it, then restart.
+  if (!arc_session_runner_->IsStopped()) {

[Luis Héctor Chávez, 2017/05/30 16:06:27]

nit: maybe invert the order so that the positive case comes before.

[victorhsieh0, 2017/05/30 19:52:42]

Done.

+    // ARC may be kept alive for the user to send feedback during some opt-in
+    // failure, in order to include ARC's internal state into the report.  Here,
+    // on retry, stop it, then restart.
     DCHECK_EQ(State::ACTIVE, state_);
     support_host_->ShowArcLoading();
     ShutdownSession();
     reenable_arc_ = true;
-  } else if (state_ == State::ACTIVE) {
-    // This case is handled in ArcAuthService.
-    // Do nothing.
   } else {
-    // Otherwise, we restart ARC. Note: this is the first boot case.
-    // For second or later boot, either ERROR_WITH_FEEDBACK case or ACTIVE
-    // case must hit.
+    // Otherwise, we simply start ARC again.
     StartAndroidManagementCheck();
   }
 }