Restructure ExtensionAppShimHandler testing.

apps/app_shim/extension_app_shim_handler_mac.cc

 // Copyright 2013 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 "apps/app_shim/extension_app_shim_handler_mac.h"
 
 #include "apps/app_lifetime_monitor_factory.h"
 #include "apps/app_shim/app_shim_messages.h"
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/extensions/extension_host.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/extensions/shell_window_registry.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/extensions/application_launch.h"
 #include "chrome/browser/ui/extensions/native_app_window.h"
 #include "chrome/browser/ui/extensions/shell_window.h"
 #include "chrome/browser/ui/web_applications/web_app_ui.h"
 #include "chrome/browser/web_applications/web_app_mac.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
 #include "ui/base/cocoa/focus_window_set.h"
 
-namespace {
+namespace apps {
 
 typedef extensions::ShellWindowRegistry::ShellWindowList ShellWindowList;
 
-ShellWindowList GetWindows(Profile* profile, const std::string& extension_id) {
-  return extensions::ShellWindowRegistry::Get(profile)->
-      GetShellWindowsForApp(extension_id);
-}
-
-const extensions::Extension* GetExtension(Profile* profile,
-                                          const std::string& extension_id) {
-  return extensions::ExtensionSystem::Get(profile)->extension_service()->
-      GetExtensionById(extension_id, false);
-}
-
-}  // namespace
-
-namespace apps {
-
-bool ExtensionAppShimHandler::ProfileManagerFacade::ProfileExistsForPath(
+bool ExtensionAppShimHandler::Delegate::ProfileExistsForPath(
     const base::FilePath& path) {
   ProfileManager* profile_manager = g_browser_process->profile_manager();
   // Check for the profile name in the profile info cache to ensure that we
   // never access any directory that isn't a known profile.
   base::FilePath full_path = profile_manager->user_data_dir().Append(path);
   ProfileInfoCache& cache = profile_manager->GetProfileInfoCache();
   return cache.GetIndexOfProfileWithPath(full_path) != std::string::npos;
 }
 
-Profile* ExtensionAppShimHandler::ProfileManagerFacade::ProfileForPath(
+Profile* ExtensionAppShimHandler::Delegate::ProfileForPath(
     const base::FilePath& path) {
   return g_browser_process->profile_manager()->GetProfile(path);
 }
 
+ShellWindowList ExtensionAppShimHandler::Delegate::GetWindows(
+    Profile* profile,
+    const std::string& extension_id) {
+  return extensions::ShellWindowRegistry::Get(profile)->
+      GetShellWindowsForApp(extension_id);
+}
+
+const extensions::Extension*
+ExtensionAppShimHandler::Delegate::GetExtension(

[tapted, 2013/06/21 04:30:47]

since the is_platform_app check is moving in here, this might need a new name.
GetAppExtension maybe?

[jackhou1, 2013/06/21 05:46:27]

Done.

+    Profile* profile,
+    const std::string& extension_id) {
+  const extensions::Extension* extension =
+      extensions::ExtensionSystem::Get(profile)->extension_service()->

[tapted, 2013/06/21 04:30:47]

Should DCHECK here that that extension_service is non-null. Will need another
temporary, but that will help split up the line anyway.

[jackhou1, 2013/06/21 05:46:27]

Done.

+          GetExtensionById(extension_id, false);
+  return extension && extension->is_platform_app() ? extension : NULL;
+}
+
+content::WebContents* ExtensionAppShimHandler::Delegate::LaunchApp(
+    Profile* profile,
+    const extensions::Extension* extension) {
+  return chrome::OpenApplication(chrome::AppLaunchParams(
+      profile, extension, NEW_FOREGROUND_TAB));
+}
+
+void ExtensionAppShimHandler::Delegate::LaunchShim(
+    Profile* profile,
+    const extensions::Extension* extension) {
+  web_app::MaybeLaunchShortcut(
+      web_app::ShortcutInfoForExtensionAndProfile(extension, profile));
+}
+
 ExtensionAppShimHandler::ExtensionAppShimHandler()
-    : profile_manager_facade_(new ProfileManagerFacade) {
+    : delegate_(new Delegate) {
   // This is instantiated in BrowserProcessImpl::PreMainMessageLoopRun with
   // AppShimHostManager. Since PROFILE_CREATED is not fired until
   // ProfileManager::GetLastUsedProfile/GetLastOpenedProfiles, this should catch
   // notifications for all profiles.
   registrar_.Add(this, chrome::NOTIFICATION_PROFILE_CREATED,
                  content::NotificationService::AllBrowserContextsAndSources());
   registrar_.Add(this, chrome::NOTIFICATION_PROFILE_DESTROYED,
                  content::NotificationService::AllBrowserContextsAndSources());
 }
 
 ExtensionAppShimHandler::~ExtensionAppShimHandler() {}
 
 bool ExtensionAppShimHandler::OnShimLaunch(Host* host,
                                            AppShimLaunchType launch_type) {
   const base::FilePath& profile_path = host->GetProfilePath();
   DCHECK(!profile_path.empty());
 
-  if (!profile_manager_facade_->ProfileExistsForPath(profile_path)) {
+  if (!delegate_->ProfileExistsForPath(profile_path)) {
     // User may have deleted the profile this shim was originally created for.
     // TODO(jackhou): Add some UI for this case and remove the LOG.
     LOG(ERROR) << "Requested directory is not a known profile '"
                << profile_path.value() << "'.";
     return false;
   }
 
-  Profile* profile = profile_manager_facade_->ProfileForPath(profile_path);
+  Profile* profile = delegate_->ProfileForPath(profile_path);
 
   const std::string& app_id = host->GetAppId();
   if (!extensions::Extension::IdIsValid(app_id)) {
     LOG(ERROR) << "Bad app ID from app shim launch message.";
     return false;
   }
 
   // TODO(jackhou): Add some UI for this case and remove the LOG.
-  if (!LaunchApp(profile, app_id, launch_type))
+  const extensions::Extension* extension =
+      delegate_->GetExtension(profile, app_id);
+  if (!extension) {
+    LOG(ERROR) << "Attempted to launch nonexistent app with id '"
+               << app_id << "'.";
     return false;
+  }
+
+  if (launch_type == APP_SHIM_LAUNCH_REGISTER_ONLY &&

[tapted, 2013/06/21 04:30:47]

needs curlies if the condition is multi-line

[jackhou1, 2013/06/21 05:46:27]

Done.

+      delegate_->GetWindows(profile, app_id).empty())

[tapted, 2013/06/21 04:30:47]

Should this be !empty()?

Maybe a comment, like ~"if the shim was launched in response to a window
appearing, but it closed by the time the shim process launched, shut it down."

[jackhou1, 2013/06/21 05:46:27]

It should be empty(), because it returns false if there are no windows. Added
comment.

+    return false;
+
+  // TODO(jeremya): Handle the case that launching the app fails. Probably we
+  // need to watch for 'app successfully launched' or at least 'background page
+  // exists/was created' and time out with failure if we don't see that sign of
+  // life within a certain window.
+  if (launch_type == APP_SHIM_LAUNCH_NORMAL)
+    delegate_->LaunchApp(profile, extension);

[tapted, 2013/06/21 04:30:47]

Should it return false if LaunchApp returns NULL here?

[jackhou1, 2013/06/21 05:46:27]

Turns out LaunchApplication always returns NULL for platform apps.

 
   // The first host to claim this (profile, app_id) becomes the main host.
   // For any others, focus the app and return false.
   if (!hosts_.insert(make_pair(make_pair(profile, app_id), host)).second) {
     OnShimFocus(host);
     return false;
   }
 
   return true;
 }
 
 void ExtensionAppShimHandler::OnShimClose(Host* host) {
-  DCHECK(profile_manager_facade_->ProfileExistsForPath(
+  DCHECK(delegate_->ProfileExistsForPath(

[tapted, 2013/06/21 04:30:47]

nit: can this fit on the line above now?

[jackhou1, 2013/06/21 05:46:27]

Done.

       host->GetProfilePath()));
   Profile* profile =
-      profile_manager_facade_->ProfileForPath(host->GetProfilePath());
+      delegate_->ProfileForPath(host->GetProfilePath());

[tapted, 2013/06/21 04:30:47]

nit: one line?

up to 6 more below too

[jackhou1, 2013/06/21 05:46:27]

Done.

 
   HostMap::iterator it = hosts_.find(make_pair(profile, host->GetAppId()));
   // Any hosts other than the main host will still call OnShimClose, so ignore
   // them.
   if (it != hosts_.end() && it->second == host)
     hosts_.erase(it);
 }
 
 void ExtensionAppShimHandler::OnShimFocus(Host* host) {
-  DCHECK(profile_manager_facade_->ProfileExistsForPath(
+  DCHECK(delegate_->ProfileExistsForPath(
       host->GetProfilePath()));
   Profile* profile =
-      profile_manager_facade_->ProfileForPath(host->GetProfilePath());
+      delegate_->ProfileForPath(host->GetProfilePath());
 
-  const ShellWindowList windows = GetWindows(profile, host->GetAppId());
+  const ShellWindowList windows =
+      delegate_->GetWindows(profile, host->GetAppId());
   std::set<gfx::NativeWindow> native_windows;
   for (ShellWindowList::const_iterator it = windows.begin();
        it != windows.end(); ++it) {
     native_windows.insert((*it)->GetNativeWindow());
   }
   ui::FocusWindowSet(native_windows);
 }
 
 void ExtensionAppShimHandler::OnShimQuit(Host* host) {
-  DCHECK(profile_manager_facade_->ProfileExistsForPath(
+  DCHECK(delegate_->ProfileExistsForPath(
       host->GetProfilePath()));
   Profile* profile =
-      profile_manager_facade_->ProfileForPath(host->GetProfilePath());
+      delegate_->ProfileForPath(host->GetProfilePath());
 
-  const ShellWindowList windows = GetWindows(profile, host->GetAppId());
+  const ShellWindowList windows =
+      delegate_->GetWindows(profile, host->GetAppId());
   for (extensions::ShellWindowRegistry::const_iterator it = windows.begin();
        it != windows.end(); ++it) {
     (*it)->GetBaseWindow()->Close();
   }
 }
 
-void ExtensionAppShimHandler::set_profile_manager_facade(
-    ProfileManagerFacade* profile_manager_facade) {
-  profile_manager_facade_.reset(profile_manager_facade);
-}
-
-bool ExtensionAppShimHandler::LaunchApp(Profile* profile,
-                                        const std::string& app_id,
-                                        AppShimLaunchType launch_type) {
-  const extensions::Extension* extension = GetExtension(profile, app_id);
-  if (!extension) {
-    LOG(ERROR) << "Attempted to launch nonexistent app with id '"
-               << app_id << "'.";
-    return false;
-  }
-
-  if (launch_type == APP_SHIM_LAUNCH_REGISTER_ONLY)
-    return !GetWindows(profile, app_id).empty();
-
-  // TODO(jeremya): Handle the case that launching the app fails. Probably we
-  // need to watch for 'app successfully launched' or at least 'background page
-  // exists/was created' and time out with failure if we don't see that sign of
-  // life within a certain window.
-  chrome::OpenApplication(chrome::AppLaunchParams(
-      profile, extension, NEW_FOREGROUND_TAB));
-  return true;
+void ExtensionAppShimHandler::set_dependency_facade(
+    Delegate* dependency_facade) {
+  delegate_.reset(dependency_facade);
 }
 
 void ExtensionAppShimHandler::Observe(
     int type,
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
   Profile* profile = content::Source<Profile>(source).ptr();
   if (profile->IsOffTheRecord())
     return;
 
@@ skipping 16 matching lines @@
       NOTREACHED();  // Unexpected notification.
       break;
   }
 }
 
 void ExtensionAppShimHandler::OnAppStart(Profile* profile,
                                          const std::string& app_id) {}
 
 void ExtensionAppShimHandler::OnAppActivated(Profile* profile,
                                              const std::string& app_id) {
-  const extensions::Extension* extension = GetExtension(profile, app_id);
-  if (!extension || !extension->is_platform_app())
+  const extensions::Extension* extension =
+      delegate_->GetExtension(profile, app_id);
+  if (!extension)
     return;
 
-  if (hosts_.count(make_pair(profile, extension->id())))
+  if (hosts_.count(make_pair(profile, app_id)))
     return;
 
-  web_app::MaybeLaunchShortcut(
-      web_app::ShortcutInfoForExtensionAndProfile(extension, profile));
+  delegate_->LaunchShim(profile, extension);
 }
 
 void ExtensionAppShimHandler::OnAppDeactivated(Profile* profile,
                                                const std::string& app_id) {
   HostMap::const_iterator it = hosts_.find(make_pair(profile, app_id));
   if (it != hosts_.end())
     it->second->OnAppClosed();
 }
 
 void ExtensionAppShimHandler::OnAppStop(Profile* profile,
                                         const std::string& app_id) {}
 
 void ExtensionAppShimHandler::OnChromeTerminating() {}
 
 }  // namespace apps