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

chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.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/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"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/common/pref_names.h"
 #include "components/prefs/pref_service.h"
 
 namespace chromeos {
 
 ArcTermsOfServiceScreen::ArcTermsOfServiceScreen(
     BaseScreenDelegate* base_screen_delegate,
     ArcTermsOfServiceScreenActor* actor)
     : 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() {
   if (!actor_)
     return;
 
   // Show the screen.
   actor_->Show();
 }
 
 void ArcTermsOfServiceScreen::Hide() {
   if (actor_)
     actor_->Hide();
 }
 
 std::string ArcTermsOfServiceScreen::GetName() const {
   return WizardController::kArcTermsOfServiceScreenName;
 }
 
 void ArcTermsOfServiceScreen::OnSkip() {
   ApplyTerms(false);
 }
 
 void ArcTermsOfServiceScreen::OnAccept() {
   ApplyTerms(true);
 }
 
 void ArcTermsOfServiceScreen::ApplyTerms(bool accepted) {
   Profile* profile = ProfileManager::GetActiveUserProfile();
   profile->GetPrefs()->SetBoolean(prefs::kArcTermsAccepted, accepted);

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

Looks a kind of race.
Because both ArcTermsOfServiceScreen and ArcTermsOfServiceOobeNegotiator
implement same observer, and there is no guarantee about the order. So, if the
negotiator's called first, and in its call stack kArcTermsAccepted is set to
false somehow, then this is called, it is wrongly set to true. Of course, if
this is called first, and in its call stack, kArcTermsAccepted is set to false,
in ArcSessionManager it is set to true again.

In that sense, IMHO, observing kArcTermsAccepted approach sounds better to me,
but I'm not strong opinion here as long as the race above is resolved cleanly.

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

There is no race condition here. ArcTermsOfServiceScreen sets observer first.
UserSessionManager::InitializeUserSession(). 
PS. This code would work even with race condition.

[hidehiko, 2016/12/19 15:49:44]

Ah, maybe "race" was not a good word choice. Sorry for that.
But I'm still worried about some points.

1) Depending on "ArcTermsOfServiceScreen callbacks is called earlier than
ArcTermsOfServiceOobeNegotiatorForManagedUser" sounds like much fragile to me.
It is because the registering order and the callback invocation order of
ObserverList look just implementation details.
2) Your code sets kArcTermsAccepted twice, which sounds a source of bug.
Actually, it is problematic in my scenario above.
3) Because ArcSessionManager and OOBE systems are designed to be independent, so
life cycle management look unclear to me.

So, if you continue to choose this new observer approach, please address those
three.
1) Get rid of depending the order of same observer callback invocation.
2) Do not set kArcTermsAccepted twice per user action.
3) Add clear and detailed documentation / comments about life cycle management
of two independent systems.

Also, please add explicit tests for these to guard from breakage.

Or, alternatively, I still think observing kArcTermsAccepted is an option. That
approach does not have those concerns, IIUC.
# Instead, it is probably necessary to document who sets the preference etc.

WDYT?

[khmel, 2016/12/19 17:48:10]

I don't see any problem in order which are they called. To test this I
explicitly changed the order of calling and it still works.
removed.
added comments in OOBE negotiator.
Sorry, I don't understand about which test are you talking. If this is OOBE test
for Arc pages, then there is TODO to implement such tests and this is big task
outside of this CL.

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

I meant a test for this specific implementation. In order to guarantee;
- kArcTermsAccepted is set only once.
- lifecycle of actor and negotiator instances.

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

Unit test that validates using OOBE negotiator already added.

-  profile->GetPrefs()->SetBoolean(prefs::kArcEnabled, accepted);
+  if (!profile->GetPrefs()->IsManagedPreference(prefs::kArcEnabled))

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

Is this check really needed?

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

This prevents calling redundant OnOptInProfileChanged

[hidehiko, 2016/12/19 15:49:44]

According to the current implementation and comments,
it looks designed to be called only when the value is actually changed.
If not, it sounds like a bug? Who actually calls it?
# I wish it were explicitly documented...

cf) 
https://cs.chromium.org/chromium/src/components/prefs/json_pref_store.cc?sq=p...
https://cs.chromium.org/chromium/src/components/prefs/in_memory_pref_store.cc...
https://cs.chromium.org/chromium/src/services/preferences/public/cpp/pref_obs...

and so on.

[khmel, 2016/12/19 17:48:10]

I have no idea at this moment who calls this method. I think this is definitely
out of scope of this CL. Even so changing this is very risky in terms of whole
Chrome project. FYI, we already use this approach here:
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_session_...

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

Even if there is similar code, code change for the unknown case is very danger.
Could you investigate and identify the cause, first?

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

I think investigation of this conceptual problem is outside of this CL. Please
also see ref in my comment above that we have code that already worked with this
approach.

+    profile->GetPrefs()->SetBoolean(prefs::kArcEnabled, accepted);
 
   Finish(BaseScreenDelegate::ARC_TERMS_OF_SERVICE_FINISHED);
 }
 
 void ArcTermsOfServiceScreen::OnActorDestroyed(
     ArcTermsOfServiceScreenActor* actor) {
   DCHECK_EQ(actor, actor_);
   actor_ = nullptr;
 }
 
 }  // namespace chromeos