Extract ArcSupportMessageHost.

chrome/browser/chromeos/arc/arc_auth_service.cc

Index: chrome/browser/chromeos/arc/arc_auth_service.cc
diff --git a/chrome/browser/chromeos/arc/arc_auth_service.cc b/chrome/browser/chromeos/arc/arc_auth_service.cc
index a5e680699ccbe8b9f55c2b1aa7be7e9d142f4d2a..d448581e753b29cdccfce18d970397f324f876e3 100644
--- a/chrome/browser/chromeos/arc/arc_auth_service.cc
+++ b/chrome/browser/chromeos/arc/arc_auth_service.cc
@@ -469,6 +469,7 @@ void ArcAuthService::OnPrimaryUserProfilePrepared(Profile* profile) {
   }
 
   profile_ = profile;
+  support_host_.reset(new ArcSupportHost());

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

I would recommend to create it on demand. In most cases, UI is not shown.

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

Ah, yes it can be in theory, but the current code complexity is making it
difficult. Specifically, I'm moving more UI related code to ArcSupportHost so
that the timing gets more and more complicated.

So, let me keep it as is for now, and let's revisit here after more refactoring,
so that life-time management can be easier, I think. Added TODO.

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

Took another look. arc_support_host is not trivial, it register pref observers.
Having this functionality on for all cases is too risky and wasteful. How about
creating support host on demand for external consumers?

ArcSupportHost* GetSupportHost() {
  if (!support_host) {
     ....
  }
  return support_host;
}

IMHO You can get rid from additional TODO

// TODO(hidehiko): This if statement is only for testing. Clean up this.
if (g_browser_process->local_state())
...

[hidehiko, 2016/10/24 19:00:58]

I agree that observing pref can be wasteful, but it looks small enough?
Also, could you explain what's the risk there?

Anyway, I agreed that observing pref in ArcSupportHost is not a good way.
Actually, it will be removed from ArcSupportHost with your pending CL ;-). So,
the TODO will be addressed in near future. Considering the code change size,
let's work on it separately.

Also, introducing GetSupportHost() needs singleton-ish mechanism, which Luis and
I agreed that we should avoid. Or, we need a new UI code in ArcAuthService,
which I'm actually removing. So, I'll avoid it.

   SetState(State::STOPPED);
 
   PrefServiceSyncableFromProfile(profile_)->AddSyncedPrefObserver(
@@ -550,7 +551,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));
@@ -667,8 +667,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();
 }
@@ -676,8 +676,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_)

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

In my previous comment I recommend to create host on demand.
If this is not possible, IMHO this check is redundant and we can change to
DCHECK()

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

Unfortunately, it cannot, specifically for CloseUI(), which is called in
Shutdown() called in OnPrimaryUserProfilePrepared(). I do want to be consistent
SetUIPage() and CloseUI(), so let me keep this as is now.

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

Acknowledged, however it would be nice to have LOG(ERROR) if we try to show
something and cannot dur missing the host. At least we would have log record
that will help to analyze the problem.

[hidehiko, 2016/10/24 19:00:58]

Adding log makes sense. But, as I said, it is not an error so LOG(ERROR) sounds
too strong. Let's use VLOG(2) instead, which is used for normal code path in
c/b/c/arc.

+    support_host_->ShowPage(ui_page_, ui_page_status_);
 }
 
 // This is the special method to support enterprise mojo API.