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

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 6107f7ff95a49ec38d9202f69016dcf119eb30e8..fae851765b07640c4882676b43348b9f6524610a 100644
--- a/chrome/browser/chromeos/arc/arc_session_manager.cc
+++ b/chrome/browser/chromeos/arc/arc_session_manager.cc
@@ -78,6 +78,14 @@ ash::ShelfDelegate* GetShelfDelegate() {
   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)
@@ -603,13 +611,34 @@ void ArcSessionManager::OnOptInPreferenceChanged() {
   // 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() {
@@ -781,7 +810,9 @@ 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(