Extract ArcSupportMessageHost.

Currently ArcSupportHost is the ARC's implementation of
NativeMessageHost. Because of the system structure, the instance
is created and managed by the module outside of ARC.

This CL extracts the implementation of that part into
ArcSupportMessageHost so that the ArcSupportHost, which will
be the main interface to show UI things for ARC, can be managed
along with the ARC's life-time.

There is a plan to extract non authorization related part from
ArcAuthSerivce. Meanwhile, the instance is managed by
ArcAuthService, temporarily.

BUG=657687, 636218, 633258, b/31079732
TEST=Ran on test device. Ran trybots.

Review-Url: https://codereview.chromium.org/2436903003
Cr-Commit-Position: refs/heads/master@{#427277}

chrome/browser/chromeos/BUILD.gn

@@ -214,6 +214,8 @@ source_set("chromeos") {
     "arc/downloads_watcher/arc_downloads_watcher_service.h",
     "arc/enterprise/arc_enterprise_reporting_service.cc",
     "arc/enterprise/arc_enterprise_reporting_service.h",
+    "arc/extensions/arc_support_message_host.cc",
+    "arc/extensions/arc_support_message_host.h",
     "arc/fileapi/arc_content_file_system_async_file_util.cc",
     "arc/fileapi/arc_content_file_system_async_file_util.h",
     "arc/fileapi/arc_content_file_system_backend_delegate.cc",

chrome/browser/chromeos/arc/arc_auth_service.cc

@@ -484,6 +484,11 @@ void ArcAuthService::OnPrimaryUserProfilePrepared(Profile* profile) {
   }
 
   profile_ = profile;
+  // Create the support host at initialization. Note that, practically,
+  // ARC support Chrome app is rarely used (only opt-in and re-auth flow).
+  // So, it may be better to initialize it lazily.
+  // TODO(hidehiko): Revisit to think about lazy initialization.
+  support_host_.reset(new ArcSupportHost());
   SetState(State::STOPPED);
 
   PrefServiceSyncableFromProfile(profile_)->AddSyncedPrefObserver(
@@ -565,7 +570,6 @@ void ArcAuthService::ShowUI(UIPage page, const base::string16& status) {
           ArcSupportHost::kHostAppId);
   CHECK(extension && extensions::util::IsAppLaunchable(
                          ArcSupportHost::kHostAppId, profile_));
-
   OpenApplication(CreateAppLaunchParamsUserContainer(
       profile_, extension, WindowOpenDisposition::NEW_WINDOW,
       extensions::SOURCE_CHROME_INTERNAL));
@@ -682,8 +686,8 @@ void ArcAuthService::RemoveObserver(Observer* observer) {
 void ArcAuthService::CloseUI() {
   ui_page_ = UIPage::NO_PAGE;
   ui_page_status_.clear();
-  for (auto& observer : observer_list_)
-    observer.OnOptInUIClose();
+  if (support_host_)
+    support_host_->Close();
   if (!g_disable_ui_for_testing)
     ArcAuthNotification::Hide();
 }
@@ -691,8 +695,8 @@ void ArcAuthService::CloseUI() {
 void ArcAuthService::SetUIPage(UIPage page, const base::string16& status) {
   ui_page_ = page;
   ui_page_status_ = status;
-  for (auto& observer : observer_list_)
-    observer.OnOptInUIShowPage(ui_page_, ui_page_status_);
+  if (support_host_)
+    support_host_->ShowPage(ui_page_, ui_page_status_);
 }
 
 // This is the special method to support enterprise mojo API.

chrome/browser/chromeos/arc/arc_auth_service.h

@@ -36,6 +36,8 @@ namespace user_prefs {
 class PrefRegistrySyncable;
 }
 
+class ArcSupportHost;
+
 namespace arc {
 
 class ArcAndroidManagementChecker;
@@ -80,12 +82,6 @@ class ArcAuthService : public ArcService,
     // Called whenever Opt-In state of the ARC has been changed.
     virtual void OnOptInChanged(State state) {}
 
-    // Called to notify that OptIn UI needs to be closed.
-    virtual void OnOptInUIClose() {}
-
-    // Called to notify that OptIn UI needs to show specific page.
-    virtual void OnOptInUIShowPage(UIPage page, const base::string16& status) {}
-
     // Called to notify that ARC bridge is shut down.
     virtual void OnShutdownBridge() {}
 
@@ -207,8 +203,11 @@ class ArcAuthService : public ArcService,
   // Returns current page status, relevant to the specific page.
   const base::string16& ui_page_status() const { return ui_page_status_; }
 
+  ArcSupportHost* support_host() { return support_host_.get(); }
+
  private:
   void StartArc();
+  // TODO: move UI methods/fields to ArcSupportHost.
   void ShowUI(UIPage page, const base::string16& status);
   void CloseUI();
   void SetUIPage(UIPage page, const base::string16& status);
@@ -248,6 +247,11 @@ class ArcAuthService : public ArcService,
   bool reenable_arc_ = false;
   base::OneShotTimer arc_sign_in_timer_;
 
+  // Temporarily keeps the ArcSupportHost instance.
+  // This should be moved to ArcSessionManager when the refactoring is
+  // done.
+  std::unique_ptr<ArcSupportHost> support_host_;
+
   std::unique_ptr<ArcAuthContext> context_;
   std::unique_ptr<ArcAuthCodeFetcher> auth_code_fetcher_;
   std::unique_ptr<ArcAndroidManagementChecker> android_management_checker_;

chrome/browser/chromeos/arc/arc_auth_service_unittest.cc

@@ -114,8 +114,8 @@ class ArcAuthServiceTest : public testing::Test {
 
   content::TestBrowserThreadBundle thread_bundle_;
   std::unique_ptr<FakeArcBridgeService> bridge_service_;
-  std::unique_ptr<ArcAuthService> auth_service_;
   std::unique_ptr<TestingProfile> profile_;
+  std::unique_ptr<ArcAuthService> auth_service_;
   chromeos::ScopedUserManagerEnabler user_manager_enabler_;
   base::ScopedTempDir temp_dir_;
 

chrome/browser/chromeos/arc/arc_support_host.h

@@ -7,37 +7,43 @@
 
 #include "base/macros.h"
 #include "chrome/browser/chromeos/arc/arc_auth_service.h"
+#include "chrome/browser/chromeos/arc/extensions/arc_support_message_host.h"
 #include "components/prefs/pref_change_registrar.h"
 #include "extensions/browser/api/messaging/native_message_host.h"
 #include "ui/display/display_observer.h"
 
-// Supports communication with Arc support dialog.
-class ArcSupportHost : public extensions::NativeMessageHost,
-                       public arc::ArcAuthService::Observer,
+// Native interface to control ARC support chrome App.
+// TODO(hidehiko): Move more implementation for the UI control from
+// ArcAuthService to this class.
+// TODO(hidehiko,lhchavez,khmel): Extract preference observing into a
+// standalone class so that it can be shared with OOBE flow.
+// TODO(hidehiko,lhchavez): Move this into extensions/ directory, and put it
+// into "arc" namespace. Add unittests at the time.
+class ArcSupportHost : public arc::ArcSupportMessageHost::Observer,
                        public display::DisplayObserver {
  public:
-  static const char kHostName[];
   static const char kHostAppId[];
   static const char kStorageId[];
-  static const char* const kHostOrigin[];
-
-  static std::unique_ptr<NativeMessageHost> Create();
 
+  ArcSupportHost();
   ~ArcSupportHost() override;
 
+  // Called when the communication to arc_support Chrome App is ready.
+  void SetMessageHost(arc::ArcSupportMessageHost* message_host);
+
+  // Called when the communication to arc_support Chrome App is closed.
+  // The argument message_host is used to check if the given |message_host|
+  // is what this instance uses know, to avoid racy case.
+  // If |message_host| is different from the one this instance knows,
+  // this is no op.
+  void UnsetMessageHost(arc::ArcSupportMessageHost* message_host);
+
   // Requests to close the extension window.
   void Close();
+  void ShowPage(arc::ArcAuthService::UIPage page, const base::string16& status);
 
-  // Overrides NativeMessageHost:
-  void Start(Client* client) override;
-  void OnMessage(const std::string& request_string) override;
-  scoped_refptr<base::SingleThreadTaskRunner> task_runner() const override;
-
-  // Overrides arc::ArcAuthService::Observer:
-  // TODO(hidehiko): Get rid of Observer interface.
-  void OnOptInUIClose() override;
-  void OnOptInUIShowPage(arc::ArcAuthService::UIPage page,
-                         const base::string16& status) override;
+  // arc::ArcSupportMessageHost::Observer override:
+  void OnMessage(const base::DictionaryValue& message) override;
 
   // display::DisplayObserver:
   void OnDisplayAdded(const display::Display& new_display) override;
@@ -46,8 +52,6 @@ class ArcSupportHost : public extensions::NativeMessageHost,
                                uint32_t changed_metrics) override;
 
  private:
-  ArcSupportHost();
-
   bool Initialize();
   void OnMetricsPreferenceChanged();
   void OnBackupAndRestorePreferenceChanged();
@@ -75,17 +79,10 @@ class ArcSupportHost : public extensions::NativeMessageHost,
   void EnableBackupRestore(bool is_enabled);
   void EnableLocationService(bool is_enabled);
 
-  // Unowned pointer.
-  Client* client_ = nullptr;
-
-  // Keep if Close() is requested from the browser.
-  // TODO(hidehiko): Remove this. This is temporarily introduced for checking
-  // if ArcAuthService::CancelAuthCode() needs to be invoked or not.
-  // ArcAuthService should know its own state and the transition so moving to
-  // there should simplify the structure. However, it is blocked by the current
-  // dependency. For the clean up, more refactoring is needed, which can be
-  // bigger changes.
-  bool close_requested_ = false;
+  void DisconnectMessageHost();
+
+  // The instance is created and managed by Chrome.
+  arc::ArcSupportMessageHost* message_host_ = nullptr;
 
   // Used to track metrics preference.
   PrefChangeRegistrar pref_local_change_registrar_;

chrome/browser/chromeos/arc/extensions/arc_support_message_host.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/arc/extensions/arc_support_message_host.h"
+
+#include "base/json/json_reader.h"
+#include "base/json/json_writer.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "chrome/browser/chromeos/arc/arc_auth_service.h"
+#include "chrome/browser/chromeos/arc/arc_support_host.h"
+
+namespace arc {
+
+// static
+const char ArcSupportMessageHost::kHostName[] = "com.google.arc_support";
+
+// static
+const char* const ArcSupportMessageHost::kHostOrigin[] = {
+    "chrome-extension://cnbgggchhmkkdmeppjobngjoejnihlei/"};
+
+// static
+std::unique_ptr<extensions::NativeMessageHost> ArcSupportMessageHost::Create() {
+  return std::unique_ptr<NativeMessageHost>(new ArcSupportMessageHost());
+}
+
+ArcSupportMessageHost::ArcSupportMessageHost() = default;
+
+ArcSupportMessageHost::~ArcSupportMessageHost() {
+  // On shutdown, ArcAuthService may be already deleted. In that case,
+  // ArcAuthService::Get() returns nullptr. Note that, ArcSupportHost
+  // disconnects to this instance on shutdown already.
+  ArcAuthService* auth_service = ArcAuthService::Get();
+  if (auth_service) {
+    DCHECK(auth_service->support_host());
+    auth_service->support_host()->UnsetMessageHost(this);
+  }
+}
+
+void ArcSupportMessageHost::SendMessage(const base::Value& message) {
+  if (!client_)
+    return;
+
+  std::string message_string;
+  base::JSONWriter::Write(message, &message_string);
+  client_->PostMessageFromNativeHost(message_string);
+}
+
+void ArcSupportMessageHost::SetObserver(Observer* observer) {
+  // We assume that the observer instance is only ArcSupportHost, which is
+  // currently system unique. This is also used to reset the observere,
+  // so |observer| xor |observer_| needs to be nullptr.
+  DCHECK(!observer != !observer_);
+  observer_ = observer;
+}
+
+void ArcSupportMessageHost::Start(Client* client) {
+  DCHECK(!client_);
+  client_ = client;
+
+  ArcAuthService* auth_service = ArcAuthService::Get();
+  DCHECK(auth_service);
+  DCHECK(auth_service->support_host());
+  auth_service->support_host()->SetMessageHost(this);
+}
+
+void ArcSupportMessageHost::OnMessage(const std::string& message_string) {
+  if (!observer_)
+    return;
+
+  std::unique_ptr<base::Value> message_value =
+      base::JSONReader::Read(message_string);
+  base::DictionaryValue* message;
+  if (!message_value || !message_value->GetAsDictionary(&message)) {
+    NOTREACHED();
+    return;
+  }
+
+  observer_->OnMessage(*message);
+}
+
+scoped_refptr<base::SingleThreadTaskRunner> ArcSupportMessageHost::task_runner()
+    const {
+  return base::ThreadTaskRunnerHandle::Get();
+}
+
+}  // namespace arc

chrome/browser/chromeos/arc/extensions/arc_support_message_host.h

+// 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_EXTENSIONS_ARC_SUPPORT_MESSAGE_HOST_H_
+#define CHROME_BROWSER_CHROMEOS_ARC_EXTENSIONS_ARC_SUPPORT_MESSAGE_HOST_H_
+
+#include <memory>
+
+#include "base/macros.h"
+#include "base/values.h"
+#include "extensions/browser/api/messaging/native_message_host.h"
+
+namespace arc {
+
+// Implements message routing to communicate with arc_support Chrome App.
+// Provides JSON (base::Value) APIs for the communication.
+class ArcSupportMessageHost : public extensions::NativeMessageHost {
+ public:
+  class Observer {
+   public:
+    // Called when an message is sent from arc_support Chrome App.
+    virtual void OnMessage(const base::DictionaryValue& message) = 0;
+
+   protected:
+    virtual ~Observer() = default;
+  };
+
+  static const char kHostName[];
+  static const char* const kHostOrigin[];
+
+  // Called when the arc_support connects the "port". Returns the
+  // instance of ArcSupportMessageHost.
+  static std::unique_ptr<NativeMessageHost> Create();
+
+  ~ArcSupportMessageHost() override;
+
+  // Sends a message to arc_support. If the client is not yet ready, the
+  // message will be just ignored.
+  void SendMessage(const base::Value& message);
+
+  // Registers (or unregisters if nullptr) the observer. Currently this class
+  // assumes that it has only one observer.
+  void SetObserver(Observer* observer);
+
+ private:
+  // Keep private. The instance will be created via Create().
+  ArcSupportMessageHost();
+
+  // extensions::NativeMessageHost overrides:
+  void Start(Client* client) override;
+  void OnMessage(const std::string& request_string) override;
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner() const override;
+
+  Observer* observer_ = nullptr;
+  Client* client_ = nullptr;
+
+  DISALLOW_COPY_AND_ASSIGN(ArcSupportMessageHost);
+};
+
+}  // namespace arc
+
+#endif  // CHROME_BROWSER_CHROMEOS_ARC_EXTENSIONS_ARC_SUPPORT_MESSAGE_HOST_H_

chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc

@@ -17,7 +17,7 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
-#include "chrome/browser/chromeos/arc/arc_support_host.h"
+#include "chrome/browser/chromeos/arc/extensions/arc_support_message_host.h"
 #include "components/policy/core/common/policy_service.h"
 #include "content/public/browser/browser_thread.h"
 #include "extensions/common/constants.h"
@@ -129,18 +129,13 @@ const char* const kRemotingIt2MeOrigins[] = {
     "chrome-extension://hpodccmdligbeohchckkeajbfohibipg/"};
 
 static const BuiltInHost kBuiltInHost[] = {
-    {"com.google.chrome.test.echo", // ScopedTestNativeMessagingHost::kHostName
-     kEchoHostOrigins,
-     arraysize(kEchoHostOrigins),
-     &EchoHost::Create},
-     {"com.google.chrome.remote_assistance",
-     kRemotingIt2MeOrigins,
-     arraysize(kRemotingIt2MeOrigins),
-     &CreateIt2MeHost},
-     {ArcSupportHost::kHostName,
-     ArcSupportHost::kHostOrigin,
-     1,
-     &ArcSupportHost::Create},
+    {"com.google.chrome.test.echo",  // ScopedTestNativeMessagingHost::kHostName
+     kEchoHostOrigins, arraysize(kEchoHostOrigins), &EchoHost::Create},
+    {"com.google.chrome.remote_assistance", kRemotingIt2MeOrigins,
+     arraysize(kRemotingIt2MeOrigins), &CreateIt2MeHost},
+    {arc::ArcSupportMessageHost::kHostName,
+     arc::ArcSupportMessageHost::kHostOrigin, 1,
+     &arc::ArcSupportMessageHost::Create},
 };
 
 bool MatchesSecurityOrigin(const BuiltInHost& host,