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 f3ab8b7a2911ee590d2191efb52f09589700c5d4..e1219dcbacb7e286d20d19e99a6355a69b1967cc 100644
--- a/chrome/browser/chromeos/arc/arc_session_manager.cc
+++ b/chrome/browser/chromeos/arc/arc_session_manager.cc
@@ -426,7 +426,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_);
@@ -448,8 +448,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();
@@ -953,51 +953,27 @@ 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::OnOptInAborted() {
   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_);

[hidehiko, 2017/05/15 09:05:44]

Could you verify that this is not a part of auth flow case, too?

[victorhsieh0, 2017/05/15 22:31:53]

Any suggestion on how to check this?  Auth is mostly not involved in ASM now,
except perhaps the final onProvisioningFinished.

[hidehiko, 2017/05/25 12:05:52]

Three scenarios, I'm worried about.
1)
- Enable ARC with manual auth flag.
- And let manual auth fail in ArcSupport app. Then click retry.

2)
- Enable ARC with manual auth flag.
- Let auth fail in ARC container.
- Then click retry.

3)
- Enable ARC, and wait for PlayStore. Maybe install random app.
- Disable ARC, which should trigger data removal.
- Immediately after disabling (before actual stop), re-enable ARC.
- I think you'd see error page (This behavior itself is a bug, and WIP, though).
Then click retry.

[victorhsieh0, 2017/05/26 19:38:41]

Done.  It's a bit hacky but please let me know if you see a better way.

[hidehiko, 2017/05/31 17:17:01]

Thank you for manual check.
Could you double check if third case is covered.

In that case, error is reported L665 at latest PS. So, tos negotiator is not
created yet, and this function is called, IIUC.
Then, there is no way to restart ToS negotiation?

[victorhsieh0, 2017/05/31 20:49:28]

I see the problem now.  How would you suggest to fix this?  I see your TODO at
L665.  Is it ok to also check enable_requests_ at L665 as a workaround, until
you remove L665?

  if (!arc_session_runner_->IsStopped() && !enable_requested_) {

Or we may have to add back the origianl prefs::kArcTermsAccepted condition, and
not restart the instance as a special case.

[hidehiko, 2017/06/01 16:25:58]

I think kArcTermsAccepted sounds safer.

[victorhsieh0, 2017/06/01 21:27:23]

Done.  Reverted some changes here.  Also added a TODO to you as we may be able
to remove this case later, but let me know if it's not the case.

 
   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()) {

[hidehiko, 2017/05/15 09:05:44]

The condition looks negated?

Unfortunately, it is difficult to make testing automated now, but could you
kindly test the scenarios manually, too? (And update TEST= line in
description?).

[victorhsieh0, 2017/05/15 22:31:53]

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();
   }
 }