Instantiate AppShimMenuController when app launcher is enabled. (Mac)

Instantiate AppShimMenuController when app launcher is enabled. (Mac)

Currently the AppShimMenuController is only instantiated if app shims
are enabled at startup. It's possible to enable app shims while Chrome
is running by enabling the app launcher. In this case v2 apps don't get
their own menu bar until Chrome is restarted.

This CL exposes InitAppShimMenuController in AppController, which is
called by AppListServiceMac when the app list is enabled.

BUG=314890
TEST=Start with app shims not enabled.
Enable app shims (e.g. enable the app launcher).
Start an app.
The app should have its own menu bar.

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

[benwells, 2013-11-01 06:03:59]

On 2013/11/01 05:58:18, jackhou1 wrote:

lgtm

[jackhou1, 2013-11-01 06:05:14]

thakis, please review for OWNERS

[Nico, 2013-11-01 23:47:59]

https://codereview.chromium.org/55683002/diff/1/chrome/browser/app_controller...
File chrome/browser/app_controller_mac.mm (left):

https://codereview.chromium.org/55683002/diff/1/chrome/browser/app_controller...
chrome/browser/app_controller_mac.mm:684: if (apps::IsAppShimsEnabled())
I suppose this becomes true some time after startup in your scenario. Could you
create the controller at that point?

[jackhou1, 2013-11-04 00:14:29]

+tapted, what do you think?

https://codereview.chromium.org/55683002/diff/1/chrome/browser/app_controller...
File chrome/browser/app_controller_mac.mm (left):

https://codereview.chromium.org/55683002/diff/1/chrome/browser/app_controller...
chrome/browser/app_controller_mac.mm:684: if (apps::IsAppShimsEnabled())
On 2013/11/01 23:48:00, Nico wrote:
> I suppose this becomes true some time after startup in your scenario. Could
you
> create the controller at that point?

It's not quite as neat. I've uploaded two options to minimize impact on startup
time. Patch 2 initializes the menu items lazily, but we still need to listen to
notifications. Patch 3 initializes the AppShimMenuController when the app
launcher is enabled, but we need to expose a method on AppController, and call
it from AppListServiceMac.

[Nico, 2013-11-04 00:22:57]

Patch set 3 is what I meant, thanks. tapted is back tomorrow, so if he likes
that I'd say we go with that. (in that case, patch set lgtm.)

https://codereview.chromium.org/55683002/diff/110001/chrome/browser/app_contr...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/55683002/diff/110001/chrome/browser/app_contr...
chrome/browser/app_controller_mac.mm:684: [self initAppShimMenuController];
I meant "can you keep the `if (apps::IsAppShimsEnabled())` here" (and alloc the
AppShimMenuController elsewhere if it turns out to be needed later)

…err nevermind, you just moved the if into another function. Sorry.

[tapted, 2013-11-04 00:46:40]

On 2013/11/04 00:14:29, jackhou1 wrote:
> +tapted, what do you think?
> 
>
https://codereview.chromium.org/55683002/diff/1/chrome/browser/app_controller...
> File chrome/browser/app_controller_mac.mm (left):
> 
>
https://codereview.chromium.org/55683002/diff/1/chrome/browser/app_controller...
> chrome/browser/app_controller_mac.mm:684: if (apps::IsAppShimsEnabled())
> On 2013/11/01 23:48:00, Nico wrote:
> > I suppose this becomes true some time after startup in your scenario. Could
> you
> > create the controller at that point?
> 
> It's not quite as neat. I've uploaded two options to minimize impact on
startup
> time. Patch 2 initializes the menu items lazily, but we still need to listen
to
> notifications. Patch 3 initializes the AppShimMenuController when the app
> launcher is enabled, but we need to expose a method on AppController, and call
> it from AppListServiceMac.

[tapted, 2013-11-04 00:49:26]

ugh - must have hit some hotkey

On 2013/11/04 00:46:40, tapted wrote:
> On 2013/11/04 00:14:29, jackhou1 wrote:
> > +tapted, what do you think?

Patchset 3 lgtm too. In the distant future this will probably roll into
(currently non-existent) apps_client_mac, but it's not there yet.

https://codereview.chromium.org/55683002/diff/110001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_service_mac.h (right):

https://codereview.chromium.org/55683002/diff/110001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_service_mac.h:46: virtual void
EnableAppList(Profile* initial_profile) OVERRIDE;
nit: I think this belongs under `AppListService overrides:`, since it's first
declared there. Should be ordered before `CreateShortcut()` in either case too
(also in the .cc).

[jackhou1, 2013-11-04 01:31:37]

https://codereview.chromium.org/55683002/diff/110001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_service_mac.h (right):

https://codereview.chromium.org/55683002/diff/110001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_service_mac.h:46: virtual void
EnableAppList(Profile* initial_profile) OVERRIDE;
On 2013/11/04 00:49:26, tapted wrote:
> nit: I think this belongs under `AppListService overrides:`, since it's first
> declared there. Should be ordered before `CreateShortcut()` in either case too
> (also in the .cc).

Done.

[commit-bot: I haz the power, 2013-11-04 06:52:18]

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

[commit-bot: I haz the power, 2013-11-04 07:30:51]

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

[commit-bot: I haz the power, 2013-11-05 02:38:05]

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

[commit-bot: I haz the power, 2013-11-05 03:17:42]

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

[commit-bot: I haz the power, 2013-11-05 05:19:51]

Retried try job too often on win_rel for step(s) content_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-06 01:24:37]

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

[commit-bot: I haz the power, 2013-11-06 02:16:45]

Change committed as 233190