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_handler_mac.h"

[tapted, 2013/05/21 06:15:37]

don't need this one, since the files "own" header includes it

[jackhou1, 2013/05/22 23:54:44]

Done.

+#include "apps/app_shim/app_shim_messages.h"
+#include "base/bind.h"
+#include "base/files/file_path.h"
+#include "base/logging.h"
+#include "base/stl_util.h"
+#include "chrome/browser/browser_process.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/profiles/profile_manager.h"
+#include "chrome/browser/ui/extensions/application_launch.h"
+#include "chrome/browser/ui/extensions/shell_window.h"
+#include "chrome/common/extensions/extension_constants.h"
+#include "ipc/ipc_channel_proxy.h"

[tapted, 2013/05/21 06:15:37]

should be able to drop this. And maybe some others -- file_path? bind?
app_shim_messages? profile_manager?

[jackhou1, 2013/05/22 23:54:44]

Done.

+#include "ui/base/cocoa/focus_window_set.h"
+
+ExtensionAppShimHandler::ExtensionAppShimHandler() {

[tapted, 2013/05/21 06:15:37]

nit: no blank line in function. Most common is curlies on one line -- {}.
Update: registrar_.Add(..) should be in here.

[jackhou1, 2013/05/22 23:54:44]

Done.

+
+}
+
+ExtensionAppShimHandler::~ExtensionAppShimHandler() {
+  STLDeleteContainerPairSecondPointers(hosts_.begin(), hosts_.end());
+}
+
+bool ExtensionAppShimHandler::OnShimLaunch(Host* host) {
+  Profile* profile = host->profile();
+  if (!profile)

[tapted, 2013/05/21 06:15:37]

DCHECK instead? This should never fail.

[jackhou1, 2013/05/22 23:54:44]

Done.

+    return false;
+
+  const std::string& app_id = host->app_id();
+  if (!extensions::Extension::IdIsValid(app_id)) {
+    LOG(ERROR) << "Bad app ID from app shim launch message.";

[tapted, 2013/05/21 06:15:37]

I think this is OK for now.. But we might eventually want a localized prompt
somewhere -- LOG(FOO) messages tend to get lost, and just bloat the binary.

+    return false;
+  }
+
+  if (!LaunchApp(profile, app_id))
+    return false;
+
+  GetObserverList(profile, app_id, true)->AddObserver(host);
+
+  registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_HOST_DESTROYED,

[tapted, 2013/05/21 06:15:37]

This should only be added once, in the constructor.

[jackhou1, 2013/05/22 23:54:44]

We don't know the profile at that point. Checking registrar_.IsRegistered(...)
instead.

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

Ah, good point.

+                 content::Source<Profile>(profile));
+  return true;
+}
+
+void ExtensionAppShimHandler::OnShimClose(Host* host) {
+  HostMap::mapped_type observer_list =
+      GetObserverList(host->profile(), host->app_id(), false);
+  if (observer_list)

[tapted, 2013/05/21 06:15:37]

DCHECK (and segfault in Release) if this should never happen. If it can happen,
add a comment about the corner case where it might occur.

i.e. If OnShimLaunch returns false for a given host we should guarantee the
handler interfaces are not be subsequently used for the same host. Likewise, we
should guarantee that OnShimClose is not called twice for the same host, nor is
there any way for an observer to be added or removed except in these two
functions.

[jackhou1, 2013/05/22 23:54:44]

Done.

+    observer_list->RemoveObserver(host);
+}

[tapted, 2013/05/21 06:15:37]

I think this should also remove the entry from hosts_ when the last shim closes.

[jackhou1, 2013/05/22 23:54:44]

Done.

+
+void ExtensionAppShimHandler::OnShimFocus(Host* host) {
+  if (!host->profile())
+    return;
+
+  extensions::ShellWindowRegistry* registry =
+      extensions::ShellWindowRegistry::Get(host->profile());
+  const std::set<ShellWindow*> windows =

[tapted, 2013/05/21 06:15:37]

Should perhaps be ShellWindowRegistry::ShellWindowSet rather than
std::set<ShellWindow*>

[jackhou1, 2013/05/22 23:54:44]

Done.

+      registry->GetShellWindowsForApp(host->app_id());
+  std::set<gfx::NativeWindow> native_windows;
+  for (std::set<ShellWindow*>::const_iterator i = windows.begin();

[tapted, 2013/05/21 06:15:37]

-> ShellWindowRegistry::const_iterator.

[jackhou1, 2013/05/22 23:54:44]

Done.

+       i != windows.end();
+       ++i) {

[tapted, 2013/05/21 06:15:37]

nit: put on line above. 

[jackhou1, 2013/05/22 23:54:44]

Done.

+    native_windows.insert((*i)->GetNativeWindow());
+  }
+  ui::FocusWindowSet(native_windows);

[tapted, 2013/05/21 06:15:37]

something to think about in future: there is a ~5second time period between the
last window closing and the extension host being destroyed (I think). Typical
behaviour on OSX is to open a new window in this case, but how that feeds into
packaged apps is probably something to discuss on apps-dev.

+}
+
+ObserverList<apps::AppShimHandler::Host>*

[tapted, 2013/05/21 06:15:37]

the return type here should match the one in the header (the typedef)

[jackhou1, 2013/05/22 23:54:44]

I can't seem to use HostMap::mapped_type here, might be because it's protected?
Made them both the full typename.

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

I think you just need to resolve scope?

`ExtensionAppShimHandler::HostMap::mapped_type
ExtensionAppShimHandler::FindObserverList(..) {..}` should work even though the
typedef is protected. And I think is preferred..

Factoid: C++11 would let us do

`auto ExtensionAppShimHandler::FindObserverList(..) -> HostMap::mapped_type
{..}`

but until then, we need to put up with the return type being in a different
scope to the rest of the function definition.

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

Done.

+ExtensionAppShimHandler::GetObserverList(Profile* profile,
+                                         const std::string& app_id,
+                                         bool create_if_not_exist) {
+  if (!create_if_not_exist) {

[tapted, 2013/05/21 06:15:37]

I think this should be two functions if there's no overlap.

Then the 'GetOrAdd..' function can never return NULL, so you could make it
return a reference if you like.

[jackhou1, 2013/05/22 23:54:44]

Done.

+    HostMap::iterator it = hosts_.find(make_pair(profile, app_id));
+    return it == hosts_.end() ? NULL : it->second;
+  }
+
+  std::pair<HostMap::iterator, bool> result =

[tapted, 2013/05/21 06:15:37]

Maybe

HostMap::mapped_type& value = hosts_[make_pair(profile, app_id)];
if (!value)
  value = new ObserverList<..>()
return value;

[jackhou1, 2013/05/22 23:54:44]

Done.

+      hosts_.insert(make_pair(
+          make_pair(profile, app_id),
+          static_cast<HostMap::mapped_type>(NULL)));
+  if (result.second) {
+    result.first->second = new ObserverList<apps::AppShimHandler::Host>();
+  }
+  return result.first->second;
+}
+
+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::AppLaunchParams params(profile,
+                                 extension,
+                                 extension_misc::LAUNCH_NONE,

[tapted, 2013/05/21 06:15:37]

interesting factoid. Using LAUNCH_NONE means OpenApplication will hit a
NOTREACHED() if the extension is ever something other than a platform app. I
guess that's OK (since Focus makes no sense if it's not a platform app)... an
alternative would be

  chrome::OpenApplication(chrome::AppLaunchParams(
      profile, extension, NEW_FOREGROUND_TAB));

which the app_list_controller uses, and might be preferred if we open up shims
to non-platform-apps.

[jackhou1, 2013/05/22 23:54:44]

Done.

+                                 NEW_WINDOW);
+  chrome::OpenApplication(params);
+  return true;
+}
+
+void ExtensionAppShimHandler::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  DCHECK(CalledOnValidThread());
+  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 sent.";

[tapted, 2013/05/21 06:15:37]

nit: drop the string literal, or make it a comment, to reduce binary bloat.

[jackhou1, 2013/05/22 23:54:44]

Done.

+      break;
+  }
+}
+
+void ExtensionAppShimHandler::Close(Profile* profile,
+                                    const std::string& app_id) {
+  DCHECK(CalledOnValidThread());
+
+  HostMap::mapped_type observer_list = GetObserverList(profile, app_id, false);

[tapted, 2013/05/21 06:15:37]

nit: return early instead, `if (!observer_list)`. 

[jackhou1, 2013/05/22 23:54:44]

Done.

+  if (observer_list) {
+    FOR_EACH_OBSERVER(apps::AppShimHandler::Host,
+                      *observer_list,
+                      OnAppClosed());
+  }
+}