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

chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator_for_managed_user.cc

Index: chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator_for_managed_user.cc
diff --git a/chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator_for_managed_user.cc b/chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator_for_managed_user.cc
new file mode 100644
index 0000000000000000000000000000000000000000..1f6fb88bd87a0b5653c18312d411cae873845c6e
--- /dev/null
+++ b/chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator_for_managed_user.cc
@@ -0,0 +1,80 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.

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

Could you add unittest for this class, too? (Sorry, for some reason, this was in
my draft...)

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

I already added unit test that validates using 
ArcTermsOfServiceInitialOobeNegotiatorForManagedUser

+// 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/optin/arc_terms_of_service_oobe_negotiator_for_managed_user.h"
+
+#include "base/bind.h"
+#include "chrome/browser/chromeos/login/screens/arc_terms_of_service_screen_actor.h"
+#include "chrome/browser/chromeos/login/ui/login_display_host.h"
+#include "chrome/browser/ui/webui/chromeos/login/oobe_ui.h"
+#include "chrome/common/pref_names.h"
+#include "components/prefs/pref_service.h"
+
+namespace arc {
+
+namespace {
+chromeos::ArcTermsOfServiceScreenActor* actor_for_testing = nullptr;
+}  // namespace
+
+// static
+void ArcTermsOfServiceInitialOobeNegotiatorForManagedUser::
+    SetArcTermsOfServiceScreenActorForTesting(
+        chromeos::ArcTermsOfServiceScreenActor* actor) {
+  actor_for_testing = actor;
+}
+
+ArcTermsOfServiceInitialOobeNegotiatorForManagedUser::
+    ArcTermsOfServiceInitialOobeNegotiatorForManagedUser() = default;
+
+ArcTermsOfServiceInitialOobeNegotiatorForManagedUser::
+    ~ArcTermsOfServiceInitialOobeNegotiatorForManagedUser() {
+  chromeos::LoginDisplayHost* host = chromeos::LoginDisplayHost::default_host();

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

There are many occurrence to find ArcTermsOfServiceScreenActor*. So could you
introduce a getter in anonymous namespace?

chromeos::ArcTermsOfServiceScreenActor* GetScreenActor() {
  // Inject testing instance.
  if (actor_for_testing)
    return actor_for_testing;

  chormeos::LoginDisplayHost* host = chromeos::LoginDisplayHost::default_host();
  if (!host)
    return nullptr;
  DCHECK(host->GetOobeUI());
  return host->GetOobeUI()->GetArcTermsOfServiceScreenActor();
}

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

Done.

+  if (actor_for_testing) {
+    actor_for_testing->RemoveObserver(this);
+    return;
+  }
+  if (host && host->GetOobeUI())

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

IIUC;
- in most case, we do not need RemoveObserver() in dtor, because it should be
already removed in HandleTermsAccepted().
- The only exception is, when the arc.enabled preference is set to false, so
that negotiation procedure is cancelled. In such a case, host and OOBE UI
instances should be alive always.

Is my understanding true? Could you comment to cover all the case where this
RemoveObserver is actually needed?

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

It is impossible for ARC managed user.

Removed in DTOR

+    host->GetOobeUI()->GetArcTermsOfServiceScreenActor()->RemoveObserver(this);
+}
+
+void ArcTermsOfServiceInitialOobeNegotiatorForManagedUser::
+    StartNegotiationImpl() {
+  if (actor_for_testing) {
+    actor_for_testing->AddObserver(this);
+    return;
+  }
+  chromeos::LoginDisplayHost* host = chromeos::LoginDisplayHost::default_host();

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

Could you comment that who "conceptually" guarantees this is alive at this
moment?
(Also please see my comments about observing the actor and preference.)

It's better to have a very detailed documentation about the lifetime management
across modules (ArcSessionManager, this class, and
ArcTermsOfServiceScreenActor).

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

Comments added.

+  DCHECK(host);
+  DCHECK(host->GetOobeUI());
+
+  host->GetOobeUI()->GetArcTermsOfServiceScreenActor()->AddObserver(this);
+}
+
+void ArcTermsOfServiceInitialOobeNegotiatorForManagedUser::HandleTermsAccepted(
+    bool accepted) {
+  if (actor_for_testing) {
+    actor_for_testing->RemoveObserver(this);
+  } else {
+    chromeos::LoginDisplayHost* host =
+        chromeos::LoginDisplayHost::default_host();
+    DCHECK(host && host->GetOobeUI());
+    host->GetOobeUI()->GetArcTermsOfServiceScreenActor()->RemoveObserver(this);
+  }
+  ReportResult(accepted);
+}
+
+void ArcTermsOfServiceInitialOobeNegotiatorForManagedUser::OnSkip() {
+  HandleTermsAccepted(false);
+}
+
+void ArcTermsOfServiceInitialOobeNegotiatorForManagedUser::OnAccept() {
+  HandleTermsAccepted(true);
+}
+
+void ArcTermsOfServiceInitialOobeNegotiatorForManagedUser::OnActorDestroyed(
+    chromeos::ArcTermsOfServiceScreenActor* actor) {
+  HandleTermsAccepted(false);
+}
+
+}  // namespace arc