Extract ArcSupportMessageHost.

chrome/browser/chromeos/arc/arc_auth_service.cc

 // Copyright 2015 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_auth_service.h"
 
 #include <utility>
 
 #include "ash/common/shelf/shelf_delegate.h"
 #include "ash/common/wm_shell.h"
@@ skipping 451 matching lines @@
   if (!IsAllowedForProfile(profile))
     return;
 
   // TODO(khmel): Move this to IsAllowedForProfile.
   if (IsArcDisabledForEnterprise() && IsAccountManaged(profile)) {
     VLOG(2) << "Enterprise users are not supported in ARC.";
     return;
   }
 
   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(
       prefs::kArcEnabled, this);
 
   context_.reset(new ArcAuthContext(this, profile_));
 
   // In case UI is disabled we assume that ARC is opted-in.
   if (IsOptInVerificationDisabled()) {
     auth_code_.clear();
@@ skipping 61 matching lines @@
   if (app_window_registry->GetCurrentAppWindowForApp(
           ArcSupportHost::kHostAppId)) {
     return;
   }
 
   const extensions::Extension* extension =
       extensions::ExtensionRegistry::Get(profile_)->GetInstalledExtension(
           ArcSupportHost::kHostAppId);
   CHECK(extension && extensions::util::IsAppLaunchable(
                          ArcSupportHost::kHostAppId, profile_));
-
   OpenApplication(CreateAppLaunchParamsUserContainer(
       profile_, extension, WindowOpenDisposition::NEW_WINDOW,
       extensions::SOURCE_CHROME_INTERNAL));
 }
 
 void ArcAuthService::OnContextReady() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   DCHECK(!initial_opt_in_);
   CheckAndroidManagement(false);
@@ skipping 96 matching lines @@
 }
 
 void ArcAuthService::RemoveObserver(Observer* observer) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   observer_list_.RemoveObserver(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();
 }
 
 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.
 // TODO(hidehiko): Remove this.
 void ArcAuthService::StopAndEnableArc() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(!arc_bridge_service()->stopped());
   reenable_arc_ = true;
   StopArc();
 }
@@ skipping 252 matching lines @@
       return os << kStateFetchingCode;
     case ArcAuthService::State::ACTIVE:
       return os << kStateActive;
     default:
       NOTREACHED();
       return os;
   }
 }
 
 }  // namespace arc