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

chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc

Index: chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc
diff --git a/chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc b/chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc
index 1da3072521b8c3086275456054483620c1aaf631..e13b550a4d83b1f32066894f151e8f5d61dd8c63 100644
--- a/chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc
+++ b/chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.h"
 
+#include "chrome/browser/chromeos/login/screens/arc_terms_of_service_screen_actor.h"
 #include "chrome/browser/chromeos/login/screens/base_screen_delegate.h"
 #include "chrome/browser/chromeos/login/wizard_controller.h"
 #include "chrome/browser/metrics/metrics_reporting_state.h"
@@ -20,12 +21,12 @@ ArcTermsOfServiceScreen::ArcTermsOfServiceScreen(
     : BaseScreen(base_screen_delegate), actor_(actor) {
   DCHECK(actor_);
   if (actor_)
-    actor_->SetDelegate(this);
+    actor_->AddObserver(this);
 }
 
 ArcTermsOfServiceScreen::~ArcTermsOfServiceScreen() {
   if (actor_)
-    actor_->SetDelegate(nullptr);
+    actor_->RemoveObserver(this);
 }
 
 void ArcTermsOfServiceScreen::Show() {
@@ -55,8 +56,8 @@ void ArcTermsOfServiceScreen::OnAccept() {
 
 void ArcTermsOfServiceScreen::ApplyTerms(bool accepted) {
   Profile* profile = ProfileManager::GetActiveUserProfile();
-  profile->GetPrefs()->SetBoolean(prefs::kArcTermsAccepted, accepted);
-  profile->GetPrefs()->SetBoolean(prefs::kArcEnabled, accepted);
+  if (!profile->GetPrefs()->IsManagedPreference(prefs::kArcEnabled))

[hidehiko, 2016/12/20 15:20:45]

IIUC, kArcTermsAccepted must be set here, otherwise non managed user case gets
broken.

If it is set here, to avoid two-times-setting for managed user case,
it is necessary to do it in ARC side.

Considering the consistency, we should have kArcTermsAccepted set to true before
starting management state check in ArcSessionManager. (Maybe we should have
DCHECK).

For this perspective, ArcTermsOfServiceScreen's callback needs to be called
before ARC's. So this introduces implicit dependency (observer callback order)
between independent modules, which should be avoided.

At the moment, I do not have a clean approach to address this.
Note that, one hacky option is to add the observer to the actor in the
OnOptInPrefChanged() call stack. However, this also introduces another implicit
complexity and dependency, which should be avoided, too.

So, proposal: considering we already have many iterations, but it looks not yet
clear about how to resolve the above cleanly, could you move back to your
original approach, observing pref, in this CL for now, to focus on fixing the
current user visible problem (= not to show ToS Opt-In page for OOBE managed
user case)?
Later, once the problem above gets resolved with this observer approach, let's
refactor it.

WDYT?

[khmel, 2016/12/20 16:27:58]

FYI, kArcTermsAccepted is set in
voidrcSessionManager::OnTermsOfServiceNegotiated(bool accepted) {
And yes, I tested this code and managed case is not broken in real life.

Observer approach has problem, if user presses 'Skip' kArcTermsAccepted is not
set and pref should not be called, right? Which event is useful for this case?
In this CL you request me to investigate why pref is called if we don't change
actual value but here you request me to rely on this? I don't want to implement
code that relies on signaling that pref was changed from "false default"
to "false". 

+    profile->GetPrefs()->SetBoolean(prefs::kArcEnabled, accepted);
 
   Finish(BaseScreenDelegate::ARC_TERMS_OF_SERVICE_FINISHED);
 }