arc: Use v2/auth LSO page.

chrome/browser/extensions/api/messaging/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/extensions/api/messaging/arc_support_host.h"

[Sergey Ulanov, 2016/03/31 17:24:23]

Does this code need to be in this directory? Can it be moved to components/arc?
This directory is intended for messaging API implementation, but not for
implementation of native messaging hosts.

[khmel, 2016/03/31 18:02:31]

I would say this is hardly achievable. Based on previous reviews we don't want
to add chrome/browser, ash dependencies to components/arc.

As alternative, I can try to move this to c/b/chromeos/arc/
(however arc_support_host.h will be referenced from
c/b/extensions/api/messaging/native_message_host_chromeos.cc)

WDYT?

[Sergey Ulanov, 2016/03/31 18:47:54]

I think c/b/chromeos/arc/ would be a better place for this code.

[khmel, 2016/03/31 19:33:33]

Done.

 
 #include "ash/system/chromeos/devicetype_utils.h"
 #include "base/json/json_reader.h"
 #include "base/json/json_writer.h"
 #include "base/thread_task_runner_handle.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/webui/web_ui_util.h"
 
 namespace {
 const char kAction[] = "action";
+const char kCode[] = "code";
 const char kStatus[] = "status";
 const char kData[] = "data";
 const char kPage[] = "page";
 const char kActionSetLocalization[] = "setLocalization";
-const char kActionCheckAuthCode[] = "checkAuthCode";
+const char kActionStartLSO[] = "startLSO";

[Sergey Ulanov, 2016/03/31 17:24:23]

nit: Suggest calling these startLso and kActionStartLso, with lower-case "so".
Style guide is not prescriptive about abbreviations in names, but it gives
"UrlTable" as a good example of proper name, not "URLTable". This approach would
also allow to avoid unreadable names like EnsureNSSSSLInit.

[khmel, 2016/03/31 18:02:31]

Thanks for sharing this, this makes sense. Noted for the future.

 const char kActionCancelAuthCode[] = "cancelAuthCode";
+const char kActionSetAuthCode[] = "setAuthCode";
 const char kActionCloseUI[] = "closeUI";
 const char kActionShowPage[] = "showPage";
 }  // namespace
 
 // static
 const char ArcSupportHost::kHostName[] = "com.google.arc_support";
 
 // static
 const char* const ArcSupportHost::kHostOrigin[] = {
     "chrome-extension://cnbgggchhmkkdmeppjobngjoejnihlei/"
@@ skipping 43 matching lines @@
       l10n_util::GetStringUTF16(IDS_ARC_OPT_IN_DIALOG_BUTTON_GET_STARTED));
   localized_strings->SetString(
       "buttonRetry",
       l10n_util::GetStringUTF16(IDS_ARC_OPT_IN_DIALOG_BUTTON_RETRY));
   localized_strings->SetString(
       "progressLSOLoading",
       l10n_util::GetStringUTF16(IDS_ARC_OPT_IN_DIALOG_PROGRESS_LSO));
   localized_strings->SetString(
       "progressAndroidLoading",
       l10n_util::GetStringUTF16(IDS_ARC_OPT_IN_DIALOG_PROGRESS_ANDROID));
+  localized_strings->SetString(
+      "authorizationFailed",
+      l10n_util::GetStringUTF16(IDS_ARC_OPT_IN_DIALOG_AUTHORIZATION_FAILED));
 
   const std::string& app_locale = g_browser_process->GetApplicationLocale();
   webui::SetLoadTimeDataDefaults(app_locale, localized_strings.get());
 
   base::DictionaryValue request;
   std::string request_string;
   request.SetString(kAction, kActionSetLocalization);
   request.Set(kData, std::move(localized_strings));
   base::JSONWriter::Write(request, &request_string);
   client_->PostMessageFromNativeHost(request_string);
@@ skipping 21 matching lines @@
   response.SetString(kStatus, status);
   std::string response_string;
   base::JSONWriter::Write(response, &response_string);
   client_->PostMessageFromNativeHost(response_string);
 }
 
 void ArcSupportHost::OnMessage(const std::string& request_string) {
   scoped_ptr<base::Value> request_value =
       base::JSONReader::Read(request_string);
   scoped_ptr<base::DictionaryValue> request(
       static_cast<base::DictionaryValue*>(request_value.release()));

[Sergey Ulanov, 2016/03/31 17:24:23]

You should to use Value::GetAsDictionary() to convert value to DictionaryValue.
This static_cast<> will succeed even if request_value is not a DictionaryValue
and then GetString() call below is likely to crash.

   if (!request.get()) {
     NOTREACHED();
     return;
   }
 
   std::string action;
   if (!request->GetString(kAction, &action)) {
     NOTREACHED();

[Sergey Ulanov, 2016/03/31 17:24:24]

I think you want to log this error, but it shouldn't be marked as NOTREACHED().
You don't want to crash chrome when an extension sends invalid message. The only
exception is when the messages come only from built-in extension. Is that the
case here?

[khmel, 2016/03/31 18:02:31]

This is our built_in extension. Seems NOTREACHED  is acceptable here.

     return;
   }
 
-  if (action == kActionCheckAuthCode) {
-    arc::ArcAuthService::Get()->CheckAuthCode();
+  if (action == kActionStartLSO) {
+    arc::ArcAuthService::Get()->StartLSO();

[Sergey Ulanov, 2016/03/31 17:24:23]

nit: StartLso()

[khmel, 2016/03/31 18:02:31]

Done.

+  } else if (action == kActionSetAuthCode) {
+    std::string code;
+    if (!request->GetString(kCode, &code)) {
+      NOTREACHED();
+      return;
+    }
+    arc::ArcAuthService::Get()->SetAuthCodeAndStartArc(code);
   } else if (action == kActionCancelAuthCode) {
     arc::ArcAuthService::Get()->CancelAuthCode();
   } else {
     NOTREACHED();
   }
 }
 
 scoped_refptr<base::SingleThreadTaskRunner> ArcSupportHost::task_runner()
     const {
   return base::ThreadTaskRunnerHandle::Get();
 }