Turn ArcSupportHost from Observer model to Delegate

chrome/browser/chromeos/arc/arc_session_manager.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/arc/arc_session_manager.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
@@ skipping 408 matching lines @@
   // TODO(hidehiko): Revisit to think about lazy initialization.
   //
   // Don't show UI for ARC Kiosk because the only one UI in kiosk mode must
   // be the kiosk app. In case of error the UI will be useless as well, because
   // in typical use case there will be no one nearby the kiosk device, who can
   // do some action to solve the problem be means of UI.
   if (!g_disable_ui_for_testing && !IsArcOptInVerificationDisabled() &&
       !IsArcKioskMode()) {
     DCHECK(!support_host_);
     support_host_ = base::MakeUnique<ArcSupportHost>(profile_);
-    support_host_->AddObserver(this);
+    support_host_->SetErrorDelegate(this);
   }
 
   DCHECK_EQ(State::NOT_INITIALIZED, state_);
   SetState(State::STOPPED);
 
   context_ = base::MakeUnique<ArcAuthContext>(profile_);
 
   if (!g_disable_ui_for_testing ||
       g_enable_check_android_management_for_testing) {
     ArcAndroidManagementChecker::StartClient();
   }
 
   // Chrome may be shut down before completing ARC data removal.
   // For such a case, start removing the data now, if necessary.
   MaybeStartArcDataRemoval();
 }
 
 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();
   profile_ = nullptr;
   SetState(State::NOT_INITIALIZED);
   if (scoped_opt_in_tracker_) {
     scoped_opt_in_tracker_->TrackShutdown();
     scoped_opt_in_tracker_.reset();
   }
 }
@@ skipping 483 matching lines @@
     return;
   }
 
   // Restart ARC anyway. Let the enterprise reporting instance decide whether
   // the ARC user data wipe is still required or not.
   reenable_arc_ = false;
   VLOG(1) << "Reenable ARC";
   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();
   }
 }
 
 void ArcSessionManager::OnSendFeedbackClicked() {
   DCHECK(support_host_);
   chrome::OpenFeedbackDialog(nullptr, chrome::kFeedbackSourceArcApp);
 }
 
 void ArcSessionManager::SetArcSessionRunnerForTesting(
@@ skipping 38 matching lines @@
 
 #undef MAP_STATE
 
   // Some compilers report an error even if all values of an enum-class are
   // covered exhaustively in a switch statement.
   NOTREACHED() << "Invalid value " << static_cast<int>(state);
   return os;
 }
 
 }  // namespace arc