Hosted apps on OS X now act more like a native app.

chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc

Index: chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc
diff --git a/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc b/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc
index d41bb0547d7dbe5aac3db33bb7ddb09b4e131f53..6760cd5673a0000c32fa387fadb53d2cfdbde0aa 100644
--- a/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc
+++ b/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc
@@ -11,11 +11,15 @@
 #include "chrome/browser/apps/app_shim/app_shim_host_manager_mac.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/extensions/launch_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/ui/browser_list.h"
+#include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/extensions/application_launch.h"
 #include "chrome/browser/ui/extensions/extension_enable_flow.h"
 #include "chrome/browser/ui/extensions/extension_enable_flow_delegate.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"

[jackhou1, 2014/12/12 00:48:29]

Do you still need this #include?

[mitchellj, 2014/12/12 03:46:23]

Acknowledged.

 #include "chrome/browser/web_applications/web_app_mac.h"
 #include "chrome/common/extensions/extension_constants.h"
 #include "chrome/common/extensions/extension_metrics.h"
@@ -29,6 +33,7 @@
 #include "extensions/browser/app_window/app_window_registry.h"
 #include "extensions/browser/app_window/native_app_window.h"
 #include "extensions/browser/extension_host.h"
+#include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/constants.h"
 #include "ui/base/cocoa/focus_window_set.h"
@@ -82,6 +87,19 @@ bool FocusWindows(const AppWindowList& windows) {
   return true;
 }
 
+bool FocusHostedAppWindows(std::set<Browser*>& browsers) {
+  if (browsers.empty())
+    return false;
+
+  std::set<gfx::NativeWindow> native_windows;
+  for (const Browser* browser : browsers) {

[jackhou1, 2014/12/12 00:48:28]

No braces on one-line for-loop.

[mitchellj, 2014/12/12 03:46:23]

Done.

+    native_windows.insert(browser->window()->GetNativeWindow());
+  }
+
+  ui::FocusWindowSet(native_windows);
+  return true;
+}
+
 // Attempts to launch a packaged app, prompting the user to enable it if
 // necessary. The prompt is shown in its own window.
 // This class manages its own lifetime.
@@ -168,13 +186,7 @@ const extensions::Extension*
 ExtensionAppShimHandler::Delegate::GetAppExtension(
     Profile* profile,
     const std::string& extension_id) {
-  ExtensionRegistry* registry = ExtensionRegistry::Get(profile);
-  const extensions::Extension* extension =
-      registry->GetExtensionById(extension_id, ExtensionRegistry::ENABLED);
-  return extension &&
-                 (extension->is_platform_app() || extension->is_hosted_app())
-             ? extension
-             : NULL;
+  return ExtensionAppShimHandler::GetAppExtension(profile, extension_id);
 }
 
 void ExtensionAppShimHandler::Delegate::EnableExtension(
@@ -229,9 +241,14 @@ ExtensionAppShimHandler::ExtensionAppShimHandler()
                  content::NotificationService::AllBrowserContextsAndSources());
   registrar_.Add(this, chrome::NOTIFICATION_PROFILE_DESTROYED,
                  content::NotificationService::AllBrowserContextsAndSources());
+  registrar_.Add(this, chrome::NOTIFICATION_BROWSER_WINDOW_READY,
+                 content::NotificationService::AllBrowserContextsAndSources());
+  BrowserList::AddObserver(this);
 }
 
-ExtensionAppShimHandler::~ExtensionAppShimHandler() {}
+ExtensionAppShimHandler::~ExtensionAppShimHandler() {
+  BrowserList::RemoveObserver(this);
+}
 
 AppShimHandler::Host* ExtensionAppShimHandler::FindHost(
     Profile* profile,
@@ -256,6 +273,18 @@ void ExtensionAppShimHandler::QuitAppForWindow(AppWindow* app_window) {
   }
 }
 
+// static
+void ExtensionAppShimHandler::QuitHostedAppForWindow(

[jackhou1, 2014/12/12 00:48:28]

I don't think you need to check is_hosted_app here, because if there's a host,
then we'll want to call OnShimQuit. So you can have this function take a
Profile* and app_id instead.

Also, we need to handle the case when there is no host. Chrome should just close
all the hosted app's windows.

[mitchellj, 2014/12/12 03:46:23]

Done.

+    Browser* browser,
+    const extensions::Extension* extension) {
+  ExtensionAppShimHandler* handler = GetInstance();
+  Host* host = handler->FindHost(
+      Profile::FromBrowserContext(browser->profile()), extension->id());
+  if (host && extension->is_hosted_app()) {
+    handler->OnShimQuit(host);
+  }
+}
+
 void ExtensionAppShimHandler::HideAppForWindow(AppWindow* app_window) {
   ExtensionAppShimHandler* handler = GetInstance();
   Profile* profile = Profile::FromBrowserContext(app_window->browser_context());
@@ -266,6 +295,37 @@ void ExtensionAppShimHandler::HideAppForWindow(AppWindow* app_window) {
     SetAppHidden(profile, app_window->extension_id(), true);
 }
 
+void ExtensionAppShimHandler::HideHostedApp(Profile* profile,
+                                            const std::string& app_id) {
+  ExtensionAppShimHandler* handler = GetInstance();
+  Host* host = handler->FindHost(profile, app_id);
+  if (host)
+    host->OnAppHide();
+  else
+    handler->SetHostedAppHidden(profile, app_id, true);
+}
+
+void ExtensionAppShimHandler::SetHostedAppHidden(Profile* profile,
+                                                 const std::string& app_id,
+                                                 bool hidden) {
+  const AppBrowserMap::iterator it = app_browser_windows_.find(app_id);
+  if (it == app_browser_windows_.end())
+    return;
+
+  for (const Browser* browser : it->second) {
+    if (!browser->is_app() ||

[jackhou1, 2014/12/12 00:48:29]

Can this ever happen? All the Browser*s in the map should be associated with
this app.

[mitchellj, 2014/12/12 03:46:23]

This is correct. I've removed the first condition in the IF statement.

+        web_app::GetExtensionIdFromApplicationName(browser->app_name()) !=
+            app_id) {
+      continue;
+    }
+
+    if (hidden)
+      browser->window()->Hide();
+    else
+      browser->window()->Show();
+  }
+}
+
 void ExtensionAppShimHandler::FocusAppForWindow(AppWindow* app_window) {
   ExtensionAppShimHandler* handler = GetInstance();
   Profile* profile = Profile::FromBrowserContext(app_window->browser_context());
@@ -281,6 +341,22 @@ void ExtensionAppShimHandler::FocusAppForWindow(AppWindow* app_window) {
 }
 
 // static
+const extensions::Extension* ExtensionAppShimHandler::GetAppExtension(
+    Profile* profile,
+    const std::string& extension_id) {
+  if (!profile)
+    return NULL;
+
+  ExtensionRegistry* registry = ExtensionRegistry::Get(profile);
+  const extensions::Extension* extension =
+      registry->GetExtensionById(extension_id, ExtensionRegistry::ENABLED);
+  return extension &&
+                 (extension->is_platform_app() || extension->is_hosted_app())
+             ? extension
+             : NULL;
+}
+
+// static
 bool ExtensionAppShimHandler::ActivateAndRequestUserAttentionForWindow(
     AppWindow* app_window) {
   ExtensionAppShimHandler* handler = GetInstance();
@@ -399,9 +475,14 @@ void ExtensionAppShimHandler::OnProfileLoaded(
       delegate_->GetAppExtension(profile, app_id);
   if (extension) {
     delegate_->LaunchApp(profile, extension, files);
-    // If it's a hosted app, just kill it immediately after opening for now.
-    if (extension->is_hosted_app())
+    // If it's a hosted app and a tabbed application, let the shim terminate

[jackhou1, 2014/12/12 00:48:29]

s/and a tabbed application/that opens in a tab/

[mitchellj, 2014/12/12 03:46:23]

Acknowledged.

+    // immediately.
+    if (extension->is_hosted_app() &&
+        extensions::GetLaunchType(extensions::ExtensionPrefs::Get(profile),
+                                  extension) ==
+            extensions::LAUNCH_TYPE_REGULAR) {
       host->OnAppLaunchComplete(APP_SHIM_LAUNCH_DUPLICATE_HOST);
+    }
     return;
   }
 
@@ -454,9 +535,19 @@ void ExtensionAppShimHandler::OnShimFocus(
   DCHECK(delegate_->ProfileExistsForPath(host->GetProfilePath()));
   Profile* profile = delegate_->ProfileForPath(host->GetProfilePath());
 
-  const AppWindowList windows =
-      delegate_->GetWindows(profile, host->GetAppId());
-  bool windows_focused = FocusWindows(windows);
+  bool windows_focused;
+  const std::string& app_id = host->GetAppId();
+  if (delegate_->GetAppExtension(profile, app_id)->is_hosted_app()) {
+    std::vector<content::WebContents*> items;

[jackhou1, 2014/12/12 00:48:28]

Is |items| ever used?

[mitchellj, 2014/12/12 03:46:23]

Oops, forgot to remove it when I changed the signature of FocusHostedAppWindows
and had to refactor the code. Done.

+    AppBrowserMap::iterator it = app_browser_windows_.find(app_id);
+    if (it == app_browser_windows_.end())
+      return;

[jackhou1, 2014/12/12 00:48:28]

Blank line after return.

[mitchellj, 2014/12/12 03:46:23]

Done.

+    windows_focused = FocusHostedAppWindows(it->second);
+  } else {
+    const AppWindowList windows =
+        delegate_->GetWindows(profile, host->GetAppId());
+    windows_focused = FocusWindows(windows);
+  }
 
   if (focus_type == APP_SHIM_FOCUS_NORMAL ||
       (focus_type == APP_SHIM_FOCUS_REOPEN && windows_focused)) {
@@ -478,7 +569,13 @@ void ExtensionAppShimHandler::OnShimSetHidden(Host* host, bool hidden) {
   DCHECK(delegate_->ProfileExistsForPath(host->GetProfilePath()));
   Profile* profile = delegate_->ProfileForPath(host->GetProfilePath());
 
-  SetAppHidden(profile, host->GetAppId(), hidden);
+  const std::string& app_id = host->GetAppId();
+  if (delegate_->GetAppExtension(profile, app_id)->is_hosted_app()) {
+    ExtensionAppShimHandler* handler = GetInstance();
+    handler->SetHostedAppHidden(profile, app_id, hidden);

[jackhou1, 2014/12/12 00:48:28]

You can call SetHostedAppHidden directly here since OnShimSetHidden is a
non-static method.

[mitchellj, 2014/12/12 03:46:23]

Done.

+  } else {
+    SetAppHidden(profile, app_id, hidden);
+  }
 }
 
 void ExtensionAppShimHandler::OnShimQuit(Host* host) {
@@ -486,14 +583,26 @@ void ExtensionAppShimHandler::OnShimQuit(Host* host) {
   Profile* profile = delegate_->ProfileForPath(host->GetProfilePath());
 
   const std::string& app_id = host->GetAppId();
-  const AppWindowList windows = delegate_->GetWindows(profile, app_id);
-  for (AppWindowRegistry::const_iterator it = windows.begin();
-       it != windows.end();
-       ++it) {
-    (*it)->GetBaseWindow()->Close();
+  if (delegate_->GetAppExtension(profile, app_id)->is_hosted_app()) {
+    std::vector<content::WebContents*> items;
+    AppBrowserMap::iterator it = app_browser_windows_.find(app_id);
+    if (it == app_browser_windows_.end())
+      return;
+
+    for (const Browser* browser : it->second) {

[jackhou1, 2014/12/12 00:48:29]

No braces here.

[mitchellj, 2014/12/12 03:46:23]

Done.

+      browser->window()->Close();
+    }
+  } else {
+    const AppWindowList windows = delegate_->GetWindows(profile, app_id);
+    for (AppWindowRegistry::const_iterator it = windows.begin();
+         it != windows.end(); ++it) {
+      (*it)->GetBaseWindow()->Close();
+    }
   }
   // Once the last window closes, flow will end up in OnAppDeactivated via
   // AppLifetimeMonitor.
+  // Otherwise, once the last window closes for a hosted app, OnBrowserRemoved
+  // will call OnAppDeactivated.
 }
 
 void ExtensionAppShimHandler::set_delegate(Delegate* delegate) {
@@ -504,16 +613,20 @@ void ExtensionAppShimHandler::Observe(
     int type,
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
-  Profile* profile = content::Source<Profile>(source).ptr();
-  if (profile->IsOffTheRecord())
-    return;
-
   switch (type) {
     case chrome::NOTIFICATION_PROFILE_CREATED: {
+      Profile* profile = content::Source<Profile>(source).ptr();
+      if (profile->IsOffTheRecord())
+        return;
+
       AppLifetimeMonitorFactory::GetForProfile(profile)->AddObserver(this);
       break;
     }
     case chrome::NOTIFICATION_PROFILE_DESTROYED: {
+      Profile* profile = content::Source<Profile>(source).ptr();
+      if (profile->IsOffTheRecord())
+        return;
+
       AppLifetimeMonitorFactory::GetForProfile(profile)->RemoveObserver(this);
       // Shut down every shim associated with this profile.
       for (HostMap::iterator it = hosts_.begin(); it != hosts_.end(); ) {
@@ -527,6 +640,21 @@ void ExtensionAppShimHandler::Observe(
       }
       break;
     }
+    case chrome::NOTIFICATION_BROWSER_WINDOW_READY: {
+      Browser* browser = content::Source<Browser>(source).ptr();
+      // Don't keep track of browsers that are not associated with an app.
+      if (!browser->is_app())

[jackhou1, 2014/12/12 00:48:29]

This should also check that the Extension is a hosted app.

[mitchellj, 2014/12/12 03:46:23]

Done.

+        return;
+
+      std::string app_id =
+          web_app::GetExtensionIdFromApplicationName(browser->app_name());
+      BrowserSet& browsers = app_browser_windows_[app_id];
+      browsers.insert(browser);
+      if (browsers.size() == 1)
+        OnAppActivated(browser->profile(), app_id);
+
+      break;
+    }
     default: {
       NOTREACHED();  // Unexpected notification.
       break;
@@ -569,4 +697,32 @@ void ExtensionAppShimHandler::OnAppStop(Profile* profile,
 
 void ExtensionAppShimHandler::OnChromeTerminating() {}
 
+// The BrowserWindow may be NULL when this is called.
+// Therefore we listen for the notification
+// chrome::NOTIFICATION_BROWSER_WINDOW_READY and then call OnAppActivited.

[jackhou1, 2014/12/12 00:48:28]

OnAppActivated

[mitchellj, 2014/12/12 03:46:23]

Done.

+// If this notification is removed, check that OnBrowserAdded is called after
+// the BrowserWindow is ready.
+void ExtensionAppShimHandler::OnBrowserAdded(Browser* browser) {
+}
+
+void ExtensionAppShimHandler::OnBrowserRemoved(Browser* browser) {
+  if (!browser->is_app())
+    return;
+
+  std::string app_id =
+      web_app::GetExtensionIdFromApplicationName(browser->app_name());
+  const extensions::Extension* extension =
+      GetAppExtension(browser->profile(), app_id);
+  if (!extension)
+    return;
+
+  AppBrowserMap::iterator it = app_browser_windows_.find(app_id);
+  if (it != app_browser_windows_.end()) {
+    BrowserSet& browsers = it->second;
+    browsers.erase(browser);
+    if (browsers.empty())
+      OnAppDeactivated(browser->profile(), app_id);
+  }
+}
+
 }  // namespace apps