Refactor ArcSupportHost and ArcAuthService part2.

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 189fa74911c8b736a57d41b66618a88e5683f869..045b5fef635262d55976a4f2b72f5ac662507be6 100644
--- a/chrome/browser/chromeos/arc/arc_support_host.cc
+++ b/chrome/browser/chromeos/arc/arc_support_host.cc
@@ -13,12 +13,15 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
-#include "chrome/browser/chromeos/arc/arc_auth_service.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
+#include "chrome/browser/extensions/extension_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/ash/multi_user/multi_user_util.h"
+#include "chrome/browser/ui/extensions/app_launch_params.h"
+#include "chrome/browser/ui/extensions/application_launch.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/user_manager/known_user.h"
+#include "extensions/browser/extension_registry.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/webui/web_ui_util.h"
 #include "ui/display/screen.h"
@@ -82,6 +85,52 @@ constexpr char kEventOnRetryClicked[] = "onRetryClicked";
 // "onSendFeedbackClicked" is fired when a user clicks "Send Feedback" button.
 constexpr char kEventOnSendFeedbackClicked[] = "onSendFeedbackClicked";
 
+std::ostream& operator<<(std::ostream& os, ArcSupportHost::UIPage ui_page) {
+  switch (ui_page) {
+    case ArcSupportHost::UIPage::NO_PAGE:
+      return os << "NO_PAGE";
+    case ArcSupportHost::UIPage::TERMS:
+      return os << "TERMS";
+    case ArcSupportHost::UIPage::LSO:
+      return os << "LSO";
+    case ArcSupportHost::UIPage::ARC_LOADING:
+      return os << "ARC_LOADING";
+    case ArcSupportHost::UIPage::ERROR:
+      return os << "ERROR";
+  }
+
+  // Some compiler reports an error even if all values of an enum-class are
+  // covered indivisually in a switch statement.
+  NOTREACHED();
+  return os;
+}
+
+std::ostream& operator<<(std::ostream& os, ArcSupportHost::Error error) {
+  switch (error) {
+    case ArcSupportHost::Error::SIGN_IN_NETWORK_ERROR:
+      return os << "SIGN_IN_NETWORK_ERROR";
+    case ArcSupportHost::Error::SIGN_IN_SERVICE_UNAVAILABLE_ERROR:
+      return os << "SIGN_IN_SERVICE_UNAVAILABLE_ERROR";
+    case ArcSupportHost::Error::SIGN_IN_BAD_AUTHENTICATION_ERROR:
+      return os << "SIGN_IN_BAD_AUTHENTICATION_ERROR";
+    case ArcSupportHost::Error::SIGN_IN_GMS_NOT_AVAILABLE_ERROR:
+      return os << "SIGN_IN_GMS_NOT_AVAILABLE_ERROR";
+    case ArcSupportHost::Error::SIGN_IN_CLOUD_PROVISION_FLOW_FAIL_ERROR:
+      return os << "SIGN_IN_CLOUD_PROVISION_FLOW_FAIL_ERROR";
+    case ArcSupportHost::Error::SIGN_IN_UNKNOWN_ERROR:
+      return os << "SIGN_IN_UNKNOWN_ERROR";
+    case ArcSupportHost::Error::SERVER_COMMUNICATION_ERROR:
+      return os << "SERVER_COMMUNICATION_ERROR";
+    case ArcSupportHost::Error::ANDROID_MANAGEMENT_REQUIRED_ERROR:
+      return os << "ANDROID_MANAGEMENT_REQUIRED_ERROR";
+  }
+
+  // Some compiler reports an error even if all values of an enum-class are
+  // covered indivisually in a switch statement.
+  NOTREACHED();
+  return os;
+}
+
 }  // namespace
 
 // static
@@ -90,7 +139,7 @@ const char ArcSupportHost::kHostAppId[] = "cnbgggchhmkkdmeppjobngjoejnihlei";
 // static
 const char ArcSupportHost::kStorageId[] = "arc_support";
 
-ArcSupportHost::ArcSupportHost() = default;
+ArcSupportHost::ArcSupportHost(Profile* profile) : profile_(profile) {}
 
 ArcSupportHost::~ArcSupportHost() {
   if (message_host_)
@@ -102,7 +151,13 @@ void ArcSupportHost::AddObserver(Observer* observer) {
   observer_ = observer;
 }
 
+void ArcSupportHost::SetArcManaged(bool is_arc_managed) {
+  DCHECK(!message_host_);
+  is_arc_managed_ = is_arc_managed;
+}
+
 void ArcSupportHost::Close() {
+  ui_page_ = UIPage::NO_PAGE;
   if (!message_host_) {
     VLOG(2) << "ArcSupportHost::Close() is called "
             << "but message_host_ is not available.";
@@ -118,41 +173,96 @@ void ArcSupportHost::Close() {
   DisconnectMessageHost();
 }
 
-void ArcSupportHost::ShowPage(UIPage page, const base::string16& status) {
+void ArcSupportHost::ShowTermsOfService() {
+  ShowPage(UIPage::TERMS);
+}
+
+void ArcSupportHost::ShowLso() {
+  ShowPage(UIPage::LSO);
+}
+
+void ArcSupportHost::ShowArcLoading() {
+  ShowPage(UIPage::ARC_LOADING);
+}
+
+void ArcSupportHost::ShowPage(UIPage ui_page) {
+  ui_page_ = ui_page;

[Luis Héctor Chávez, 2016/11/16 02:58:53]

How about moving this to L191?

[hidehiko, 2016/11/16 17:52:21]

Because ArcAuthService checks UIPage in some context, it needs to be set always
(= even in messag_host_ is non-null case). Clean up is TODO in the header.

[Luis Héctor Chávez, 2016/11/16 23:20:30]

Acknowledged.

   if (!message_host_) {
-    VLOG(2) << "ArcSupportHost::ShowPage() is called "
-            << "but message_host_ is not available.";
+    if (app_start_requested_) {
+      VLOG(2) << "ArcSupportHost::ShowPage(" << ui_page << ") is called "
+              << "before connection to ARC support Chrome app is established.";

[Luis Héctor Chávez, 2016/11/16 02:58:53]

nit: s/is established/has finished establishing/ ? That better reflects that the
connection was pending.

[hidehiko, 2016/11/16 17:52:21]

Done.

+      return;
+    }
+    RequestAppStart();
     return;
   }
 
   base::DictionaryValue message;
-  if (page == UIPage::ERROR || page == UIPage::ERROR_WITH_FEEDBACK) {
-    message.SetString(kAction, kActionShowErrorPage);
-    message.SetString(kErrorMessage, status);
-    message.SetBoolean(kShouldShowSendFeedback,
-                       page == UIPage::ERROR_WITH_FEEDBACK);
-  } else {
-    message.SetString(kAction, kActionShowPage);
-    switch (page) {
-      case UIPage::NO_PAGE:
-        message.SetString(kPage, "none");
-        break;
-      case UIPage::TERMS:
-        message.SetString(kPage, "terms");
-        break;
-      case UIPage::LSO_PROGRESS:
-        message.SetString(kPage, "lso-loading");
-        break;
-      // Skip LSO. LSO and LSO_LOADING should be merged well.
-      // TODO(hidehiko): Do it.
-      case UIPage::START_PROGRESS:
-        message.SetString(kPage, "arc-loading");
-        break;
-      default:
-        NOTREACHED();
-        return;
+  message.SetString(kAction, kActionShowPage);
+  switch (ui_page) {
+    case UIPage::TERMS:
+      message.SetString(kPage, "terms");
+      break;
+    case UIPage::LSO:
+      // TODO(hidehiko): Merge LSO and LSO_LOADING.
+      message.SetString(kPage, "lso-loading");
+      break;
+    case UIPage::ARC_LOADING:
+      message.SetString(kPage, "arc-loading");
+      break;
+    default:
+      NOTREACHED();
+      return;
+  }
+  message_host_->SendMessage(message);
+}
+
+void ArcSupportHost::ShowError(Error error, bool should_show_send_feedback) {
+  ui_page_ = UIPage::ERROR;

[Luis Héctor Chávez, 2016/11/16 02:58:53]

Given that you only need these if |!message_host_|, how about moving these to
L225?

[hidehiko, 2016/11/16 17:52:21]

Ditto.

+  error_ = error;
+  should_show_send_feedback_ = should_show_send_feedback;
+  if (!message_host_) {
+    if (app_start_requested_) {
+      VLOG(2) << "ArcSupportHost::ShowError(" << error << ", "
+              << should_show_send_feedback << ") is called before connection "
+              << "to ARC support Chrome app is established.";
+      return;
     }
+    RequestAppStart();
+    return;
   }
+
+  base::DictionaryValue message;
+  message.SetString(kAction, kActionShowErrorPage);
+  int message_id;
+  switch (error) {
+    case Error::SIGN_IN_NETWORK_ERROR:
+      message_id = IDS_ARC_SIGN_IN_NETWORK_ERROR;
+      break;
+    case Error::SIGN_IN_SERVICE_UNAVAILABLE_ERROR:
+      message_id = IDS_ARC_SIGN_IN_SERVICE_UNAVAILABLE_ERROR;
+      break;
+    case Error::SIGN_IN_BAD_AUTHENTICATION_ERROR:
+      message_id = IDS_ARC_SIGN_IN_BAD_AUTHENTICATION_ERROR;
+      break;
+    case Error::SIGN_IN_GMS_NOT_AVAILABLE_ERROR:
+      message_id = IDS_ARC_SIGN_IN_GMS_NOT_AVAILABLE_ERROR;
+      break;
+    case Error::SIGN_IN_CLOUD_PROVISION_FLOW_FAIL_ERROR:
+      message_id = IDS_ARC_SIGN_IN_CLOUD_PROVISION_FLOW_FAIL_ERROR;
+      break;
+    case Error::SIGN_IN_UNKNOWN_ERROR:
+      message_id = IDS_ARC_SIGN_IN_UNKNOWN_ERROR;
+      break;
+    case Error::SERVER_COMMUNICATION_ERROR:
+      message_id = IDS_ARC_SERVER_COMMUNICATION_ERROR;
+      break;
+    case Error::ANDROID_MANAGEMENT_REQUIRED_ERROR:
+      message_id = IDS_ARC_ANDROID_MANAGEMENT_REQUIRED_ERROR;
+      break;
+  }
+  message.SetString(kErrorMessage, l10n_util::GetStringUTF16(message_id));
+  message.SetBoolean(kShouldShowSendFeedback, should_show_send_feedback);
   message_host_->SendMessage(message);
 }
 
@@ -193,6 +303,7 @@ void ArcSupportHost::SetMessageHost(arc::ArcSupportMessageHost* message_host) {
   if (message_host_ == message_host)
     return;
 
+  app_start_requested_ = false;
   if (message_host_)
     DisconnectMessageHost();
   message_host_ = message_host;
@@ -204,15 +315,23 @@ void ArcSupportHost::SetMessageHost(arc::ArcSupportMessageHost* message_host) {
     return;
   }
 
+  // Hereafter, install the requested ui state into the ARC support Chrome app.
+
+  // Set preference checkbox state.
   SendPreferenceCheckboxUpdate(kActionSetMetricsMode, metrics_checkbox_);
   SendPreferenceCheckboxUpdate(kActionBackupAndRestoreMode,
                                backup_and_restore_checkbox_);
   SendPreferenceCheckboxUpdate(kActionLocationServiceMode,
                                location_services_checkbox_);
 
-  arc::ArcAuthService* arc_auth_service = arc::ArcAuthService::Get();
-  DCHECK(arc_auth_service);
-  ShowPage(arc_auth_service->ui_page(), arc_auth_service->ui_page_status());
+  if (ui_page_ == UIPage::NO_PAGE) {
+    // Close() is called before opening the window.
+    Close();
+  } else if (ui_page_ == UIPage::ERROR) {
+    ShowError(error_, should_show_send_feedback_);
+  } else {
+    ShowPage(ui_page_);
+  }
 }
 
 void ArcSupportHost::UnsetMessageHost(
@@ -229,14 +348,25 @@ void ArcSupportHost::DisconnectMessageHost() {
   message_host_ = nullptr;
 }
 
+void ArcSupportHost::RequestAppStart() {
+  DCHECK(!message_host_);
+  DCHECK(!app_start_requested_);
+
+  app_start_requested_ = true;
+  const extensions::Extension* extension =
+      extensions::ExtensionRegistry::Get(profile_)->GetInstalledExtension(
+          kHostAppId);
+  CHECK(extension);

[Luis Héctor Chávez, 2016/11/16 02:58:53]

According to dcheng@, CHECK should never be used outside of base/, and DCHECK
should be used instead.

[hidehiko, 2016/11/16 17:52:21]

Done.

+  CHECK(extensions::util::IsAppLaunchable(kHostAppId, profile_));
+  OpenApplication(CreateAppLaunchParamsUserContainer(
+      profile_, extension, WindowOpenDisposition::NEW_WINDOW,
+      extensions::SOURCE_CHROME_INTERNAL));
+}
+
 bool ArcSupportHost::Initialize() {
   DCHECK(message_host_);
-  arc::ArcAuthService* arc_auth_service = arc::ArcAuthService::Get();
-  if (!arc_auth_service->IsAllowed())
-    return false;
 
-  std::unique_ptr<base::DictionaryValue> loadtime_data(
-      new base::DictionaryValue());
+  auto loadtime_data = base::MakeUnique<base::DictionaryValue>();
   base::string16 device_name = ash::GetChromeOSDeviceName();
   loadtime_data->SetString(
       "greetingHeader",
@@ -308,24 +438,25 @@ bool ArcSupportHost::Initialize() {
       "privacyPolicyLink",
       l10n_util::GetStringUTF16(IDS_ARC_OPT_IN_PRIVACY_POLICY_LINK));
 
-  const std::string& app_locale = g_browser_process->GetApplicationLocale();
+  loadtime_data->SetBoolean(kArcManaged, is_arc_managed_);
+  loadtime_data->SetBoolean("isOwnerProfile",
+                            chromeos::ProfileHelper::IsOwnerProfile(profile_));
+
   const std::string& country_code = base::CountryCodeForCurrentTimezone();
   loadtime_data->SetString("countryCode", country_code);
-  loadtime_data->SetBoolean(kArcManaged, arc_auth_service->IsArcManaged());
 
+  const std::string& app_locale = g_browser_process->GetApplicationLocale();
   webui::SetLoadTimeDataDefaults(app_locale, loadtime_data.get());
-  DCHECK(arc_auth_service);
-  const std::string device_id = user_manager::known_user::GetDeviceId(
-      multi_user_util::GetAccountIdFromProfile(arc_auth_service->profile()));
-  DCHECK(!device_id.empty());
-  loadtime_data->SetBoolean(
-      "isOwnerProfile",
-      chromeos::ProfileHelper::IsOwnerProfile(arc_auth_service->profile()));
 
   base::DictionaryValue message;
   message.SetString(kAction, kActionInitialize);
   message.Set(kData, std::move(loadtime_data));
+
+  const std::string device_id = user_manager::known_user::GetDeviceId(
+      multi_user_util::GetAccountIdFromProfile(profile_));
+  DCHECK(!device_id.empty());
   message.SetString(kDeviceId, device_id);
+
   message_host_->SendMessage(message);
   return true;
 }