diff options
author | jackhou@chromium.org <jackhou@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-06-21 16:50:20 +0000 |
---|---|---|
committer | jackhou@chromium.org <jackhou@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-06-21 16:50:20 +0000 |
commit | 936f09617f6273e508b66ec1e8d89bfaca2fd155 (patch) | |
tree | 86f93d7832c21a584cd6ab59c977666a49cb7530 /apps | |
parent | d46dc78255481d716d3be4da48f70a0bf06dabe6 (diff) | |
download | chromium_src-936f09617f6273e508b66ec1e8d89bfaca2fd155.zip chromium_src-936f09617f6273e508b66ec1e8d89bfaca2fd155.tar.gz chromium_src-936f09617f6273e508b66ec1e8d89bfaca2fd155.tar.bz2 |
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
Diffstat (limited to 'apps')
-rw-r--r-- | apps/app_shim/extension_app_shim_handler_mac.cc | 143 | ||||
-rw-r--r-- | apps/app_shim/extension_app_shim_handler_mac.h | 29 | ||||
-rw-r--r-- | apps/app_shim/extension_app_shim_handler_mac_unittest.cc | 125 |
3 files changed, 195 insertions, 102 deletions
diff --git a/apps/app_shim/extension_app_shim_handler_mac.cc b/apps/app_shim/extension_app_shim_handler_mac.cc index 21edc8a..eea475c 100644 --- a/apps/app_shim/extension_app_shim_handler_mac.cc +++ b/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, diff --git a/apps/app_shim/extension_app_shim_handler_mac.h b/apps/app_shim/extension_app_shim_handler_mac.h index 043a4d5..25d991e 100644 --- a/apps/app_shim/extension_app_shim_handler_mac.h +++ b/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_; diff --git a/apps/app_shim/extension_app_shim_handler_mac_unittest.cc b/apps/app_shim/extension_app_shim_handler_mac_unittest.cc index 6f650e6..a987325 100644 --- a/apps/app_shim/extension_app_shim_handler_mac_unittest.cc +++ b/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 |