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"
@@ skipping 60 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 IsFirstRunForUser() {

[hidehiko, 2016/12/09 06:59:57]

The name looks not reflecting what this does, because the name does not implies
the OOBE. Could you rename?
E.g.; IsFirstRunForOobeEnabledUser() etc.?

[khmel, 2016/12/14 01:27:59]

Nice name, thanks!

+  // Arc OOBE OptIn is optional for now. Test if it exists and current user is

[hidehiko, 2016/12/09 06:59:57]

s/Arc/ARC/ for all comments in the CL, including CL description.

[khmel, 2016/12/14 01:27:59]

Done.

+  // 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), weak_ptr_factory_(this) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(!g_arc_session_manager);
   g_arc_session_manager = this;
 
   arc_bridge_service()->AddObserver(this);
 }
@@ skipping 505 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();
+    // In case this is after OOBE, don't show the initial progress.
+    if (!IsFirstRunForUser())

[xiyuan, 2016/12/09 00:01:45]

This changes the existing behavior. 

For OOBE OptIn, we will not show ARC loading from 2nd login and onward.

For non OOBE optin, since we don't have kEnableArcOOBEOptIn switch, we will
never show ARC loading.

Is this right?

[hidehiko, 2016/12/09 06:59:57]

I'm confused.

IIUC;
OnPrimaryUserProfilePrepared is run before OOBE, you said.
Then, this should be always false, otherwise this means we run ARC before ToS
agreement on OOBE?

[khmel, 2016/12/14 01:27:59]

ARC OptIn OOBE is shown only for first time user. In case kEnableArcOOBEOptIn is
not set, IsFirstRunForUser is always false and this is legacy case. Anyway I
drop this line as described in comment above.

[khmel, 2016/12/14 01:27:59]

That is good point and in theory it is true :), however Chrome  sends us
OnOptInPreferenceChanged notification even if set Arc.Enabled managed pref to
true. Resulting state of Arc.Enabled is not changed but notification is issued.
That is quite confusing. Anyway I discard this code for more clear logic and
avoid settings arc.enabled pref for managed ARC.

+      support_host_->ShowArcLoading();
     StartArcAndroidManagementCheck();
     return;
   }
 
-  // Need user's explicit Terms Of Service agreement.
-  StartTermsOfServiceNegotiation();
+  // Need user's explicit Terms Of Service agreement. In case Arc is managed and
+  // forced to on, check if this is first time run. First time run is covered by
+  // OOBE OptIn. In last case observe terms accepted change in order to start
+  // Arc when required.
+  if (IsArcManaged() && IsFirstRunForUser()) {

[hidehiko, 2016/12/09 06:59:57]

This condition looks very complicated to me.
Could you postpone running this method until OOBE ToS accepting condition meets,
even on managed case, instead?

[khmel, 2016/12/14 01:27:59]

I changed code to negotiator based solution. So this one is deprecated.

+    pref_change_registrar_.Add(

[xiyuan, 2016/12/09 00:01:45]

nit: move into StartTermsOfServiceNegotiation() since OOBE ToS is conceptually a
ToS negotiation too.

[khmel, 2016/12/14 01:27:59]

That is right, Done.

+        prefs::kArcTermsAccepted,
+        base::Bind(&ArcSessionManager::OnTermsAcceptedPreferenceChanged,
+                   weak_ptr_factory_.GetWeakPtr()));
+  } else {
+    StartTermsOfServiceNegotiation();
+  }
+}
+
+void ArcSessionManager::OnTermsAcceptedPreferenceChanged() {
+  if (profile_->GetPrefs()->GetBoolean(prefs::kArcTermsAccepted))

[xiyuan, 2016/12/09 00:01:45]

Why do we need to bail and keep observing kArcTermsAccepted?

Also, I am wondering that maybe we can chain this with
OnTermsOfServiceNegotiated() instead of OnOptInPreferenceChanged ?

[khmel, 2016/12/14 01:27:59]

Agree, this is misprint. However it used to work in my tests because
OnOptInPreferenceChanged was triggered even if I set managed Arc.enabled.

+    return;
+  pref_change_registrar_.Remove(prefs::kArcTermsAccepted);
+
+  DCHECK(IsArcManaged() && IsArcEnabled());

[hidehiko, 2016/12/09 06:59:57]

nit: please split DCHECK().
DCHECK(IsArcManaged());
DCHECK(IsArcEnabled());

[khmel, 2016/12/14 01:27:59]

This code is discarded.

+  OnOptInPreferenceChanged();
 }
 
 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 151 matching lines @@
   profile_->GetPrefs()->SetBoolean(prefs::kArcTermsAccepted, true);
 
   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 &&

[hidehiko, 2016/12/09 06:59:57]

Good catch! Thanks for fixing.

[khmel, 2016/12/14 01:27:59]

Acknowledged.

+          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 120 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