Start and close app shims based on shell windows.

Start an app's shim when the first shell window is opened instead of
when the extension host is created. Close the shim when the last shell
window is closed. This makes the app shim more consistent with users'
expectations of when the app is 'open'.

BUG=168080
TEST=Set --enable-app-shims.
Find an app that does not open a shell window on launch.
'Create shortcut' to ensure the app shim exists.
Launching the app should not start the shim.
Make the app open a shell window, the shim should start.
Close the window, the shim should close immediately.

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

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

3 files changed, 87 insertions, 77 deletions

diff --git a/apps/app_shim/extension_app_shim_handler_mac.cc b/apps/app_shim/extension_app_shim_handler_mac.cc
index 2ba950f..95169b7 100644
--- a/apps/app_shim/extension_app_shim_handler_mac.cc
+++ b/apps/app_shim/extension_app_shim_handler_mac.cc
@@ -4,6 +4,7 @@
 
 #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"
@@ -12,6 +13,7 @@
 #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"
@@ -24,6 +26,23 @@
 #include "content/public/browser/notification_source.h"
 #include "ui/base/cocoa/focus_window_set.h"
 
+namespace {
+
+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(
@@ -43,17 +62,17 @@ Profile* ExtensionAppShimHandler::ProfileManagerFacade::ProfileForPath(
 
 ExtensionAppShimHandler::ExtensionAppShimHandler()
     : profile_manager_facade_(new ProfileManagerFacade) {
-  registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_HOST_CREATED,
+  // 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() {
-  for (HostMap::iterator it = hosts_.begin(); it != hosts_.end(); ) {
-    // Increment the iterator first as OnAppClosed may call back to OnShimClose
-    // and invalidate the iterator.
-    it++->second->OnAppClosed();
-  }
-}
+ExtensionAppShimHandler::~ExtensionAppShimHandler() {}
 
 bool ExtensionAppShimHandler::OnShimLaunch(Host* host,
                                            AppShimLaunchType launch_type) {
@@ -81,19 +100,12 @@ bool ExtensionAppShimHandler::OnShimLaunch(Host* host,
     return false;
 
   // The first host to claim this (profile, app_id) becomes the main host.
-  // For any others, we focus the app and return false.
+  // 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;
   }
 
-  if (!registrar_.IsRegistered(
-      this, chrome::NOTIFICATION_EXTENSION_HOST_DESTROYED,
-      content::Source<Profile>(profile))) {
-    registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_HOST_DESTROYED,
-                   content::Source<Profile>(profile));
-  }
-
   return true;
 }
 
@@ -116,12 +128,9 @@ void ExtensionAppShimHandler::OnShimFocus(Host* host) {
   Profile* profile =
       profile_manager_facade_->ProfileForPath(host->GetProfilePath());
 
-  extensions::ShellWindowRegistry* registry =
-      extensions::ShellWindowRegistry::Get(profile);
-  const extensions::ShellWindowRegistry::ShellWindowList windows =
-      registry->GetShellWindowsForApp(host->GetAppId());
+  const ShellWindowList windows = GetWindows(profile, host->GetAppId());
   std::set<gfx::NativeWindow> native_windows;
-  for (extensions::ShellWindowRegistry::const_iterator it = windows.begin();
+  for (ShellWindowList::const_iterator it = windows.begin();
        it != windows.end(); ++it) {
     native_windows.insert((*it)->GetNativeWindow());
   }
@@ -134,9 +143,7 @@ void ExtensionAppShimHandler::OnShimQuit(Host* host) {
   Profile* profile =
       profile_manager_facade_->ProfileForPath(host->GetProfilePath());
 
-  extensions::ShellWindowRegistry::ShellWindowList windows =
-      extensions::ShellWindowRegistry::Get(profile)->
-          GetShellWindowsForApp(host->GetAppId());
+  const ShellWindowList windows = GetWindows(profile, host->GetAppId());
   for (extensions::ShellWindowRegistry::const_iterator it = windows.begin();
        it != windows.end(); ++it) {
     (*it)->GetBaseWindow()->Close();
@@ -151,12 +158,7 @@ void ExtensionAppShimHandler::set_profile_manager_facade(
 bool ExtensionAppShimHandler::LaunchApp(Profile* profile,
                                         const std::string& app_id,
                                         AppShimLaunchType launch_type) {
-  extensions::ExtensionSystem* extension_system =
-      extensions::ExtensionSystem::Get(profile);
-  ExtensionServiceInterface* extension_service =
-      extension_system->extension_service();
-  const extensions::Extension* extension =
-      extension_service->GetExtensionById(app_id, false);
+  const extensions::Extension* extension = GetExtension(profile, app_id);
   if (!extension) {
     LOG(ERROR) << "Attempted to launch nonexistent app with id '"
                << app_id << "'.";
@@ -164,7 +166,7 @@ bool ExtensionAppShimHandler::LaunchApp(Profile* profile,
   }
 
   if (launch_type == APP_SHIM_LAUNCH_REGISTER_ONLY)
-    return true;
+    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
@@ -180,14 +182,23 @@ void ExtensionAppShimHandler::Observe(
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
   Profile* profile = content::Source<Profile>(source).ptr();
-  extensions::ExtensionHost* extension_host =
-      content::Details<extensions::ExtensionHost>(details).ptr();
+  if (profile->IsOffTheRecord())
+    return;
+
   switch (type) {
-    case chrome::NOTIFICATION_EXTENSION_HOST_CREATED:
-      StartShim(profile, extension_host->extension());
+    case chrome::NOTIFICATION_PROFILE_CREATED:
+      AppLifetimeMonitorFactory::GetForProfile(profile)->AddObserver(this);
       break;
-    case chrome::NOTIFICATION_EXTENSION_HOST_DESTROYED:
-      CloseShim(profile, extension_host->extension_id());
+    case chrome::NOTIFICATION_PROFILE_DESTROYED:
+      AppLifetimeMonitorFactory::GetForProfile(profile)->RemoveObserver(this);
+      // Shut down every shim associated with this profile.
+      for (HostMap::iterator it = hosts_.begin(); it != hosts_.end(); ) {
+        // Increment the iterator first as OnAppClosed may call back to
+        // OnShimClose and invalidate the iterator.
+        HostMap::iterator current = it++;
+        if (profile->IsSameProfile(current->first.first))
+          current->second->OnAppClosed();
+      }
       break;
     default:
       NOTREACHED();  // Unexpected notification.
@@ -195,10 +206,13 @@ void ExtensionAppShimHandler::Observe(
   }
 }
 
-void ExtensionAppShimHandler::StartShim(
-    Profile* profile,
-    const extensions::Extension* extension) {
-  if (!extension->is_platform_app())
+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())
     return;
 
   if (hosts_.count(make_pair(profile, extension->id())))
@@ -208,11 +222,16 @@ void ExtensionAppShimHandler::StartShim(
       web_app::ShortcutInfoForExtensionAndProfile(extension, profile));
 }
 
-void ExtensionAppShimHandler::CloseShim(Profile* profile,
-                                        const std::string& app_id) {
+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
diff --git a/apps/app_shim/extension_app_shim_handler_mac.h b/apps/app_shim/extension_app_shim_handler_mac.h
index 1be556a..043a4d5 100644
--- a/apps/app_shim/extension_app_shim_handler_mac.h
+++ b/apps/app_shim/extension_app_shim_handler_mac.h
@@ -8,6 +8,7 @@
 #include <map>
 #include <string>
 
+#include "apps/app_lifetime_monitor.h"
 #include "apps/app_shim/app_shim_handler_mac.h"
 #include "base/memory/scoped_ptr.h"
 #include "content/public/browser/notification_observer.h"
@@ -28,7 +29,8 @@ namespace apps {
 // This app shim handler that handles events for app shims that correspond to an
 // extension.
 class ExtensionAppShimHandler : public AppShimHandler,
-                                public content::NotificationObserver {
+                                public content::NotificationObserver,
+                                public AppLifetimeMonitor::Observer {
  public:
   class ProfileManagerFacade {
    public:
@@ -46,6 +48,15 @@ class ExtensionAppShimHandler : public AppShimHandler,
   virtual void OnShimFocus(Host* host) OVERRIDE;
   virtual void OnShimQuit(Host* host) OVERRIDE;
 
+  // AppLifetimeMonitor::Observer overrides:
+  virtual void OnAppStart(Profile* profile, const std::string& app_id) OVERRIDE;
+  virtual void OnAppActivated(Profile* profile,
+                              const std::string& app_id) OVERRIDE;
+  virtual void OnAppDeactivated(Profile* profile,
+                                const std::string& app_id) OVERRIDE;
+  virtual void OnAppStop(Profile* profile, const std::string& app_id) OVERRIDE;
+  virtual void OnChromeTerminating() OVERRIDE;
+
  protected:
   typedef std::map<std::pair<Profile*, std::string>, AppShimHandler::Host*>
       HostMap;
@@ -68,11 +79,8 @@ class ExtensionAppShimHandler : public AppShimHandler,
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE;
 
-  void StartShim(Profile* profile, const extensions::Extension* extension);
-
-  void CloseShim(Profile* profile, const std::string& app_id);
-
   scoped_ptr<ProfileManagerFacade> profile_manager_facade_;
+
   HostMap hosts_;
 
   content::NotificationRegistrar registrar_;
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 d440555..6f650e6 100644
--- a/apps/app_shim/extension_app_shim_handler_mac_unittest.cc
+++ b/apps/app_shim/extension_app_shim_handler_mac_unittest.cc
@@ -118,46 +118,34 @@ class ExtensionAppShimHandlerTest : public testing::Test {
 };
 
 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, APP_SHIM_LAUNCH_NORMAL))
+  EXPECT_CALL(*handler_, LaunchApp(&profile_a_, kTestAppIdA, normal_launch))
       .WillOnce(Return(false));
-  EXPECT_EQ(false, handler_->OnShimLaunch(&host_aa_, APP_SHIM_LAUNCH_NORMAL));
+  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, APP_SHIM_LAUNCH_NORMAL))
+  EXPECT_CALL(*handler_, LaunchApp(&profile_a_, kTestAppIdA, normal_launch))
       .WillOnce(Return(true));
-  EXPECT_EQ(true, handler_->OnShimLaunch(&host_aa_, APP_SHIM_LAUNCH_NORMAL));
+  EXPECT_EQ(true, handler_->OnShimLaunch(&host_aa_, normal_launch));
   EXPECT_EQ(&host_aa_, handler_->FindHost(&profile_a_, kTestAppIdA));
-  EXPECT_TRUE(handler_->GetRegistrar().IsRegistered(
-      handler_.get(),
-      chrome::NOTIFICATION_EXTENSION_HOST_DESTROYED,
-      content::Source<Profile>(&profile_a_)));
 
-  EXPECT_CALL(*handler_,
-              LaunchApp(&profile_a_, kTestAppIdB, APP_SHIM_LAUNCH_NORMAL))
+  EXPECT_CALL(*handler_, LaunchApp(&profile_a_, kTestAppIdB, normal_launch))
       .WillOnce(Return(true));
-  EXPECT_EQ(true, handler_->OnShimLaunch(&host_ab_, APP_SHIM_LAUNCH_NORMAL));
+  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, APP_SHIM_LAUNCH_NORMAL))
+  EXPECT_CALL(*handler_, LaunchApp(&profile_b_, kTestAppIdB, normal_launch))
       .WillOnce(Return(true));
-  EXPECT_EQ(true, handler_->OnShimLaunch(&host_bb_, APP_SHIM_LAUNCH_NORMAL));
+  EXPECT_EQ(true, handler_->OnShimLaunch(&host_bb_, normal_launch));
   EXPECT_EQ(&host_bb_, handler_->FindHost(&profile_b_, kTestAppIdB));
-  EXPECT_TRUE(handler_->GetRegistrar().IsRegistered(
-      handler_.get(),
-      chrome::NOTIFICATION_EXTENSION_HOST_DESTROYED,
-      content::Source<Profile>(&profile_b_)));
 
   // Starting and closing a second host does nothing.
-  EXPECT_CALL(*handler_,
-              LaunchApp(&profile_a_, kTestAppIdA, APP_SHIM_LAUNCH_NORMAL))
+  EXPECT_CALL(*handler_, LaunchApp(&profile_a_, kTestAppIdA, normal_launch))
       .WillOnce(Return(false));
-  EXPECT_EQ(false, handler_->OnShimLaunch(&host_aa_duplicate_,
-                                          APP_SHIM_LAUNCH_NORMAL));
+  EXPECT_EQ(false, handler_->OnShimLaunch(&host_aa_duplicate_, normal_launch));
   EXPECT_EQ(&host_aa_, handler_->FindHost(&profile_a_, kTestAppIdA));
   handler_->OnShimClose(&host_aa_duplicate_);
   EXPECT_EQ(&host_aa_, handler_->FindHost(&profile_a_, kTestAppIdA));
@@ -169,11 +157,6 @@ TEST_F(ExtensionAppShimHandlerTest, LaunchAndCloseShim) {
   // Closing the second host afterward does nothing.
   handler_->OnShimClose(&host_aa_duplicate_);
   EXPECT_FALSE(handler_->FindHost(&profile_a_, kTestAppIdA));
-
-  // Destruction sends OnAppClose to remaining hosts.
-  handler_.reset();
-  EXPECT_EQ(1, host_ab_.close_count());
-  EXPECT_EQ(1, host_bb_.close_count());
 }
 
 }  // namespace apps