arc: ARC loading progress should not be shown when started from OOBE.

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 "ash/common/shelf/shelf_delegate.h"
 #include "ash/common/wm_shell.h"
 #include "base/bind.h"
 #include "base/callback_helpers.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/strings/string16.h"
 #include "base/time/time.h"
 #include "chrome/browser/chromeos/arc/arc_auth_context.h"
 #include "chrome/browser/chromeos/arc/arc_optin_uma.h"
 #include "chrome/browser/chromeos/arc/arc_support_host.h"
-#include "chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h"
+#include "chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.h"
+#include "chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h"
 #include "chrome/browser/chromeos/arc/policy/arc_android_management_checker.h"
 #include "chrome/browser/chromeos/arc/policy/arc_policy_util.h"
 #include "chrome/browser/chromeos/login/user_flow.h"
 #include "chrome/browser/chromeos/login/users/chrome_user_manager.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
 #include "chrome/browser/policy/profile_policy_connector.h"
 #include "chrome/browser/policy/profile_policy_connector_factory.h"
 #include "chrome/browser/prefs/pref_service_syncable_util.h"
 #include "chrome/browser/profiles/profile.h"
@@ skipping 40 matching lines @@
 ash::ShelfDelegate* GetShelfDelegate() {
   if (g_shelf_delegate_for_testing)
     return g_shelf_delegate_for_testing;
   if (ash::WmShell::HasInstance()) {
     DCHECK(ash::WmShell::Get()->shelf_delegate());
     return ash::WmShell::Get()->shelf_delegate();
   }
   return nullptr;
 }
 
+bool IsFirstRunForOobeEnabledUser() {

[hidehiko, 2016/12/16 05:31:50]

Could you comment "FirstRun" is the Chrome term meaning the first login session,
and *not* meaning this is ARC's first run?

[khmel, 2016/12/16 18:37:55]

Done.

+  // ARC OOBE OptIn is optional for now. Test if it exists and current user is
+  // first time user.
+  return user_manager::UserManager::Get()->IsCurrentUserNew() &&
+         base::CommandLine::ForCurrentProcess()->HasSwitch(
+             chromeos::switches::kEnableArcOOBEOptIn);
+}
+
 }  // namespace
 
 ArcSessionManager::ArcSessionManager(ArcBridgeService* bridge_service)
     : ArcService(bridge_service),
       attempt_user_exit_callback_(base::Bind(chrome::AttemptUserExit)),
       weak_ptr_factory_(this) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(!g_arc_session_manager);
   g_arc_session_manager = this;
 
@@ skipping 173 matching lines @@
   // 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";
   EnableArc();
 }
 
 void ArcSessionManager::OnProvisioningFinished(ProvisioningResult result) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  // Due asynchronous nature of stopping Arc bridge, OnProvisioningFinished may
+  // Due asynchronous nature of stopping ARC bridge, OnProvisioningFinished may
   // arrive after setting the |State::STOPPED| state and |State::Active| is not
   // guaranty set here. prefs::kArcDataRemoveRequested is also can be active
   // for now.
 
   if (provisioning_reported_) {
     // We don't expect ProvisioningResult::SUCCESS is reported twice or reported
     // after an error.
     DCHECK_NE(result, ProvisioningResult::SUCCESS);
     // TODO (khmel): Consider changing LOG to NOTREACHED once we guaranty that
     // no double message can happen in production.
@@ skipping 154 matching lines @@
 
   PrefServiceSyncableFromProfile(profile_)->AddSyncedPrefObserver(
       prefs::kArcEnabled, this);
 
   context_.reset(new ArcAuthContext(profile_));
 
   if (!g_disable_ui_for_testing ||
       g_enable_check_android_management_for_testing) {
     ArcAndroidManagementChecker::StartClient();
   }
+
   pref_change_registrar_.Init(profile_->GetPrefs());
   pref_change_registrar_.Add(
       prefs::kArcEnabled,
       base::Bind(&ArcSessionManager::OnOptInPreferenceChanged,
                  weak_ptr_factory_.GetWeakPtr()));
   if (profile_->GetPrefs()->GetBoolean(prefs::kArcEnabled)) {
+    if (IsFirstRunForOobeEnabledUser() && IsArcManaged())
+      StartTermsOfServiceNegotiation(NegotiatorType::OOBE);

[hidehiko, 2016/12/16 05:31:50]

I still do not want to have this function here.
So, could you move this into OnOptInPrefChanged() as follows?

void OnPrimaryUserProfilePrepared() {
  :
  s/OnOptInPrefChanged()/OnOptInPrefChanged(true /* is initial run */)/
  :
}

void OnOptInPrefChanged() {
  OnOptInPrefChangedInternal(false /* is initial run */);
}

// Welcome better naming :-)
void OnOptInPrefChangedInternal(bool is_initial_run) {
  :  // almost current code.
  StartTermsOfServiceNegotiation(is_initial_run);
}

void StartTermsOfServiceNegotiation(bool is_initial_run) {
  :  // check ABS running status.
  if (is_initial_run && IsFirstRunForOobeEnabledUser() && IsManaged()) {
    ... = base::MakeUnique<ArcTosManagedOobeNegotiator(...);
  } else if (arc_support_) {
    ... = base::MakeUnique<ArcTosDefaultNegotiator>(...);
  }

  if (negotiator)
    ...->StartNegotiation(...);
}

All other OnOptInPrefChanged() call can be OnOptInPrefChangedInternal(false);

I skipped to comment for the code which relates to this.

[khmel, 2016/12/16 18:37:55]

Done.

+
     // Don't start ARC if there is a pending request to remove the data. Restart
     // ARC once data removal finishes.
     if (profile_->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested)) {
       reenable_arc_ = true;
       RemoveArcData();
     } else {
       OnOptInPreferenceChanged();
     }
   } else {
     RemoveArcData();
@@ skipping 78 matching lines @@
     reenable_arc_ = false;
     StopArc();
     RemoveArcData();
     return;
   }
 
   if (state_ == State::ACTIVE)
     return;
 
   if (state_ == State::REMOVING_DATA_DIR) {
-    // Data removal request is in progress. Set flag to re-enable Arc once it is
+    // Data removal request is in progress. Set flag to re-enable ARC once it is
     // finished.
     reenable_arc_ = true;
     return;
   }
 
   if (support_host_)
     support_host_->SetArcManaged(IsArcManaged());
 
   // For ARC Kiosk we skip ToS because it is very likely that near the device
   // there will be no one who is eligible to accept them.
@@ skipping 34 matching lines @@
   }
 
   // If it is marked that the Terms of service is accepted already,
   // just skip the negotiation with user, and start Android management
   // check directly.
   // This happens, e.g., when;
   // 1) User accepted the Terms of service on OOBE flow.
   // 2) User accepted the Terms of service on Opt-in flow, but logged out
   //   before ARC sign in procedure was done. Then, logs in again.
   if (profile_->GetPrefs()->GetBoolean(prefs::kArcTermsAccepted)) {
-    support_host_->ShowArcLoading();
     StartArcAndroidManagementCheck();
     return;
   }
 
-  // Need user's explicit Terms Of Service agreement.
-  StartTermsOfServiceNegotiation();
+  // Need user's explicit Terms Of Service agreement. There is the case when Arc
+  // is managed on and we have OOBE negotiator already set.
+  if (!terms_of_service_negotiator_)
+    StartTermsOfServiceNegotiation(NegotiatorType::DEFAULT);
 }
 
 void ArcSessionManager::ShutdownBridge() {
   arc_sign_in_timer_.Stop();
   playstore_launcher_.reset();
   terms_of_service_negotiator_.reset();
   android_management_checker_.reset();
   arc_bridge_service()->RequestStop();
   if (state_ != State::NOT_INITIALIZED && state_ != State::REMOVING_DATA_DIR)
     SetState(State::STOPPED);
@@ skipping 16 matching lines @@
 void ArcSessionManager::StopAndEnableArc() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(!arc_bridge_service()->stopped());
   reenable_arc_ = true;
   StopArc();
 }
 
 void ArcSessionManager::StartArc() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  // Arc must be started only if no pending data removal request exists.
+  // ARC must be started only if no pending data removal request exists.
   DCHECK(!profile_->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested));
 
   provisioning_reported_ = false;
 
   arc_bridge_service()->RequestStart();
   SetState(State::ACTIVE);
 }
 
 void ArcSessionManager::OnArcSignInTimeout() {
   LOG(ERROR) << "Timed out waiting for first sign in.";
@@ skipping 67 matching lines @@
   profile_->GetPrefs()->SetBoolean(prefs::kArcEnabled, false);
 }
 
 void ArcSessionManager::RecordArcState() {
   // Only record Enabled state if ARC is allowed in the first place, so we do
   // not split the ARC population by devices that cannot run ARC.
   if (IsAllowed())
     UpdateEnabledStateUMA(IsArcEnabled());
 }
 
-void ArcSessionManager::StartTermsOfServiceNegotiation() {
+void ArcSessionManager::StartTermsOfServiceNegotiation(NegotiatorType type) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(!terms_of_service_negotiator_);
 
   if (!arc_bridge_service()->stopped()) {
     // If the user attempts to re-enable ARC while the bridge is still running
     // the user should not be able to continue until the bridge has stopped.
     if (support_host_) {
       support_host_->ShowError(
           ArcSupportHost::Error::SIGN_IN_SERVICE_UNAVAILABLE_ERROR, false);
     }
     return;
   }
 
   SetState(State::SHOWING_TERMS_OF_SERVICE);
   if (support_host_) {
-    terms_of_service_negotiator_ =
-        base::MakeUnique<ArcTermsOfServiceNegotiator>(profile_->GetPrefs(),
-                                                      support_host_.get());
+    switch (type) {
+      case NegotiatorType::DEFAULT:
+        terms_of_service_negotiator_ =
+            base::MakeUnique<ArcTermsOfServiceDefaultNegotiator>(
+                profile_->GetPrefs(), support_host_.get());
+        break;
+      case NegotiatorType::OOBE:
+        terms_of_service_negotiator_ =
+            base::MakeUnique<ArcTermsOfServiceOOBENegotiator>();
+        break;
+      default:
+        NOTREACHED();
+        return;
+    }
     terms_of_service_negotiator_->StartNegotiation(
         base::Bind(&ArcSessionManager::OnTermsOfServiceNegotiated,
                    weak_ptr_factory_.GetWeakPtr()));
   }
 }
 
 void ArcSessionManager::OnTermsOfServiceNegotiated(bool accepted) {
   DCHECK(terms_of_service_negotiator_);
   terms_of_service_negotiator_.reset();
 
   if (!accepted) {
     // To cancel, user needs to close the window. Note that clicking "Cancel"
     // button effectively just closes the window.
     CancelAuthCode();
     return;
   }
 
   // Terms were accepted.
   profile_->GetPrefs()->SetBoolean(prefs::kArcTermsAccepted, true);
 
-  support_host_->ShowArcLoading();
+  // Don't show UI for this progress if it was not shown.
+  if (support_host_->has_message_host())
+    support_host_->ShowArcLoading();
   StartArcAndroidManagementCheck();
 }
 
 void ArcSessionManager::StartArcAndroidManagementCheck() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(arc_bridge_service()->stopped());
   DCHECK(state_ == State::SHOWING_TERMS_OF_SERVICE ||
-         state_ == State::CHECKING_ANDROID_MANAGEMENT);
+         state_ == State::CHECKING_ANDROID_MANAGEMENT ||
+         (state_ == State::STOPPED &&
+          profile_->GetPrefs()->GetBoolean(prefs::kArcTermsAccepted)));
   SetState(State::CHECKING_ANDROID_MANAGEMENT);
 
   android_management_checker_.reset(new ArcAndroidManagementChecker(
       profile_, context_->token_service(), context_->account_id(),
       false /* retry_on_error */));
   android_management_checker_->StartCheck(
       base::Bind(&ArcSessionManager::OnAndroidManagementChecked,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
@@ skipping 69 matching lines @@
 void ArcSessionManager::OnRetryClicked() {
   DCHECK(support_host_);
 
   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)) {
-    StartTermsOfServiceNegotiation();
+    StartTermsOfServiceNegotiation(NegotiatorType::DEFAULT);
   } else if (support_host_->ui_page() == ArcSupportHost::UIPage::ERROR &&
              !arc_bridge_service()->stopped()) {
     // 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.
     DCHECK_EQ(State::ACTIVE, state_);
     support_host_->ShowArcLoading();
     ShutdownBridge();
     reenable_arc_ = true;
   } else if (state_ == State::ACTIVE) {
@@ skipping 36 matching lines @@
       return os << "ACTIVE";
   }
 
   // Some compiler reports an error even if all values of an enum-class are
   // covered indivisually in a switch statement.
   NOTREACHED();
   return os;
 }
 
 }  // namespace arc