Refactor extension handling code from AppShimHost into ExtensionAppShimHandler.

Refactor extension handling code from AppShimHost into ExtensionAppShimHandler.

AppShimHost is now confined to interacting with the app shim and passing
events to AppShimHandlers. ExtensionAppShimHandler becomes the default
handler for app shim events.

BUG=168080
TEST=There should be no behavior change with this refactor.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202791

[jackhou1, 2013-05-21 01:10:54]

I think this is ready, except for:
Where should extension_app_shim_handler live? 
And what should instantiate and own it?

[tapted, 2013-05-21 06:15:36]

On 2013/05/21 01:10:54, jackhou1 wrote:
> I think this is ready, except for:
> Where should extension_app_shim_handler live? 

I think it feels good in apps, so long as there are no additional DEPS. But I
think there's still a TODO to remove some of the existing ones added for this.

> And what should instantiate and own it?

This is a good question! I think there are a few options:
- Make it extend from ProfileKeyedService and add an entry in
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc, guarded
with an ifdef
- A new, apps::AppSystemFactory PKS
- Make it owned by the AppShimHost, and rely on the constructor call in
browser_process_impl.cc

Let's consult benwells as our top-level src/apps architect.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/app_shim_han...
File apps/app_shim/app_shim_handler_mac.cc (right):

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/app_shim_han...
apps/app_shim/app_shim_handler_mac.cc:40: AppShimHandler* old_default_handler =
default_handler_;
I think we can omit the return and add an appropriate DCHECK. (SetForAppMode has
a return so the compiler doesn't complain about inserted_or_removed being unused
when DCHECK is a no-op in Release builds.. -- and that DCHECK actually helped
diagnose some strangeness).

`DCHECK_NE(!default_handler_, !handler)` would do the job, but might be too
terse/obscure. Maybe DCHECK_NE(default_handler_ == NULL, handler == NULL)

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/app_shim_han...
File apps/app_shim/app_shim_handler_mac.h (right):

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/app_shim_han...
apps/app_shim/app_shim_handler_mac.h:24: virtual Profile* profile() const = 0;
Since there is no corresponding data member at this level, I think these need to
be TitleCase -- Profile, AppId -- rather than hacker_style. (Then, I'd just
rename the functions lower down rather than having accessors as well).

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/app_shim_han...
apps/app_shim/app_shim_handler_mac.h:25: virtual const std::string& app_id()
const = 0;
nit: Maybe drop the reference and just return std::string (avoids imposing an
implementation detail on concrete types, but it's a bit academic here).

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/app_shim_han...
apps/app_shim/app_shim_handler_mac.h:39: // handler if none is registered, or
NULL if there is no default handler.
nit: comment reads a bit funny. Maybe just drop the first "or"

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/app_shim_hos...
File apps/app_shim/app_shim_host_mac.cc (right):

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/app_shim_hos...
apps/app_shim/app_shim_host_mac.cc:16: #include
"chrome/browser/extensions/shell_window_registry.h"
A lot of these #includes are probably unused now.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/app_shim_hos...
apps/app_shim/app_shim_host_mac.cc:73: if (profile_) {
Maybe DCHECK(!profile) as well? There would need to be a buggy/rogue/third-party
shim process connecting to the socket for this case (right?)

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/app_shim_hos...
apps/app_shim/app_shim_host_mac.cc:78: profile_ =
FetchProfileForDirectory(profile_dir);
nit: blank line before, for early return

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/app_shim_hos...
File apps/app_shim/app_shim_host_mac.h (right):

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/app_shim_hos...
apps/app_shim/app_shim_host_mac.h:42: virtual const std::string& app_id() const
OVERRIDE;
nit: move these to the same place as the other overrides, or add a
`apps::AppShimHandler::Host overrides:` comment.

The jury is still out on whether overrides should be private, but Chrome
preference seems to be 'make them private if you can'. It might mean you need to
implicit_cast to the base class in tests, so keep them protected if that gets
too ugly.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
File apps/app_shim/extension_app_shim_handler_mac.cc (right):

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:7: #include
"apps/app_shim/app_shim_handler_mac.h"
don't need this one, since the files "own" header includes it

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:22: #include
"ipc/ipc_channel_proxy.h"
should be able to drop this. And maybe some others -- file_path? bind?
app_shim_messages? profile_manager?

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:25:
ExtensionAppShimHandler::ExtensionAppShimHandler() {
nit: no blank line in function. Most common is curlies on one line -- {}.
Update: registrar_.Add(..) should be in here.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:35: if (!profile)
DCHECK instead? This should never fail.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:40: LOG(ERROR) << "Bad app ID
from app shim launch message.";
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.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:49: registrar_.Add(this,
chrome::NOTIFICATION_EXTENSION_HOST_DESTROYED,
This should only be added once, in the constructor.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:57: if (observer_list)
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.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:59: }
I think this should also remove the entry from hosts_ when the last shim closes.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:67: const std::set<ShellWindow*>
windows =
Should perhaps be ShellWindowRegistry::ShellWindowSet rather than
std::set<ShellWindow*>

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:70: for
(std::set<ShellWindow*>::const_iterator i = windows.begin();
-> ShellWindowRegistry::const_iterator.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:72: ++i) {
nit: put on line above.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:75:
ui::FocusWindowSet(native_windows);
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.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:78:
ObserverList<apps::AppShimHandler::Host>*
the return type here should match the one in the header (the typedef)

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:82: if (!create_if_not_exist) {
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.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:87: std::pair<HostMap::iterator,
bool> result =
Maybe

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

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:116:
extension_misc::LAUNCH_NONE,
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.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:136: NOTREACHED() << "Unexpected
notification sent.";
nit: drop the string literal, or make it a comment, to reduce binary bloat.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:145: HostMap::mapped_type
observer_list = GetObserverList(profile, app_id, false);
nit: return early instead, `if (!observer_list)`.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
File apps/app_shim/extension_app_shim_handler_mac.h (right):

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.h:5: #ifndef
CHROME_BROWSER_WEB_APPLICATIONS_EXTENSION_APP_SHIM_HANDLER_H_
(if it stays here) include guard needs updating

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.h:17: #include "ipc/ipc_listener.h"
The IPC includes shouldn't be needed here. It looks like scoped_ptr might be
unused too.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.h:24: class ExtensionAppShimHandler
: public apps::AppShimHandler,
(if it stays here) can be namespace apps

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.h:26: public base::NonThreadSafe {
We can probably drop NonThreadSafe (and the CalledOnValidThread() calls), since
there's less question over whether this runs on the UI thread

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.h:31: // Methods from
AppShimHandler.
nit: "AppShimHandler overrides:" is the usual template

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.h:44:
content::NotificationRegistrar registrar_;
data members must be private (unless it's a test file)

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
File apps/app_shim/extension_app_shim_handler_mac_unittest.cc (right):

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac_unittest.cc:13: #include
"ipc/ipc_message.h"
is this used here?

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac_unittest.cc:52: virtual void
OnAppClosed() OVERRIDE { }
nit: no space between curlies

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac_unittest.cc:59: };
nit: DISALLOW_COPY_AND_ASSIGN(..)

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac_unittest.cc:69: virtual void
SetUp() OVERRIDE {
Maybe not an issue for unittests as much as it is for a browsertest, but this
should probably call testing::Test::SetUp as well

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac_unittest.cc:75:
scoped_ptr<FakeAppShimHost> host_;
can these be regular members, rather than scoped_ptr?

You can make them protected, and access them directly in the TEST_F too if you
like

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

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/app_shim_han...
File apps/app_shim/app_shim_handler_mac.cc (right):

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/app_shim_han...
apps/app_shim/app_shim_handler_mac.cc:40: AppShimHandler* old_default_handler =
default_handler_;
On 2013/05/21 06:15:37, tapted wrote:
> I think we can omit the return and add an appropriate DCHECK. (SetForAppMode
has
> a return so the compiler doesn't complain about inserted_or_removed being
unused
> when DCHECK is a no-op in Release builds.. -- and that DCHECK actually helped
> diagnose some strangeness).
> 
> `DCHECK_NE(!default_handler_, !handler)` would do the job, but might be too
> terse/obscure. Maybe DCHECK_NE(default_handler_ == NULL, handler == NULL)

Done.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/app_shim_han...
File apps/app_shim/app_shim_handler_mac.h (right):

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/app_shim_han...
apps/app_shim/app_shim_handler_mac.h:24: virtual Profile* profile() const = 0;
On 2013/05/21 06:15:37, tapted wrote:
> Since there is no corresponding data member at this level, I think these need
to
> be TitleCase -- Profile, AppId -- rather than hacker_style. (Then, I'd just
> rename the functions lower down rather than having accessors as well).

Done.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/app_shim_han...
apps/app_shim/app_shim_handler_mac.h:25: virtual const std::string& app_id()
const = 0;
On 2013/05/21 06:15:37, tapted wrote:
> nit: Maybe drop the reference and just return std::string (avoids imposing an
> implementation detail on concrete types, but it's a bit academic here).

Done.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/app_shim_han...
apps/app_shim/app_shim_handler_mac.h:39: // handler if none is registered, or
NULL if there is no default handler.
On 2013/05/21 06:15:37, tapted wrote:
> nit: comment reads a bit funny. Maybe just drop the first "or"

Done.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/app_shim_hos...
File apps/app_shim/app_shim_host_mac.cc (right):

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/app_shim_hos...
apps/app_shim/app_shim_host_mac.cc:16: #include
"chrome/browser/extensions/shell_window_registry.h"
On 2013/05/21 06:15:37, tapted wrote:
> A lot of these #includes are probably unused now.

Done.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/app_shim_hos...
apps/app_shim/app_shim_host_mac.cc:73: if (profile_) {
On 2013/05/21 06:15:37, tapted wrote:
> Maybe DCHECK(!profile) as well? There would need to be a
buggy/rogue/third-party
> shim process connecting to the socket for this case (right?)

Done.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/app_shim_hos...
apps/app_shim/app_shim_host_mac.cc:78: profile_ =
FetchProfileForDirectory(profile_dir);
On 2013/05/21 06:15:37, tapted wrote:
> nit: blank line before, for early return

Done.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/app_shim_hos...
File apps/app_shim/app_shim_host_mac.h (right):

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/app_shim_hos...
apps/app_shim/app_shim_host_mac.h:42: virtual const std::string& app_id() const
OVERRIDE;
On 2013/05/21 06:15:37, tapted wrote:
> nit: move these to the same place as the other overrides, or add a
> `apps::AppShimHandler::Host overrides:` comment.
> 
> The jury is still out on whether overrides should be private, but Chrome
> preference seems to be 'make them private if you can'. It might mean you need
to
> implicit_cast to the base class in tests, so keep them protected if that gets
> too ugly.

Done.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
File apps/app_shim/extension_app_shim_handler_mac.cc (right):

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:7: #include
"apps/app_shim/app_shim_handler_mac.h"
On 2013/05/21 06:15:37, tapted wrote:
> don't need this one, since the files "own" header includes it

Done.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:22: #include
"ipc/ipc_channel_proxy.h"
On 2013/05/21 06:15:37, tapted wrote:
> should be able to drop this. And maybe some others -- file_path? bind?
> app_shim_messages? profile_manager?

Done.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:25:
ExtensionAppShimHandler::ExtensionAppShimHandler() {
On 2013/05/21 06:15:37, tapted wrote:
> nit: no blank line in function. Most common is curlies on one line -- {}.
> Update: registrar_.Add(..) should be in here.

Done.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:35: if (!profile)
On 2013/05/21 06:15:37, tapted wrote:
> DCHECK instead? This should never fail.

Done.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:49: registrar_.Add(this,
chrome::NOTIFICATION_EXTENSION_HOST_DESTROYED,
On 2013/05/21 06:15:37, tapted wrote:
> This should only be added once, in the constructor.

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

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:57: if (observer_list)
On 2013/05/21 06:15:37, tapted wrote:
> 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.

Done.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:59: }
On 2013/05/21 06:15:37, tapted wrote:
> I think this should also remove the entry from hosts_ when the last shim
closes.

Done.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:67: const std::set<ShellWindow*>
windows =
On 2013/05/21 06:15:37, tapted wrote:
> Should perhaps be ShellWindowRegistry::ShellWindowSet rather than
> std::set<ShellWindow*>

Done.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:70: for
(std::set<ShellWindow*>::const_iterator i = windows.begin();
On 2013/05/21 06:15:37, tapted wrote:
> -> ShellWindowRegistry::const_iterator.

Done.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:72: ++i) {
On 2013/05/21 06:15:37, tapted wrote:
> nit: put on line above. 

Done.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:78:
ObserverList<apps::AppShimHandler::Host>*
On 2013/05/21 06:15:37, tapted wrote:
> the return type here should match the one in the header (the typedef)

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

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:82: if (!create_if_not_exist) {
On 2013/05/21 06:15:37, tapted wrote:
> 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.

Done.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:87: std::pair<HostMap::iterator,
bool> result =
On 2013/05/21 06:15:37, tapted wrote:
> Maybe
> 
> HostMap::mapped_type& value = hosts_[make_pair(profile, app_id)];
> if (!value)
>   value = new ObserverList<..>()
> return value;

Done.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:116:
extension_misc::LAUNCH_NONE,
On 2013/05/21 06:15:37, tapted wrote:
> 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.

Done.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:136: NOTREACHED() << "Unexpected
notification sent.";
On 2013/05/21 06:15:37, tapted wrote:
> nit: drop the string literal, or make it a comment, to reduce binary bloat.

Done.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:145: HostMap::mapped_type
observer_list = GetObserverList(profile, app_id, false);
On 2013/05/21 06:15:37, tapted wrote:
> nit: return early instead, `if (!observer_list)`. 

Done.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
File apps/app_shim/extension_app_shim_handler_mac.h (right):

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.h:5: #ifndef
CHROME_BROWSER_WEB_APPLICATIONS_EXTENSION_APP_SHIM_HANDLER_H_
On 2013/05/21 06:15:37, tapted wrote:
> (if it stays here) include guard needs updating

Done.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.h:17: #include "ipc/ipc_listener.h"
On 2013/05/21 06:15:37, tapted wrote:
> The IPC includes shouldn't be needed here. It looks like scoped_ptr might be
> unused too.

Done.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.h:24: class ExtensionAppShimHandler
: public apps::AppShimHandler,
On 2013/05/21 06:15:37, tapted wrote:
> (if it stays here) can be namespace apps

Done.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.h:26: public base::NonThreadSafe {
On 2013/05/21 06:15:37, tapted wrote:
> We can probably drop NonThreadSafe (and the CalledOnValidThread() calls),
since
> there's less question over whether this runs on the UI thread

Done.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.h:31: // Methods from
AppShimHandler.
On 2013/05/21 06:15:37, tapted wrote:
> nit: "AppShimHandler overrides:" is the usual template

Done.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.h:44:
content::NotificationRegistrar registrar_;
On 2013/05/21 06:15:37, tapted wrote:
> data members must be private (unless it's a test file)

Done.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
File apps/app_shim/extension_app_shim_handler_mac_unittest.cc (right):

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac_unittest.cc:13: #include
"ipc/ipc_message.h"
On 2013/05/21 06:15:37, tapted wrote:
> is this used here?

Nope, removed.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac_unittest.cc:52: virtual void
OnAppClosed() OVERRIDE { }
On 2013/05/21 06:15:37, tapted wrote:
> nit: no space between curlies

Done.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac_unittest.cc:59: };
On 2013/05/21 06:15:37, tapted wrote:
> nit: DISALLOW_COPY_AND_ASSIGN(..)

Done.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac_unittest.cc:69: virtual void
SetUp() OVERRIDE {
On 2013/05/21 06:15:37, tapted wrote:
> Maybe not an issue for unittests as much as it is for a browsertest, but this
> should probably call testing::Test::SetUp as well

Done.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac_unittest.cc:75:
scoped_ptr<FakeAppShimHost> host_;
On 2013/05/21 06:15:37, tapted wrote:
> can these be regular members, rather than scoped_ptr?
> 
> You can make them protected, and access them directly in the TEST_F too if you
> like

Done.

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

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
File apps/app_shim/extension_app_shim_handler_mac.cc (right):

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:49: registrar_.Add(this,
chrome::NOTIFICATION_EXTENSION_HOST_DESTROYED,
On 2013/05/22 23:54:44, jackhou1 wrote:
> On 2013/05/21 06:15:37, tapted wrote:
> > This should only be added once, in the constructor.
> 
> We don't know the profile at that point. Checking registrar_.IsRegistered(...)
> instead.

Ah, good point.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:78:
ObserverList<apps::AppShimHandler::Host>*
On 2013/05/22 23:54:44, jackhou1 wrote:
> On 2013/05/21 06:15:37, tapted wrote:
> > the return type here should match the one in the header (the typedef)
> 
> I can't seem to use HostMap::mapped_type here, might be because it's
protected?
> Made them both the full typename.

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.

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/app_shim_ho...
File apps/app_shim/app_shim_host_mac.h (right):

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_mac.h:65: // apps::AppShimHandler::Host override:
nit: plural overrides

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/app_shim_ho...
File apps/app_shim/app_shim_host_mac_unittest.cc (right):

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_mac_unittest.cc:128:
static_cast<apps::AppShimHandler::Host*>(host())->GetAppId());
Does implicit_cast work? (defined in src/base/basictypes.h)

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/app_shim_ho...
File apps/app_shim/app_shim_host_manager_mac.mm (right):

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_manager_mac.mm:31:
apps::AppShimHandler::SetDefaultHandler(&extension_app_shim_handler_);
You need to remove it in the destructor as well, or the shim hosts could access
a destroyed object.

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
File apps/app_shim/extension_app_shim_handler_mac.cc (right):

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac.cc:8: #include "base/bind.h"
unused?

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac.cc:25:
STLDeleteContainerPairSecondPointers(hosts_.begin(), hosts_.end());
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());

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac.cc:43: if
(!observer_list->HasObserver(host))
This should always be true? A particular Host should never call OnShimLaunch
twice. Maybe DCHECK(!observer_list->HasObserver(host))

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac.cc:49: if (!already_registered)
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.

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac.cc:61: if (!observer_list->size())
nit: size() == 0 tends to be preferred when there is no ".empty()" available.

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac.cc:83: const std::string& app_id) {
nit: indenting

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

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
File apps/app_shim/extension_app_shim_handler_mac.h (right):

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac.h:12: #include
"base/memory/scoped_ptr.h"
I think this can go?

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac.h:23: class ExtensionAppShimHandler
: public apps::AppShimHandler,
nit: no longer need apps:: prefix

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac.h:36:
ObserverList<apps::AppShimHandler::Host>* > HostMap;
s/apps:://

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac.h:38:
ObserverList<apps::AppShimHandler::Host>*
s/apps:://

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac.h:41:
ObserverList<apps::AppShimHandler::Host>*
s/apps:://

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac.h:46:
content::NotificationRegistrar* registrar() { return &registrar_; }
nit: a reference is probably better here, since it can never be null. pity that
NotificationRegistrar::IsRegistered is not declared const.. Also a comment,
'Exposed for testing' is probably in order.

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac.h:59: void Close(Profile* profile,
const std::string& app_id);
Maybe CloseShim, since it's not the handler that's closing

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
File apps/app_shim/extension_app_shim_handler_mac_unittest.cc (right):

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac_unittest.cc:8: #include
"base/memory/scoped_ptr.h"
unused?

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac_unittest.cc:58: };
DISALLOW_COPY_AND_ASSIGN ?

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

As discussed, changed this to just a map of AppShimHandler::Host. Added testing
for multiple Hosts for the same profile and extension.

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
File apps/app_shim/extension_app_shim_handler_mac.cc (right):

https://codereview.chromium.org/15269003/diff/2001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:78:
ObserverList<apps::AppShimHandler::Host>*
On 2013/05/23 02:29:39, tapted wrote:
> On 2013/05/22 23:54:44, jackhou1 wrote:
> > On 2013/05/21 06:15:37, tapted wrote:
> > > the return type here should match the one in the header (the typedef)
> > 
> > I can't seem to use HostMap::mapped_type here, might be because it's
> protected?
> > Made them both the full typename.
> 
> 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.

Done.

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/app_shim_ho...
File apps/app_shim/app_shim_host_mac.h (right):

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_mac.h:65: // apps::AppShimHandler::Host override:
On 2013/05/23 02:29:39, tapted wrote:
> nit: plural overrides

Done.

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/app_shim_ho...
File apps/app_shim/app_shim_host_mac_unittest.cc (right):

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_mac_unittest.cc:128:
static_cast<apps::AppShimHandler::Host*>(host())->GetAppId());
On 2013/05/23 02:29:39, tapted wrote:
> Does implicit_cast work? (defined in src/base/basictypes.h)

Done.

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/app_shim_ho...
File apps/app_shim/app_shim_host_manager_mac.mm (right):

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_manager_mac.mm:31:
apps::AppShimHandler::SetDefaultHandler(&extension_app_shim_handler_);
On 2013/05/23 02:29:39, tapted wrote:
> You need to remove it in the destructor as well, or the shim hosts could
access
> a destroyed object.

Done.

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
File apps/app_shim/extension_app_shim_handler_mac.cc (right):

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac.cc:8: #include "base/bind.h"
On 2013/05/23 02:29:39, tapted wrote:
> unused?

Done.

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac.cc:25:
STLDeleteContainerPairSecondPointers(hosts_.begin(), hosts_.end());
On 2013/05/23 02:29:39, tapted wrote:
> 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());

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

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac.cc:43: if
(!observer_list->HasObserver(host))
On 2013/05/23 02:29:39, tapted wrote:
> This should always be true? A particular Host should never call OnShimLaunch
> twice. Maybe DCHECK(!observer_list->HasObserver(host))

Done.

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac.cc:49: if (!already_registered)
On 2013/05/23 02:29:39, tapted wrote:
> 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.

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

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac.cc:61: if (!observer_list->size())
On 2013/05/23 02:29:39, tapted wrote:
> nit: size() == 0 tends to be preferred when there is no ".empty()" available.

Done.

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac.cc:83: const std::string& app_id) {
On 2013/05/23 02:29:39, tapted wrote:
> nit: indenting
> 
> You could also use HostMap::const_iterator and declare this function const to
> drum it in

Done.

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
File apps/app_shim/extension_app_shim_handler_mac.h (right):

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac.h:12: #include
"base/memory/scoped_ptr.h"
On 2013/05/23 02:29:39, tapted wrote:
> I think this can go?

Done.

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac.h:23: class ExtensionAppShimHandler
: public apps::AppShimHandler,
On 2013/05/23 02:29:39, tapted wrote:
> nit: no longer need apps:: prefix

Done.

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac.h:36:
ObserverList<apps::AppShimHandler::Host>* > HostMap;
On 2013/05/23 02:29:39, tapted wrote:
> s/apps:://

Done.

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac.h:38:
ObserverList<apps::AppShimHandler::Host>*
On 2013/05/23 02:29:39, tapted wrote:
> s/apps:://

Done.

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac.h:41:
ObserverList<apps::AppShimHandler::Host>*
On 2013/05/23 02:29:39, tapted wrote:
> s/apps:://

Done.

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac.h:46:
content::NotificationRegistrar* registrar() { return &registrar_; }
On 2013/05/23 02:29:39, tapted wrote:
> nit: a reference is probably better here, since it can never be null. pity
that
> NotificationRegistrar::IsRegistered is not declared const.. Also a comment,
> 'Exposed for testing' is probably in order.

Done.

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac.h:59: void Close(Profile* profile,
const std::string& app_id);
On 2013/05/23 02:29:39, tapted wrote:
> Maybe CloseShim, since it's not the handler that's closing

Done.

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
File apps/app_shim/extension_app_shim_handler_mac_unittest.cc (right):

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac_unittest.cc:8: #include
"base/memory/scoped_ptr.h"
On 2013/05/23 02:29:39, tapted wrote:
> unused?

Done.

https://codereview.chromium.org/15269003/diff/13001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac_unittest.cc:58: };
On 2013/05/23 02:29:39, tapted wrote:
> DISALLOW_COPY_AND_ASSIGN ?

Done.

[tapted, 2013-05-23 07:03:11]

sweet! lgtm with a few nits

https://codereview.chromium.org/15269003/diff/28001/apps/app_shim/app_shim_ho...
File apps/app_shim/app_shim_host_manager_mac.h (right):

https://codereview.chromium.org/15269003/diff/28001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_manager_mac.h:9: #include
"base/mac/scoped_cftyperef.h"
while you're here... I think this one and base/threading/thread.h are strays
that can be dropped (might need to be exchanged for base/memory/scoped_ptr.h).

And... if scoped_ctyperef.h goes, it looks like there's no more objective C here
(and so the manager_mac.mm can be a .cc). But, let's save that for a follow-up.

https://codereview.chromium.org/15269003/diff/28001/apps/app_shim/extension_a...
File apps/app_shim/extension_app_shim_handler_mac.cc (right):

https://codereview.chromium.org/15269003/diff/28001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac.cc:26: HostMap::iterator current =
it;
I usually use postfix for this. `it++->second->OnAppClosed();` should do the
trick.

also, I think it "can't" call back (except in your test), since a call to
OnShimClose in the host requires the default handler to still be around (which
would have been set NULL in the body of ~AppShimHostManager()). But safe is
better ;)

https://codereview.chromium.org/15269003/diff/28001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac.cc:46: // For any others, we launch
the app but return false.
nice - this feels right

https://codereview.chromium.org/15269003/diff/28001/apps/app_shim/extension_a...
File apps/app_shim/extension_app_shim_handler_mac.h (right):

https://codereview.chromium.org/15269003/diff/28001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac.h:22: public
content::NotificationObserver{
nit: space before curly

https://codereview.chromium.org/15269003/diff/28001/apps/app_shim/extension_a...
File apps/app_shim/extension_app_shim_handler_mac_unittest.cc (right):

https://codereview.chromium.org/15269003/diff/28001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac_unittest.cc:83:
scoped_ptr<TestingExtensionAppShimHandler> handler_;
scoped_ptr is back! --> #include :p

[jackhou1, 2013-05-24 00:12:27]

https://codereview.chromium.org/15269003/diff/28001/apps/app_shim/app_shim_ho...
File apps/app_shim/app_shim_host_manager_mac.h (right):

https://codereview.chromium.org/15269003/diff/28001/apps/app_shim/app_shim_ho...
apps/app_shim/app_shim_host_manager_mac.h:9: #include
"base/mac/scoped_cftyperef.h"
On 2013/05/23 07:03:11, tapted wrote:
> while you're here... I think this one and base/threading/thread.h are strays
> that can be dropped (might need to be exchanged for base/memory/scoped_ptr.h).
> 
> And... if scoped_ctyperef.h goes, it looks like there's no more objective C
here
> (and so the manager_mac.mm can be a .cc). But, let's save that for a
follow-up.

Done.

https://codereview.chromium.org/15269003/diff/28001/apps/app_shim/extension_a...
File apps/app_shim/extension_app_shim_handler_mac.cc (right):

https://codereview.chromium.org/15269003/diff/28001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac.cc:26: HostMap::iterator current =
it;
On 2013/05/23 07:03:11, tapted wrote:
> I usually use postfix for this. `it++->second->OnAppClosed();` should do the
> trick.
> 
> also, I think it "can't" call back (except in your test), since a call to
> OnShimClose in the host requires the default handler to still be around (which
> would have been set NULL in the body of ~AppShimHostManager()). But safe is
> better ;)

Done.

https://codereview.chromium.org/15269003/diff/28001/apps/app_shim/extension_a...
File apps/app_shim/extension_app_shim_handler_mac.h (right):

https://codereview.chromium.org/15269003/diff/28001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac.h:22: public
content::NotificationObserver{
On 2013/05/23 07:03:11, tapted wrote:
> nit: space before curly

Done.

https://codereview.chromium.org/15269003/diff/28001/apps/app_shim/extension_a...
File apps/app_shim/extension_app_shim_handler_mac_unittest.cc (right):

https://codereview.chromium.org/15269003/diff/28001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac_unittest.cc:83:
scoped_ptr<TestingExtensionAppShimHandler> handler_;
On 2013/05/23 07:03:11, tapted wrote:
> scoped_ptr is back! --> #include :p

Done.

[jackhou1, 2013-05-26 23:11:08]

benwells, could you please review for OWNERS?

[benwells, 2013-05-28 00:36:58]

apps/apps.gypi lgtm

[commit-bot: I haz the power, 2013-05-28 00:39:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jackhou@chromium.org/15269003/85001

[commit-bot: I haz the power, 2013-05-28 02:25:07]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-05-29 01:42:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jackhou@chromium.org/15269003/99001

[commit-bot: I haz the power, 2013-05-29 07:19:26]

Change committed as 202791