Refactor extension handling code from AppShimHost into ExtensionAppShimHandler.

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_shim/app_shim_messages.h"
+#include "base/bind.h"

[tapted, 2013/05/23 02:29:39]

unused?

[jackhou1, 2013/05/23 06:10:19]

Done.

+#include "base/files/file_path.h"
+#include "base/logging.h"
+#include "base/stl_util.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/ui/extensions/application_launch.h"
+#include "chrome/browser/ui/extensions/shell_window.h"
+#include "ui/base/cocoa/focus_window_set.h"
+
+namespace apps {
+
+ExtensionAppShimHandler::ExtensionAppShimHandler() {}
+
+ExtensionAppShimHandler::~ExtensionAppShimHandler() {
+  STLDeleteContainerPairSecondPointers(hosts_.begin(), hosts_.end());

[tapted, 2013/05/23 02:29:39]

This should probably send all the shim hosts a close message.. OR, it should now
always be empty, since we should have received the notifications for all the
extension hosts being unloaded before the browser process started shutting down
in void BrowserProcessImpl::StartTearDown() where this should be triggered from.

If we're confident, maybe this should be a DCHECK(hosts_.empty());

[jackhou1, 2013/05/23 06:10:19]

Turns out we can't have a map of ObserverLists because we can't delete the
ObserverList while iterating over it.

+}
+
+bool ExtensionAppShimHandler::OnShimLaunch(Host* host) {
+  Profile* profile = host->GetProfile();
+  DCHECK(profile);
+
+  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;
+  }
+
+  if (!LaunchApp(profile, app_id))
+    return false;
+
+  HostMap::mapped_type observer_list =
+      FindOrInsertObserverList(profile, app_id);
+  if (!observer_list->HasObserver(host))

[tapted, 2013/05/23 02:29:39]

This should always be true? A particular Host should never call OnShimLaunch
twice. Maybe DCHECK(!observer_list->HasObserver(host))

[jackhou1, 2013/05/23 06:10:19]

Done.

+    observer_list->AddObserver(host);
+
+  bool already_registered = registrar_.IsRegistered(
+      this, chrome::NOTIFICATION_EXTENSION_HOST_DESTROYED,
+      content::Source<Profile>(profile));
+  if (!already_registered)

[tapted, 2013/05/23 02:29:39]

This needs curlies for a multi-line body. It should be ok without the
already_registered temporary too (i.e. move IsRegistered(..) into the
condition). This might also be better in the (!value) case in
FindOrAddObserverList but I'm undecided - up to you.

[jackhou1, 2013/05/23 06:10:19]

Do you mean move this code into FindOrInsertObserverList? I'd prefer for that
method to only operate on hosts_.

+    registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_HOST_DESTROYED,
+                   content::Source<Profile>(profile));
+
+  return true;
+}
+
+void ExtensionAppShimHandler::OnShimClose(Host* host) {
+  HostMap::mapped_type observer_list =
+      FindObserverList(host->GetProfile(), host->GetAppId());
+  DCHECK(observer_list);
+  observer_list->RemoveObserver(host);
+  if (!observer_list->size())

[tapted, 2013/05/23 02:29:39]

nit: size() == 0 tends to be preferred when there is no ".empty()" available.

[jackhou1, 2013/05/23 06:10:19]

Done.

+    RemoveObserverList(host->GetProfile(), host->GetAppId());
+}
+
+void ExtensionAppShimHandler::OnShimFocus(Host* host) {
+  if (!host->GetProfile())
+    return;
+
+  extensions::ShellWindowRegistry* registry =
+      extensions::ShellWindowRegistry::Get(host->GetProfile());
+  const extensions::ShellWindowRegistry::ShellWindowSet windows =
+      registry->GetShellWindowsForApp(host->GetAppId());
+  std::set<gfx::NativeWindow> native_windows;
+  for (extensions::ShellWindowRegistry::const_iterator i = windows.begin();
+       i != windows.end(); ++i) {
+    native_windows.insert((*i)->GetNativeWindow());
+  }
+  ui::FocusWindowSet(native_windows);
+}
+
+ObserverList<apps::AppShimHandler::Host>*
+ExtensionAppShimHandler::FindObserverList(Profile* profile,
+                                         const std::string& app_id) {

[tapted, 2013/05/23 02:29:39]

nit: indenting

You could also use HostMap::const_iterator and declare this function const to
drum it in

[jackhou1, 2013/05/23 06:10:19]

Done.

+  HostMap::iterator it = hosts_.find(make_pair(profile, app_id));
+  return it == hosts_.end() ? NULL : it->second;
+}
+
+ObserverList<apps::AppShimHandler::Host>*
+ExtensionAppShimHandler::FindOrInsertObserverList(Profile* profile,
+                                                  const std::string& app_id) {
+  HostMap::mapped_type& value = hosts_[make_pair(profile, app_id)];
+  if (!value)
+    value = new ObserverList<apps::AppShimHandler::Host>();
+  return value;
+}
+
+bool ExtensionAppShimHandler::RemoveObserverList(Profile* profile,
+                                                 const std::string& app_id) {
+  return hosts_.erase(make_pair(profile, app_id));
+}
+
+bool ExtensionAppShimHandler::LaunchApp(Profile* profile,
+                                        const std::string& app_id) {
+  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);
+  if (!extension) {
+    LOG(ERROR) << "Attempted to launch nonexistent app with id '"
+               << app_id << "'.";
+    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.
+  chrome::OpenApplication(chrome::AppLaunchParams(
+      profile, extension, NEW_FOREGROUND_TAB));
+  return true;
+}
+
+void ExtensionAppShimHandler::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  Profile* profile = content::Source<Profile>(source).ptr();
+  switch (type) {
+    case chrome::NOTIFICATION_EXTENSION_HOST_DESTROYED: {
+      extensions::ExtensionHost* extension_host =
+          content::Details<extensions::ExtensionHost>(details).ptr();
+      Close(profile, extension_host->extension_id());
+      break;
+    }
+    default:
+      NOTREACHED();  // Unexpected notification.
+      break;
+  }
+}
+
+void ExtensionAppShimHandler::Close(Profile* profile,
+                                    const std::string& app_id) {
+  HostMap::mapped_type observer_list = FindObserverList(profile, app_id);
+  if (!observer_list)
+    return;
+
+  FOR_EACH_OBSERVER(apps::AppShimHandler::Host,
+                    *observer_list,
+                    OnAppClosed());
+}
+
+}  // namespace apps