Extract ArcSupportMessageHost.

chrome/browser/chromeos/arc/arc_support_host.cc

Index: chrome/browser/chromeos/arc/arc_support_host.cc
diff --git a/chrome/browser/chromeos/arc/arc_support_host.cc b/chrome/browser/chromeos/arc/arc_support_host.cc
index f5d84fee35c008c5a1313bed0b0d9f7952d745dc..2475533c6ce1f135432f3d1dcc561c270f3afc43 100644
--- a/chrome/browser/chromeos/arc/arc_support_host.cc
+++ b/chrome/browser/chromeos/arc/arc_support_host.cc
@@ -78,38 +78,26 @@ constexpr char kEventOnSendFeedbackClicked[] = "onSendFeedbackClicked";
 }  // namespace
 
 // static
-const char ArcSupportHost::kHostName[] = "com.google.arc_support";
-
-// static
 const char ArcSupportHost::kHostAppId[] = "cnbgggchhmkkdmeppjobngjoejnihlei";
 
 // static
 const char ArcSupportHost::kStorageId[] = "arc_support";
 
-// static
-const char* const ArcSupportHost::kHostOrigin[] = {
-    "chrome-extension://cnbgggchhmkkdmeppjobngjoejnihlei/"};
-
-// static
-std::unique_ptr<extensions::NativeMessageHost> ArcSupportHost::Create() {
-  return std::unique_ptr<NativeMessageHost>(new ArcSupportHost());
-}
-
 ArcSupportHost::ArcSupportHost() {
+  // TODO(hidehiko): Get rid of dependency to ArcAuthService.
   arc::ArcAuthService* arc_auth_service = arc::ArcAuthService::Get();
   DCHECK(arc_auth_service);
 
   if (!arc_auth_service->IsAllowed())
     return;
 
-  arc_auth_service->AddObserver(this);
-  display::Screen::GetScreen()->AddObserver(this);
-
-  pref_local_change_registrar_.Init(g_browser_process->local_state());
-  pref_local_change_registrar_.Add(
-      metrics::prefs::kMetricsReportingEnabled,
-      base::Bind(&ArcSupportHost::OnMetricsPreferenceChanged,
-                 base::Unretained(this)));
+  if (g_browser_process->local_state()) {

[hidehiko, 2016/10/20 18:36:17]

Note: No service is running on unittest, if stmt is workaround.
This will be cleaned, when the logic is extracted.

[Luis Héctor Chávez, 2016/10/21 02:24:08]

Can you add a TODO?

[hidehiko, 2016/10/21 07:40:27]

Done.

+    pref_local_change_registrar_.Init(g_browser_process->local_state());
+    pref_local_change_registrar_.Add(
+        metrics::prefs::kMetricsReportingEnabled,
+        base::Bind(&ArcSupportHost::OnMetricsPreferenceChanged,
+                   base::Unretained(this)));
+  }
 
   pref_change_registrar_.Init(arc_auth_service->profile()->GetPrefs());
   pref_change_registrar_.Add(
@@ -123,27 +111,41 @@ ArcSupportHost::ArcSupportHost() {
 }
 
 ArcSupportHost::~ArcSupportHost() {
-  display::Screen::GetScreen()->RemoveObserver(this);
-  arc::ArcAuthService* arc_auth_service = arc::ArcAuthService::Get();
-  if (arc_auth_service)
-    arc_auth_service->RemoveObserver(this);
+  if (message_host_)
+    DisconnectMessageHost();
 }
 
 void ArcSupportHost::Close() {
-  if (!client_)
+  if (!message_host_)
     return;
 
-  close_requested_ = true;
-  base::DictionaryValue response;
-  response.SetString(kAction, kActionCloseWindow);
-  std::string response_string;
-  base::JSONWriter::Write(response, &response_string);
-  client_->PostMessageFromNativeHost(response_string);
+  base::DictionaryValue message;
+  message.SetString(kAction, kActionCloseWindow);
+  message_host_->SendMessage(message);
+
+  // Disconnect immediately, so that onWindowClosed event will not be
+  // delivered to here.

[Luis Héctor Chávez, 2016/10/21 02:24:08]

nit: s/delivered to here/delivered here/

+  DisconnectMessageHost();
 }
 
-void ArcSupportHost::Start(Client* client) {
-  DCHECK(!client_);
-  client_ = client;
+void ArcSupportHost::ShowPage(arc::ArcAuthService::UIPage page,
+                              const base::string16& status) {
+  if (!message_host_)
+    return;

[khmel, 2016/10/21 03:34:49]

IIUC this should never happen. May be LOG(ERROR)? 

[hidehiko, 2016/10/21 07:40:27]

This happens. Because ArcSupportMessageHost creation is async, when this is
called, it is no guarantee that message_host_ is ready.

[khmel, 2016/10/21 14:11:17]

I have concern about losing data for user (not shown error or other important
information is bad). Normally, IIUC, when this happens that means page is
creating in the background and will be shown soon. But if not, and page could
not be created? In this case in log we would have nothing. Not sure how to
handle this better. May be set flag that indicates we have pending request to
show the page and on closing to analyze that we reset it.

[hidehiko, 2016/10/24 06:03:26]

Currently, no data loss is ensured in the ArcAuthService.
If the page show event is fired in ArcAuthService before the actual window
creation (or, in more precise, before ArcSupportMessageHost is set to
ArcSupportHost), the ArcAuthService's fields remember it, and Initialize() sends
them to the extension.
It is just because the existing code is so, and I think it is not good design.
I'll send another CL to move the responsibility to this class immediately after
this CL ges landed. Then, client won't be necessary to take care about such a
timing issue.

[khmel, 2016/10/24 15:05:36]

Acknowledged.

+
+  base::DictionaryValue message;
+  message.SetString(kAction, kActionShowPage);
+  message.SetInteger(kPage, static_cast<int>(page));
+  message.SetString(kStatus, status);
+  message_host_->SendMessage(message);
+}
+
+void ArcSupportHost::SetMessageHost(arc::ArcSupportMessageHost* message_host) {

[Luis Héctor Chávez, 2016/10/21 02:24:07]

if (message_host_ == message_host)
  return;

for symmetry?

[hidehiko, 2016/10/21 07:40:27]

Done.

+  if (message_host_)
+    DisconnectMessageHost();
+  message_host_ = message_host;
+  message_host_->SetObserver(this);
+  display::Screen::GetScreen()->AddObserver(this);
 
   if (!Initialize()) {
     Close();
@@ -156,12 +158,24 @@ void ArcSupportHost::Start(Client* client) {
 
   arc::ArcAuthService* arc_auth_service = arc::ArcAuthService::Get();
   DCHECK(arc_auth_service);
-  OnOptInUIShowPage(arc_auth_service->ui_page(),
-                    arc_auth_service->ui_page_status());
+  ShowPage(arc_auth_service->ui_page(), arc_auth_service->ui_page_status());
+}
+
+void ArcSupportHost::UnsetMessageHost(
+    arc::ArcSupportMessageHost* message_host) {
+  if (message_host_ != message_host)
+    return;
+  DisconnectMessageHost();
+}
+
+void ArcSupportHost::DisconnectMessageHost() {
+  display::Screen::GetScreen()->RemoveObserver(this);

[Luis Héctor Chávez, 2016/10/21 02:24:07]

DCHECK(message_host_); ?

[hidehiko, 2016/10/21 07:40:27]

Done.

+  message_host_->SetObserver(nullptr);
+  message_host_ = nullptr;
 }
 
 bool ArcSupportHost::Initialize() {
-  DCHECK(client_);
+  DCHECK(message_host_);
   arc::ArcAuthService* arc_auth_service = arc::ArcAuthService::Get();
   if (!arc_auth_service->IsAllowed())
     return false;
@@ -253,14 +267,11 @@ bool ArcSupportHost::Initialize() {
       "isOwnerProfile",
       chromeos::ProfileHelper::IsOwnerProfile(arc_auth_service->profile()));
 
-  base::DictionaryValue request;
-  std::string request_string;
-  request.SetString(kAction, kActionInitialize);
-  request.Set(kData, std::move(loadtime_data));
-  request.SetString(kDeviceId, device_id);
-  base::JSONWriter::Write(request, &request_string);
-  client_->PostMessageFromNativeHost(request_string);
-
+  base::DictionaryValue message;
+  message.SetString(kAction, kActionInitialize);
+  message.Set(kData, std::move(loadtime_data));
+  message.SetString(kDeviceId, device_id);
+  message_host_->SendMessage(message);
   return true;
 }
 
@@ -270,11 +281,12 @@ void ArcSupportHost::OnDisplayRemoved(const display::Display& old_display) {}
 
 void ArcSupportHost::OnDisplayMetricsChanged(const display::Display& display,
                                              uint32_t changed_metrics) {
-  base::DictionaryValue request;
-  std::string request_string;
-  request.SetString(kAction, kActionSetWindowBounds);
-  base::JSONWriter::Write(request, &request_string);
-  client_->PostMessageFromNativeHost(request_string);
+  if (!message_host_)
+    return;
+
+  base::DictionaryValue message;
+  message.SetString(kAction, kActionSetWindowBounds);
+  message_host_->SendMessage(message);
 }
 
 void ArcSupportHost::OnMetricsPreferenceChanged() {
@@ -317,31 +329,14 @@ void ArcSupportHost::SendOptionMode(const std::string& action_name,
 void ArcSupportHost::SendPreferenceUpdate(const std::string& action_name,
                                           bool is_enabled,
                                           bool is_managed) {
-  base::DictionaryValue request;
-  std::string request_string;
-  request.SetString(kAction, action_name);
-  request.SetBoolean(kEnabled, is_enabled);
-  request.SetBoolean(kManaged, is_managed);
-  base::JSONWriter::Write(request, &request_string);
-  client_->PostMessageFromNativeHost(request_string);
-}
-
-void ArcSupportHost::OnOptInUIClose() {
-  Close();
-}
-
-void ArcSupportHost::OnOptInUIShowPage(arc::ArcAuthService::UIPage page,
-                                       const base::string16& status) {
-  if (!client_)
+  if (!message_host_)
     return;
 
-  base::DictionaryValue response;
-  response.SetString(kAction, kActionShowPage);
-  response.SetInteger(kPage, static_cast<int>(page));
-  response.SetString(kStatus, status);
-  std::string response_string;
-  base::JSONWriter::Write(response, &response_string);
-  client_->PostMessageFromNativeHost(response_string);
+  base::DictionaryValue message;
+  message.SetString(kAction, action_name);
+  message.SetBoolean(kEnabled, is_enabled);
+  message.SetBoolean(kManaged, is_managed);
+  message_host_->SendMessage(message);
 }
 
 void ArcSupportHost::EnableMetrics(bool is_enabled) {
@@ -362,17 +357,9 @@ void ArcSupportHost::EnableLocationService(bool is_enabled) {
   pref_service->SetBoolean(prefs::kArcLocationServiceEnabled, is_enabled);
 }
 
-void ArcSupportHost::OnMessage(const std::string& message_string) {
-  std::unique_ptr<base::Value> message_value =
-      base::JSONReader::Read(message_string);
-  base::DictionaryValue* message;
-  if (!message_value || !message_value->GetAsDictionary(&message)) {
-    NOTREACHED();
-    return;
-  }
-
+void ArcSupportHost::OnMessage(const base::DictionaryValue& message) {
   std::string event;
-  if (!message->GetString(kEvent, &event)) {
+  if (!message.GetString(kEvent, &event)) {
     NOTREACHED();
     return;
   }
@@ -382,11 +369,10 @@ void ArcSupportHost::OnMessage(const std::string& message_string) {
   DCHECK(arc_auth_service);
 
   if (event == kEventOnWindowClosed) {
-    if (!close_requested_)
-      arc_auth_service->CancelAuthCode();
+    arc_auth_service->CancelAuthCode();
   } else if (event == kEventOnAuthSuccedded) {
     std::string code;
-    if (message->GetString(kCode, &code)) {
+    if (message.GetString(kCode, &code)) {
       arc_auth_service->SetAuthCodeAndStartArc(code);
     } else {
       NOTREACHED();
@@ -395,11 +381,11 @@ void ArcSupportHost::OnMessage(const std::string& message_string) {
     bool is_metrics_enabled;
     bool is_backup_restore_enabled;
     bool is_location_service_enabled;
-    if (message->GetBoolean(kIsMetricsEnabled, &is_metrics_enabled) &&
-        message->GetBoolean(kIsBackupRestoreEnabled,
-                            &is_backup_restore_enabled) &&
-        message->GetBoolean(kIsLocationServiceEnabled,
-                            &is_location_service_enabled)) {
+    if (message.GetBoolean(kIsMetricsEnabled, &is_metrics_enabled) &&
+        message.GetBoolean(kIsBackupRestoreEnabled,
+                           &is_backup_restore_enabled) &&
+        message.GetBoolean(kIsLocationServiceEnabled,
+                           &is_location_service_enabled)) {
       EnableMetrics(is_metrics_enabled);
       EnableBackupRestore(is_backup_restore_enabled);
       EnableLocationService(is_location_service_enabled);
@@ -410,12 +396,7 @@ void ArcSupportHost::OnMessage(const std::string& message_string) {
   } else if (event == kEventOnSendFeedbackClicked) {
     chrome::OpenFeedbackDialog(nullptr);
   } else {
-    LOG(ERROR) << "Unknown message: " << message_string;
+    LOG(ERROR) << "Unknown message: " << event;
     NOTREACHED();
   }
 }
-
-scoped_refptr<base::SingleThreadTaskRunner> ArcSupportHost::task_runner()
-    const {
-  return base::ThreadTaskRunnerHandle::Get();
-}