Giving focus to an app shim brings windows belonging to that app to the foreground.

Giving focus to an app shim brings windows belonging to that app to the foreground.

When the user either clicks on the app shim's icon in the dock or selects it
with Cmd+Tab, we send a message to Chrome to ask it to bring that app's windows
to the front.

BUG=138733, 168080
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188926

[jeremya, 2013-03-18 00:49:57]

koz- apps/
thakis- ui/ and chrome/app
palmer- IPC changes

This depends on https://codereview.chromium.org/12623005/

[koz (OOO until 15th September), 2013-03-18 03:33:12]

https://codereview.chromium.org/12723011/diff/1/apps/app_shim/app_shim_host_m...
File apps/app_shim/app_shim_host_mac.cc (right):

https://codereview.chromium.org/12723011/diff/1/apps/app_shim/app_shim_host_m...
apps/app_shim/app_shim_host_mac.cc:61: void AppShimHost::OnFocus() {
DCHECK(CalledOnValidThread())

https://codereview.chromium.org/12723011/diff/1/apps/app_shim/app_shim_host_m...
apps/app_shim/app_shim_host_mac.cc:62: // TODO move
Move to where?

https://codereview.chromium.org/12723011/diff/1/apps/app_shim/app_shim_host_m...
File apps/app_shim/app_shim_host_mac.h (right):

https://codereview.chromium.org/12723011/diff/1/apps/app_shim/app_shim_host_m...
apps/app_shim/app_shim_host_mac.h:63: // TODO comment
TODO

https://codereview.chromium.org/12723011/diff/1/apps/app_shim/app_shim_host_m...
File apps/app_shim/app_shim_host_manager_mac.mm (right):

https://codereview.chromium.org/12723011/diff/1/apps/app_shim/app_shim_host_m...
apps/app_shim/app_shim_host_manager_mac.mm:15: #import
"chrome/common/mac/app_mode_common.h"
Is this necessary? Other places #include this file.

https://codereview.chromium.org/12723011/diff/1/apps/app_shim/app_shim_messag...
File apps/app_shim/app_shim_messages.h (right):

https://codereview.chromium.org/12723011/diff/1/apps/app_shim/app_shim_messag...
apps/app_shim/app_shim_messages.h:24:
IPC_MESSAGE_CONTROL0(AppShimHostMsg_FocusApp)
Comment

https://codereview.chromium.org/12723011/diff/1/chrome/app/chrome_main_app_mo...
File chrome/app/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/12723011/diff/1/chrome/app/chrome_main_app_mo...
chrome/app/chrome_main_app_mode_mac.mm:54: void
OnDidActivateApplicationNotification(NSNotification* notification);
comment and newline after this

[jeremya, 2013-03-18 04:54:05]

https://codereview.chromium.org/12723011/diff/1/apps/app_shim/app_shim_host_m...
File apps/app_shim/app_shim_host_mac.cc (right):

https://codereview.chromium.org/12723011/diff/1/apps/app_shim/app_shim_host_m...
apps/app_shim/app_shim_host_mac.cc:61: void AppShimHost::OnFocus() {
On 2013/03/18 03:33:12, koz wrote:
> DCHECK(CalledOnValidThread())

Done.

https://codereview.chromium.org/12723011/diff/1/apps/app_shim/app_shim_host_m...
apps/app_shim/app_shim_host_mac.cc:62: // TODO move
On 2013/03/18 03:33:12, koz wrote:
> Move to where?

Oops, this was left over from when the FocusWindowSet code was still inline
here.

https://codereview.chromium.org/12723011/diff/1/apps/app_shim/app_shim_host_m...
File apps/app_shim/app_shim_host_mac.h (right):

https://codereview.chromium.org/12723011/diff/1/apps/app_shim/app_shim_host_m...
apps/app_shim/app_shim_host_mac.h:63: // TODO comment
On 2013/03/18 03:33:12, koz wrote:
> TODO

Done.

https://codereview.chromium.org/12723011/diff/1/apps/app_shim/app_shim_host_m...
File apps/app_shim/app_shim_host_manager_mac.mm (right):

https://codereview.chromium.org/12723011/diff/1/apps/app_shim/app_shim_host_m...
apps/app_shim/app_shim_host_manager_mac.mm:15: #import
"chrome/common/mac/app_mode_common.h"
On 2013/03/18 03:33:12, koz wrote:
> Is this necessary? Other places #include this file.

I guess not. Removed.

https://codereview.chromium.org/12723011/diff/1/apps/app_shim/app_shim_messag...
File apps/app_shim/app_shim_messages.h (right):

https://codereview.chromium.org/12723011/diff/1/apps/app_shim/app_shim_messag...
apps/app_shim/app_shim_messages.h:24:
IPC_MESSAGE_CONTROL0(AppShimHostMsg_FocusApp)
On 2013/03/18 03:33:12, koz wrote:
> Comment

Done.

https://codereview.chromium.org/12723011/diff/1/chrome/app/chrome_main_app_mo...
File chrome/app/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/12723011/diff/1/chrome/app/chrome_main_app_mo...
chrome/app/chrome_main_app_mode_mac.mm:54: void
OnDidActivateApplicationNotification(NSNotification* notification);
On 2013/03/18 03:33:12, koz wrote:
> comment and newline after this

Done, plus changed the function to only be called when the notification refers
to this application.

[koz (OOO until 15th September), 2013-03-18 05:10:57]

Apps changes lgtm!

[palmer, 2013-03-18 21:03:31]

IPC security LGTM

[Nico, 2013-03-18 22:50:59]

lgtm

Do you have a plan for testing app shims?

https://codereview.chromium.org/12723011/diff/13001/chrome/app/chrome_main_ap...
File chrome/app/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/12723011/diff/13001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:111: NSRunningApplication* activated_app
=
nit: block bodies are supposed to be indented 4 spaces afaik

https://codereview.chromium.org/12723011/diff/13001/ui/base/cocoa/focus_windo...
File ui/base/cocoa/focus_window_set.mm (right):

https://codereview.chromium.org/12723011/diff/13001/ui/base/cocoa/focus_windo...
ui/base/cocoa/focus_window_set.mm:14: NSWindow* last_window = nil;
Call this frontmost_window maybe

[jeremya, 2013-03-18 23:17:13]

I'm already testing the AppShimHost by mocking out the IPC channel, and the IPC
channel itself has a couple of tests as well. As for integration tests, I think
it would make sense to fire up an app shim in the same process as the app shim
host, mock out the IPC connection between them and just drive them directly.
Shouldn't be too hard :)

https://codereview.chromium.org/12723011/diff/13001/chrome/app/chrome_main_ap...
File chrome/app/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/12723011/diff/13001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:111: NSRunningApplication* activated_app
=
On 2013/03/18 22:50:59, Nico wrote:
> nit: block bodies are supposed to be indented 4 spaces afaik

Done.

https://codereview.chromium.org/12723011/diff/13001/ui/base/cocoa/focus_windo...
File ui/base/cocoa/focus_window_set.mm (right):

https://codereview.chromium.org/12723011/diff/13001/ui/base/cocoa/focus_windo...
ui/base/cocoa/focus_window_set.mm:14: NSWindow* last_window = nil;
On 2013/03/18 22:50:59, Nico wrote:
> Call this frontmost_window maybe

Done.

[commit-bot: I haz the power, 2013-03-18 23:18:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremya@chromium.org/12723011/19001

[commit-bot: I haz the power, 2013-03-19 01:09:53]

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

[jeremya, 2013-03-19 03:09:28]

Committed patchset #4 manually as r188926 (presubmit successful).