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

chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h

Index: chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h
diff --git a/chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h b/chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h
new file mode 100644
index 0000000000000000000000000000000000000000..43d1403c06d71e988183631d38e7c4c5e5f85141
--- /dev/null
+++ b/chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h
@@ -0,0 +1,57 @@
+// 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.
+
+#ifndef CHROME_BROWSER_CHROMEOS_ARC_OPTIN_ARC_TERMS_OF_SERVICE_OOBE_NEGOTIATOR_H_
+#define CHROME_BROWSER_CHROMEOS_ARC_OPTIN_ARC_TERMS_OF_SERVICE_OOBE_NEGOTIATOR_H_
+
+#include "base/macros.h"
+#include "chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h"
+#include "chrome/browser/chromeos/login/screens/arc_terms_of_service_screen_actor_observer.h"
+
+namespace chromeos {
+class ArcTermsOfServiceScreenActor;
+}
+
+namespace arc {
+
+// Handles the Terms-of-service agreement user action via OOBE OptIn UI.
+class ArcTermsOfServiceOobeNegotiator
+    : public ArcTermsOfServiceNegotiator,
+      public chromeos::ArcTermsOfServiceScreenActorObserver {
+ public:
+  ArcTermsOfServiceOobeNegotiator();
+  ~ArcTermsOfServiceOobeNegotiator() override;
+
+  // Overrides ARC OOBE screen handler in unit tests, where OOBE UI is not

[hidehiko, 2017/01/31 13:55:30]

nit: "overrdies" is used for method overriding in C++ context in most cases. How
about "Inject ARC ..." ?

[khmel, 2017/01/31 15:25:22]

Done.

+  // available.
+  static void SetArcTermsOfServiceScreenActorForTesting(
+      chromeos::ArcTermsOfServiceScreenActor* actor);
+
+ private:
+  // Helper to handle callbacks from

[hidehiko, 2017/01/31 13:55:30]

This is what the method "is", but from future reader's point of view, what the
method "does" (or "how" the method works) is more important. Cold you comment
so?

Also, in the latest PS, you introduced a new restriction that once
StartNegotiationImpl() is called, then HandleTermsAccepted() must be called
before the instance is destroyed. Could you comment it, too?

[khmel, 2017/01/31 15:25:22]

Done.

+  // chromeos::ArcTermsOfServiceScreenActorObserver.
+  void HandleTermsAccepted(bool accepted);
+
+  // chromeos::ArcTermsOfServiceScreenActorObserver:
+  void OnSkip() override;
+  void OnAccept() override;
+  void OnActorDestroyed(chromeos::ArcTermsOfServiceScreenActor* actor) override;
+
+  // ArcTermsOfServiceNegotiator:
+  void StartNegotiationImpl() override;
+
+  // Unowned pointer. chromeos::ArcTermsOfServiceScreenActor is available at the
+  // moment of initialization. However in case user signs out while ARC OOBE

[hidehiko, 2017/01/31 13:55:30]

nit: initialization implies ctor. Maybe: "... is available on starting
negotiation"?

[khmel, 2017/01/31 15:25:22]

Done.

+  // OptIn is active then LoginDisplayHost is detached first and
+  // OnActorDestroyed is called after. There is no chance to get
+  // chromeos::ArcTermsOfServiceScreenActor in last case. Solution is to keep

[hidehiko, 2017/01/31 13:55:30]

I was confused a bit here, because the pointer to the Actor instance is passed
to OnActorDestroyed.
IIUC, you keep the raw ptr just for accessing to the Actor instance in a unified
way (regardless of in On{Skip,Accept,ActorDestroyed}())?

[khmel, 2017/01/31 15:25:22]

Yes, it is true. It is also a DCHECK that actors match in OnActorDestroyed.

[hidehiko, 2017/01/31 15:47:09]

Ok, then the comment is not reflecting the actual situation, because there is a
way to obtain the Actor (via passed as an argument).

Maybe something like;

If a user signs out while ARC OOBE opt-in is active, LoginDisplayHost is
detached first then OnActorDestroyed is called.
It means, in OnSkip() and OnAccept(), the Actor needs to be obtained via
LoginDisplayHost, but in OnActorDestroyed(), the argument needs to be used.
In order to use the same way to access the Actor, remember the pointer in
StartNegotiationImpl(), and reset in HandleTermsAccepted().

[khmel, 2017/01/31 16:32:19]

Done.

+  // pointer.
+  chromeos::ArcTermsOfServiceScreenActor* screen_actor_ = nullptr;
+
+  DISALLOW_COPY_AND_ASSIGN(ArcTermsOfServiceOobeNegotiator);
+};
+
+}  // namespace arc
+
+#endif  // CHROME_BROWSER_CHROMEOS_ARC_OPTIN_ARC_TERMS_OF_SERVICE_OOBE_NEGOTIATOR_H_