Restructure ExtensionAppShimHandler testing.

This puts all dependencies into a inner Delegate class so the test can
mock them out.

BUG=168080

Review URL: https://chromiumcodereview.appspot.com/17481002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@207862 0039d316-1c4b-4281-b951-d872f2087c98

apps/app_shim/extension_app_shim_handler_mac.cc

@@ -26,26 +26,11 @@
 #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
@@ -55,15 +40,48 @@ bool ExtensionAppShimHandler::ProfileManagerFacade::ProfileExistsForPath(
   return cache.GetIndexOfProfileWithPath(full_path) != std::string::npos;
 }
 
-Profile* ExtensionAppShimHandler::ProfileManagerFacade::ProfileForPath(
+Profile* ExtensionAppShimHandler::Delegate::ProfileForPath(
     const base::FilePath& path) {
   ProfileManager* profile_manager = g_browser_process->profile_manager();
   base::FilePath full_path = profile_manager->user_data_dir().Append(path);
   return profile_manager->GetProfile(full_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::GetAppExtension(
+    Profile* profile,
+    const std::string& extension_id) {
+  ExtensionService* extension_service =
+      extensions::ExtensionSystem::Get(profile)->extension_service();
+  DCHECK(extension_service);
+  const extensions::Extension* extension =
+      extension_service->GetExtensionById(extension_id, false);
+  return extension && extension->is_platform_app() ? extension : NULL;
+}
+
+void ExtensionAppShimHandler::Delegate::LaunchApp(
+    Profile* profile,
+    const extensions::Extension* extension) {
+  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
@@ -81,7 +99,7 @@ bool ExtensionAppShimHandler::OnShimLaunch(Host* host,
   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 '"
@@ -89,7 +107,7 @@ bool ExtensionAppShimHandler::OnShimLaunch(Host* host,
     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)) {
@@ -98,8 +116,28 @@ bool ExtensionAppShimHandler::OnShimLaunch(Host* host,
   }
 
   // TODO(jackhou): Add some UI for this case and remove the LOG.
-  if (!LaunchApp(profile, app_id, launch_type))
+  const extensions::Extension* extension =
+      delegate_->GetAppExtension(profile, app_id);
+  if (!extension) {
+    LOG(ERROR) << "Attempted to launch nonexistent app with id '"
+               << app_id << "'.";
+    return false;
+  }
+
+  // If the shim was launched in response to a window appearing, but the window
+  // is closed by the time the shim process launched, return false to close the
+  // shim.
+  if (launch_type == APP_SHIM_LAUNCH_REGISTER_ONLY &&
+      delegate_->GetWindows(profile, app_id).empty()) {
     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);
 
   // The first host to claim this (profile, app_id) becomes the main host.
   // For any others, focus the app and return false.
@@ -112,10 +150,8 @@ bool ExtensionAppShimHandler::OnShimLaunch(Host* host,
 }
 
 void ExtensionAppShimHandler::OnShimClose(Host* host) {
-  DCHECK(profile_manager_facade_->ProfileExistsForPath(
-      host->GetProfilePath()));
-  Profile* profile =
-      profile_manager_facade_->ProfileForPath(host->GetProfilePath());
+  DCHECK(delegate_->ProfileExistsForPath(host->GetProfilePath()));
+  Profile* profile = delegate_->ProfileForPath(host->GetProfilePath());
 
   HostMap::iterator it = hosts_.find(make_pair(profile, host->GetAppId()));
   // Any hosts other than the main host will still call OnShimClose, so ignore
@@ -125,12 +161,11 @@ void ExtensionAppShimHandler::OnShimClose(Host* host) {
 }
 
 void ExtensionAppShimHandler::OnShimFocus(Host* host) {
-  DCHECK(profile_manager_facade_->ProfileExistsForPath(
-      host->GetProfilePath()));
-  Profile* profile =
-      profile_manager_facade_->ProfileForPath(host->GetProfilePath());
+  DCHECK(delegate_->ProfileExistsForPath(host->GetProfilePath()));
+  Profile* profile = 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) {
@@ -140,43 +175,19 @@ void ExtensionAppShimHandler::OnShimFocus(Host* host) {
 }
 
 void ExtensionAppShimHandler::OnShimQuit(Host* host) {
-  DCHECK(profile_manager_facade_->ProfileExistsForPath(
-      host->GetProfilePath()));
-  Profile* profile =
-      profile_manager_facade_->ProfileForPath(host->GetProfilePath());
+  DCHECK(delegate_->ProfileExistsForPath(host->GetProfilePath()));
+  Profile* profile = 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_delegate(Delegate* delegate) {
+  delegate_.reset(delegate);
 }
 
 void ExtensionAppShimHandler::Observe(
@@ -213,15 +224,15 @@ void ExtensionAppShimHandler::OnAppStart(Profile* profile,
 
 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_->GetAppExtension(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,

apps/app_shim/extension_app_shim_handler_mac.h

@@ -11,6 +11,7 @@
 #include "apps/app_lifetime_monitor.h"
 #include "apps/app_shim/app_shim_handler_mac.h"
 #include "base/memory/scoped_ptr.h"
+#include "chrome/browser/extensions/shell_window_registry.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 
@@ -20,6 +21,10 @@ namespace base {
 class FilePath;
 }
 
+namespace content {
+class WebContents;
+}
+
 namespace extensions {
 class Extension;
 }
@@ -32,11 +37,23 @@ class ExtensionAppShimHandler : public AppShimHandler,
                                 public content::NotificationObserver,
                                 public AppLifetimeMonitor::Observer {
  public:
-  class ProfileManagerFacade {
+  class Delegate {
    public:
-    virtual ~ProfileManagerFacade() {}
+    virtual ~Delegate() {}
+
     virtual bool ProfileExistsForPath(const base::FilePath& path);
     virtual Profile* ProfileForPath(const base::FilePath& path);
+
+    virtual extensions::ShellWindowRegistry::ShellWindowList GetWindows(
+        Profile* profile, const std::string& extension_id);
+
+    virtual const extensions::Extension* GetAppExtension(
+        Profile* profile, const std::string& extension_id);
+    virtual void LaunchApp(Profile* profile,
+                           const extensions::Extension* extension);
+    virtual void LaunchShim(Profile* profile,
+                            const extensions::Extension* extension);
+
   };
 
   ExtensionAppShimHandler();
@@ -62,15 +79,11 @@ class ExtensionAppShimHandler : public AppShimHandler,
       HostMap;
 
   // Exposed for testing.
-  void set_profile_manager_facade(ProfileManagerFacade* profile_manager_facade);
+  void set_delegate(Delegate* delegate);
   HostMap& hosts() { return hosts_; }
   content::NotificationRegistrar& registrar() { return registrar_; }
 
  private:
-  virtual bool LaunchApp(Profile* profile,
-                         const std::string& app_id,
-                         AppShimLaunchType launch_type);
-
   // Listen to the NOTIFICATION_EXTENSION_HOST_DESTROYED message to detect when
   // an app closes. When that happens, call OnAppClosed on the relevant
   // AppShimHandler::Host which causes the app shim process to quit.
@@ -79,7 +92,7 @@ class ExtensionAppShimHandler : public AppShimHandler,
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE;
 
-  scoped_ptr<ProfileManagerFacade> profile_manager_facade_;
+  scoped_ptr<Delegate> delegate_;
 
   HostMap hosts_;
 

apps/app_shim/extension_app_shim_handler_mac_unittest.cc

@@ -7,6 +7,7 @@
 #include "apps/app_shim/app_shim_host_mac.h"
 #include "base/memory/scoped_ptr.h"
 #include "chrome/common/chrome_notification_types.h"
+#include "chrome/common/extensions/extension.h"
 #include "chrome/test/base/testing_profile.h"
 #include "content/public/browser/notification_service.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -14,27 +15,35 @@
 
 namespace apps {
 
+using extensions::Extension;
+typedef extensions::ShellWindowRegistry::ShellWindowList ShellWindowList;
+
+using ::testing::_;
 using ::testing::Return;
 
-class MockProfileManagerFacade
-    : public ExtensionAppShimHandler::ProfileManagerFacade {
+class MockDelegate : public ExtensionAppShimHandler::Delegate {
  public:
-  virtual ~MockProfileManagerFacade() {}
+  virtual ~MockDelegate() {}
+
+  MOCK_METHOD1(ProfileExistsForPath, bool(const base::FilePath&));
+  MOCK_METHOD1(ProfileForPath, Profile*(const base::FilePath&));
+
+  MOCK_METHOD2(GetWindows, ShellWindowList(Profile*, const std::string&));
+
+  MOCK_METHOD2(GetAppExtension, const Extension*(Profile*, const std::string&));
+  MOCK_METHOD2(LaunchApp, void(Profile*, const Extension*));
+  MOCK_METHOD2(LaunchShim, void(Profile*, const Extension*));
 
-  MOCK_METHOD1(ProfileExistsForPath, bool(const base::FilePath& path));
-  MOCK_METHOD1(ProfileForPath, Profile*(const base::FilePath& path));
 };
 
 class TestingExtensionAppShimHandler : public ExtensionAppShimHandler {
  public:
-  TestingExtensionAppShimHandler(ProfileManagerFacade* profile_manager_facade) {
-    set_profile_manager_facade(profile_manager_facade);
+  TestingExtensionAppShimHandler(Delegate* delegate) {
+    set_delegate(delegate);
   }
   virtual ~TestingExtensionAppShimHandler() {}
 
-  MOCK_METHOD3(LaunchApp, bool(Profile*,
-                               const std::string&,
-                               AppShimLaunchType));
+  MOCK_METHOD1(OnShimFocus, void(Host* host));
 
   AppShimHandler::Host* FindHost(Profile* profile,
                                  const std::string& app_id) {
@@ -84,25 +93,54 @@ class FakeHost : public apps::AppShimHandler::Host {
 class ExtensionAppShimHandlerTest : public testing::Test {
  protected:
   ExtensionAppShimHandlerTest()
-      : profile_manager_facade_(new MockProfileManagerFacade),
-        handler_(new TestingExtensionAppShimHandler(profile_manager_facade_)),
+      : delegate_(new MockDelegate),
+        handler_(new TestingExtensionAppShimHandler(delegate_)),
         profile_path_a_("Profile A"),
         profile_path_b_("Profile B"),
         host_aa_(profile_path_a_, kTestAppIdA, handler_.get()),
         host_ab_(profile_path_a_, kTestAppIdB, handler_.get()),
         host_bb_(profile_path_b_, kTestAppIdB, handler_.get()),
         host_aa_duplicate_(profile_path_a_, kTestAppIdA, handler_.get()) {
-    EXPECT_CALL(*profile_manager_facade_, ProfileExistsForPath(profile_path_a_))
+    base::FilePath extension_path("/fake/path");
+    base::DictionaryValue manifest;
+    manifest.SetString("name", "Fake Name");
+    manifest.SetString("version", "1");
+    std::string error;
+    extension_a_ = Extension::Create(
+        extension_path, extensions::Manifest::INTERNAL, manifest,
+        Extension::NO_FLAGS, kTestAppIdA, &error);
+    EXPECT_TRUE(extension_a_.get()) << error;
+
+    extension_b_ = Extension::Create(
+        extension_path, extensions::Manifest::INTERNAL, manifest,
+        Extension::NO_FLAGS, kTestAppIdB, &error);
+    EXPECT_TRUE(extension_b_.get()) << error;
+
+    EXPECT_CALL(*delegate_, ProfileExistsForPath(profile_path_a_))
         .WillRepeatedly(Return(true));
-    EXPECT_CALL(*profile_manager_facade_, ProfileForPath(profile_path_a_))
+    EXPECT_CALL(*delegate_, ProfileForPath(profile_path_a_))
         .WillRepeatedly(Return(&profile_a_));
-    EXPECT_CALL(*profile_manager_facade_, ProfileExistsForPath(profile_path_b_))
+    EXPECT_CALL(*delegate_, ProfileExistsForPath(profile_path_b_))
         .WillRepeatedly(Return(true));
-    EXPECT_CALL(*profile_manager_facade_, ProfileForPath(profile_path_b_))
+    EXPECT_CALL(*delegate_, ProfileForPath(profile_path_b_))
         .WillRepeatedly(Return(&profile_b_));
+
+    // In most tests, we don't care about the result of GetWindows, it just
+    // needs to be non-empty.
+    ShellWindowList shell_window_list;
+    shell_window_list.push_back(static_cast<ShellWindow*>(NULL));
+    EXPECT_CALL(*delegate_, GetWindows(_, _))
+        .WillRepeatedly(Return(shell_window_list));
+
+    EXPECT_CALL(*delegate_, GetAppExtension(_, kTestAppIdA))
+        .WillRepeatedly(Return(extension_a_.get()));
+    EXPECT_CALL(*delegate_, GetAppExtension(_, kTestAppIdB))
+        .WillRepeatedly(Return(extension_b_.get()));
+    EXPECT_CALL(*delegate_, LaunchApp(_,_))
+        .WillRepeatedly(Return());
   }
 
-  MockProfileManagerFacade* profile_manager_facade_;
+  MockDelegate* delegate_;
   scoped_ptr<TestingExtensionAppShimHandler> handler_;
   base::FilePath profile_path_a_;
   base::FilePath profile_path_b_;
@@ -112,6 +150,8 @@ class ExtensionAppShimHandlerTest : public testing::Test {
   FakeHost host_ab_;
   FakeHost host_bb_;
   FakeHost host_aa_duplicate_;
+  scoped_refptr<Extension> extension_a_;
+  scoped_refptr<Extension> extension_b_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ExtensionAppShimHandlerTest);
@@ -121,30 +161,24 @@ TEST_F(ExtensionAppShimHandlerTest, LaunchAndCloseShim) {
   const AppShimLaunchType normal_launch = APP_SHIM_LAUNCH_NORMAL;
 
   // If launch fails, the host is not added to the map.
-  EXPECT_CALL(*handler_, LaunchApp(&profile_a_, kTestAppIdA, normal_launch))
-      .WillOnce(Return(false));
+  EXPECT_CALL(*delegate_, GetAppExtension(&profile_a_, kTestAppIdA))
+      .WillOnce(Return(static_cast<const Extension*>(NULL)))
+      .WillRepeatedly(Return(extension_a_.get()));
   EXPECT_EQ(false, handler_->OnShimLaunch(&host_aa_, normal_launch));
   EXPECT_FALSE(handler_->FindHost(&profile_a_, kTestAppIdA));
 
   // Normal startup.
-  EXPECT_CALL(*handler_, LaunchApp(&profile_a_, kTestAppIdA, normal_launch))
-      .WillOnce(Return(true));
   EXPECT_EQ(true, handler_->OnShimLaunch(&host_aa_, normal_launch));
   EXPECT_EQ(&host_aa_, handler_->FindHost(&profile_a_, kTestAppIdA));
 
-  EXPECT_CALL(*handler_, LaunchApp(&profile_a_, kTestAppIdB, normal_launch))
-      .WillOnce(Return(true));
   EXPECT_EQ(true, handler_->OnShimLaunch(&host_ab_, normal_launch));
   EXPECT_EQ(&host_ab_, handler_->FindHost(&profile_a_, kTestAppIdB));
 
-  EXPECT_CALL(*handler_, LaunchApp(&profile_b_, kTestAppIdB, normal_launch))
-      .WillOnce(Return(true));
   EXPECT_EQ(true, handler_->OnShimLaunch(&host_bb_, normal_launch));
   EXPECT_EQ(&host_bb_, handler_->FindHost(&profile_b_, kTestAppIdB));
 
-  // Starting and closing a second host does nothing.
-  EXPECT_CALL(*handler_, LaunchApp(&profile_a_, kTestAppIdA, normal_launch))
-      .WillOnce(Return(false));
+  // Starting and closing a second host just focuses the app.
+  EXPECT_CALL(*handler_, OnShimFocus(&host_aa_duplicate_));
   EXPECT_EQ(false, handler_->OnShimLaunch(&host_aa_duplicate_, normal_launch));
   EXPECT_EQ(&host_aa_, handler_->FindHost(&profile_a_, kTestAppIdA));
   handler_->OnShimClose(&host_aa_duplicate_);
@@ -159,4 +193,39 @@ TEST_F(ExtensionAppShimHandlerTest, LaunchAndCloseShim) {
   EXPECT_FALSE(handler_->FindHost(&profile_a_, kTestAppIdA));
 }
 
+TEST_F(ExtensionAppShimHandlerTest, AppLifetime) {
+  EXPECT_CALL(*delegate_, LaunchShim(&profile_a_, extension_a_.get()));
+  handler_->OnAppActivated(&profile_a_, kTestAppIdA);
+
+  // Launch the shim, adding an entry in the map.
+  EXPECT_EQ(true, handler_->OnShimLaunch(&host_aa_, APP_SHIM_LAUNCH_NORMAL));
+  EXPECT_EQ(&host_aa_, handler_->FindHost(&profile_a_, kTestAppIdA));
+
+  handler_->OnAppDeactivated(&profile_a_, kTestAppIdA);
+  EXPECT_EQ(1, host_aa_.close_count());
+}
+
+TEST_F(ExtensionAppShimHandlerTest, RegisterOnly) {
+  // For an APP_SHIM_LAUNCH_REGISTER_ONLY, don't launch the app.
+  EXPECT_CALL(*delegate_, LaunchApp(_, _))
+      .Times(0);
+  EXPECT_EQ(true, handler_->OnShimLaunch(&host_aa_,
+                                          APP_SHIM_LAUNCH_REGISTER_ONLY));
+  EXPECT_TRUE(handler_->FindHost(&profile_a_, kTestAppIdA));
+
+  // Close the shim, removing the entry in the map.
+  handler_->OnShimClose(&host_aa_);
+  EXPECT_FALSE(handler_->FindHost(&profile_a_, kTestAppIdA));
+
+  // Don't register if there are no windows open for the app. This can happen if
+  // the app shim was launched in response to an app being activated, but the
+  // app was deactivated before the shim is registered.
+  ShellWindowList shell_window_list;
+  EXPECT_CALL(*delegate_, GetWindows(&profile_a_, kTestAppIdA))
+      .WillRepeatedly(Return(shell_window_list));
+  EXPECT_EQ(false, handler_->OnShimLaunch(&host_aa_,
+                                          APP_SHIM_LAUNCH_REGISTER_ONLY));
+  EXPECT_FALSE(handler_->FindHost(&profile_a_, kTestAppIdA));
+}
+
 }  // namespace apps