Remove controller concept from the ARC support extension.

chrome/browser/chromeos/arc/arc_support_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/arc_support_host.h"
 
 #include <string>
 
 #include "ash/common/system/chromeos/devicetype_utils.h"
 #include "base/i18n/timezone.h"
@@ skipping 14 matching lines @@
 #include "components/prefs/pref_service.h"
 #include "components/user_manager/known_user.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/webui/web_ui_util.h"
 #include "ui/display/screen.h"
 
 namespace {
 const char kAction[] = "action";
 const char kArcManaged[] = "arcManaged";
 const char kCanEnable[] = "canEnable";
-const char kCode[] = "code";
 const char kData[] = "data";
 const char kDeviceId[] = "deviceId";
 const char kEnabled[] = "enabled";
 const char kManaged[] = "managed";
 const char kOn[] = "on";
 const char kPage[] = "page";
 const char kStatus[] = "status";
 const char kText[] = "text";
 const char kActionInitialize[] = "initialize";
 const char kActionSetMetricsMode[] = "setMetricsMode";
 const char kActionBackupAndRestoreMode[] = "setBackupAndRestoreMode";
 const char kActionLocationServiceMode[] = "setLocationServiceMode";
 const char kActionSetWindowBounds[] = "setWindowBounds";
-const char kActionStartLso[] = "startLso";
-const char kActionSetAuthCode[] = "setAuthCode";
-const char kActionEnableMetrics[] = "enableMetrics";
-const char kActionSendFeedback[] = "sendFeedback";
-const char kActionSetBackupRestore[] = "setBackupRestore";
-const char kActionSetLocationService[] = "setLocationService";
 const char kActionCloseWindow[] = "closeWindow";
 const char kActionShowPage[] = "showPage";
 
-// Fired when the extension window is closed.
-const char kActionOnWindowClosed[] = "onWindowClosed";
+// The JSON data sent from the extension should have at least "event" field.
+// Each event data is defined below.
+// The key of the event type.
+const char kEvent[] = "event";

[Luis Héctor Chávez, 2016/10/03 20:40:16]

can you make all of these constexpr now that you're at it?

[hidehiko, 2016/10/04 04:48:19]

Sure. Done.

+
+// "onWindowClosed" is fired when the extension window is closed.
+// No data will be provided.
+const char kEventOnWindowClosed[] = "onWindowClosed";
+
+// "onAuthSucceeded" is fired when successfully done to LSO authorization in
+// extension.
+// The auth token is passed via "code" field.
+const char kEventOnAuthSuccedded[] = "onAuthSucceeded";
+const char kCode[] = "code";
+
+// "onAgree" is fired when a user clicks "Agree" button.
+// The message can have the following three optional fields:
+// - isMetricsEnabled
+// - isBackupRestoreEnabled
+// - isLocationServiceEnabled
+const char kEventOnAgreed[] = "onAgreed";
+const char kIsMetricsEnabled[] = "isMetricsEnabled";
+const char kIsBackupRestoreEnabled[] = "isBackupRestoreEnabled";
+const char kIsLocationServiceEnabled[] = "isLocationServiceEnabled";
+
+// "onSendFeedbackClicked" is fired when a user clicks "Send Feedback" button.
+const char kEventOnSendFeedbackClicked[] = "onSendFeedbackClicked";
 
 }  // namespace
 
 // static
 const char ArcSupportHost::kHostName[] = "com.google.arc_support";
 
 // static
 const char ArcSupportHost::kHostAppId[] = "cnbgggchhmkkdmeppjobngjoejnihlei";
 
 // static
@@ skipping 258 matching lines @@
   pref_service->SetBoolean(prefs::kArcBackupRestoreEnabled, is_enabled);
 }
 
 void ArcSupportHost::EnableLocationService(bool is_enabled) {
   arc::ArcAuthService* arc_auth_service = arc::ArcAuthService::Get();
   DCHECK(arc_auth_service && arc_auth_service->IsAllowed());
   PrefService* pref_service = arc_auth_service->profile()->GetPrefs();
   pref_service->SetBoolean(prefs::kArcLocationServiceEnabled, is_enabled);
 }
 
-void ArcSupportHost::OnMessage(const std::string& request_string) {
-  std::unique_ptr<base::Value> request_value =
-      base::JSONReader::Read(request_string);
-  base::DictionaryValue* request;
-  if (!request_value || !request_value->GetAsDictionary(&request)) {
+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;
   }
 
-  std::string action;
-  if (!request->GetString(kAction, &action)) {
+  std::string event;
+  if (!message->GetString(kEvent, &event)) {
     NOTREACHED();
     return;
   }
 
+  // TODO(hidehiko): Replace by Observer.
   arc::ArcAuthService* arc_auth_service = arc::ArcAuthService::Get();
   DCHECK(arc_auth_service);
 
-  if (action == kActionStartLso) {
-    arc_auth_service->StartLso();
-  } else if (action == kActionSetAuthCode) {
+  if (event == kEventOnWindowClosed) {
+    if (!close_requested_)
+      arc_auth_service->CancelAuthCode();
+    return;
+  }
+  if (event == kEventOnAuthSuccedded) {

[Luis Héctor Chávez, 2016/10/03 20:40:16]

any reason why you're not going with "} else if {"? Should be equivalent and is
more idiomatic.

[hidehiko, 2016/10/04 04:48:19]

if ( ... ) {
  ...
  return;
}

is also idiomatic, I think?
Anyway, the main reason I chose it is that we have early return on error (at
L383, and more in the new PS), so for consistency.

We can remove return, e.g.

if (message->GetString(kCode, &code)) {
  arc_auth_service->SetAuthCodeAndStartArc(code);
} else {
  NOTREACHED();
}

although the early return looks more Chrome style?

Another approach would be creating a function for each event type, if it doesn't
look overkill.

Any suggestions?

[Luis Héctor Chávez, 2016/10/04 04:57:12]

I've mostly seen the early return style when the last remaining path is the
"success" path, in this case it's an error. Function per event type i agree is
definitely overkill.

I think I like the returnless version better. it looks a bit more like a
switch/case, which would be an even better fit for this if C++ supported it for
strings.

[hidehiko, 2016/10/04 06:13:08]

Ok, done.

     std::string code;
-    if (!request->GetString(kCode, &code)) {
+    if (!message->GetString(kCode, &code)) {
       NOTREACHED();
       return;
     }
     arc_auth_service->SetAuthCodeAndStartArc(code);
-  } else if (action == kActionOnWindowClosed) {
-    if (!close_requested_)
-      arc_auth_service->CancelAuthCode();
-  } else if (action == kActionEnableMetrics) {
+    return;
+  }
+  if (event == kEventOnAgreed) {
     bool is_enabled;
-    if (!request->GetBoolean(kEnabled, &is_enabled)) {
-      NOTREACHED();
-      return;
-    }
-    EnableMetrics(is_enabled);
-  } else if (action == kActionSendFeedback) {
+    if (message->GetBoolean(kIsMetricsEnabled, &is_enabled))

[Luis Héctor Chávez, 2016/10/03 20:40:16]

With the change I suggested, it's probably better to make all these NOTREACHED()
if GetBoolean fails.

[hidehiko, 2016/10/04 04:48:19]

Done.

+      EnableMetrics(is_enabled);
+    if (message->GetBoolean(kIsBackupRestoreEnabled, &is_enabled))
+      EnableBackupRestore(is_enabled);
+    if (message->GetBoolean(kIsLocationServiceEnabled, &is_enabled))
+      EnableLocationService(is_enabled);
+    arc_auth_service->StartLso();
+    return;
+  }
+  if (event == kEventOnSendFeedbackClicked) {
     chrome::OpenFeedbackDialog(nullptr);
-  } else if (action == kActionSetBackupRestore) {
-    bool is_enabled;
-    if (!request->GetBoolean(kEnabled, &is_enabled)) {
-      NOTREACHED();
-      return;
-    }
-    EnableBackupRestore(is_enabled);
-  } else if (action == kActionSetLocationService) {
-    bool is_enabled;
-    if (!request->GetBoolean(kEnabled, &is_enabled)) {
-      NOTREACHED();
-      return;
-    }
-    EnableLocationService(is_enabled);
-  } else {
-    NOTREACHED();
+    return;
   }
+
+  LOG(ERROR) << "Unknown message: " << message_string;
+  NOTREACHED();
 }
 
 scoped_refptr<base::SingleThreadTaskRunner> ArcSupportHost::task_runner()
     const {
   return base::ThreadTaskRunnerHandle::Get();
 }