Close all windows when app shim quits.

Close all windows when app shim quits.

This adds a new IPC message QuitApp which is sent by the shim when the user
quits the shim (right click -> Quit). The AppShimHost responds by closing all
windows associated with the app. The shim actually quits when the extension
eventually closes and AppShimHost closes the channel to the shim.

BUG=168080
TEST=Start an app using its shim.
Right click the app in the dock and select quit.
The shell windows of that app should close.

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

[jackhou1, 2013-05-22 01:58:22]

This basically just makes right-click -> Quit work. You can't Command+Q because
that closes Chrome. After the windows are closed, the shim doesn't quit until
the extension closes, so we probably need to change that to be more responsive.
These will be for future CLs though.

[jackhou1, 2013-05-24 07:23:38]

PTAL

[tapted, 2013-05-24 08:43:51]

Don't forget to set a bug number and TEST=

And note that later we'll also need to get a security person to have a look-see,
since we're changing IPC messages

https://codereview.chromium.org/14579005/diff/7001/apps/app_shim/extension_ap...
File apps/app_shim/extension_app_shim_handler_mac.mm (right):

https://codereview.chromium.org/14579005/diff/7001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.mm:79: for
(extensions::ShellWindowRegistry::const_iterator i = windows.begin();
perhaps name this iterator `it` to be consistent (and I like it better than
`i`).

https://codereview.chromium.org/14579005/diff/7001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.mm:95: NSWindow* window =
(*it)->GetNativeWindow();
What about (*it)->GetBaseWindow()->Close();

That should hit the following in native_app_window_cocoa.mm

void NativeAppWindowCocoa::Close() {
  [window() performClose:nil];
}

and would avoid this file having to get infected by Objective C

https://codereview.chromium.org/14579005/diff/7001/chrome/app/chrome_main_app...
File chrome/app/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/14579005/diff/7001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:42: @interface AppShimDelegate : NSObject
{
NSObject -> NSObject<NSApplicationDelegate> to say it conforms to this protocol

https://codereview.chromium.org/14579005/diff/7001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:44: AppShimController*
appShimController_;
needs a comment "  // Weak. Owns us."

https://codereview.chromium.org/14579005/diff/7001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:112:
nsapp_delegate_.reset([[AppShimDelegate alloc] init]);
perhaps initWithController rather than the separate appShimController accessor

https://codereview.chromium.org/14579005/diff/7001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:153: void AppShimController::Quit() {
will it be less confusing if this is called 'Close', or 'CloseChannel'?

https://codereview.chromium.org/14579005/diff/7001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:165: - (id)init {
init functions all follow the pattern

- (id) init {
  if ((self = [super initXXX:YYY])) {
    /* initialize members */
  }
  return self;
}

XXX:YYY can be dropped if you want the superclass init with no arguments

https://codereview.chromium.org/14579005/diff/7001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:166: terminateNow_ = NO;
don't need to initialize objective C members that are zeroes (alloc bzeros the
initial object)

https://codereview.chromium.org/14579005/diff/7001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:177: return NSTerminateCancel;
I think this needs to set a flag like 'terminateRequsted_ = YES' then `return
NSTerminateLater`, then...

https://codereview.chromium.org/14579005/diff/7001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:181: terminateNow_ = YES;
...then after this check

if (terminateRequested_) {
  replyToApplicationShouldTerminate:NSTerminateNow;
  return;
}


reason: returning NSTerminateCancel will immediately cancel a user-initiated
system shut-down. We don't want to do that.

https://codereview.chromium.org/14579005/diff/7001/chrome/browser/resources/n...
File chrome/browser/resources/ntp4/apps_page.js (left):

https://codereview.chromium.org/14579005/diff/7001/chrome/browser/resources/n...
chrome/browser/resources/ntp4/apps_page.js:76: if (!(cr.isChromeOS || cr.isMac))
{
this should be in a separate CL

[jackhou1, 2013-05-27 01:02:47]

https://codereview.chromium.org/14579005/diff/7001/apps/app_shim/extension_ap...
File apps/app_shim/extension_app_shim_handler_mac.mm (right):

https://codereview.chromium.org/14579005/diff/7001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.mm:79: for
(extensions::ShellWindowRegistry::const_iterator i = windows.begin();
On 2013/05/24 08:43:51, tapted wrote:
> perhaps name this iterator `it` to be consistent (and I like it better than
> `i`).

Done.

https://codereview.chromium.org/14579005/diff/7001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.mm:95: NSWindow* window =
(*it)->GetNativeWindow();
On 2013/05/24 08:43:51, tapted wrote:
> What about (*it)->GetBaseWindow()->Close();
> 
> That should hit the following in native_app_window_cocoa.mm
> 
> void NativeAppWindowCocoa::Close() {
>   [window() performClose:nil];
> }
> 
> and would avoid this file having to get infected by Objective C

Close() is not available on the NativeAppWindow interface. I tried #include
"chrome/browser/ui/cocoa/extensions/native_app_window_cocoa.h" here but
presubmit says it's not allowed by checkdeps. Do I add an exception for it? Or
do you think I should add Close() to the interface?

https://codereview.chromium.org/14579005/diff/7001/chrome/app/chrome_main_app...
File chrome/app/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/14579005/diff/7001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:42: @interface AppShimDelegate : NSObject
{
On 2013/05/24 08:43:51, tapted wrote:
> NSObject -> NSObject<NSApplicationDelegate> to say it conforms to this
protocol

Done.

https://codereview.chromium.org/14579005/diff/7001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:44: AppShimController*
appShimController_;
On 2013/05/24 08:43:51, tapted wrote:
> needs a comment "  // Weak. Owns us."

Done.

https://codereview.chromium.org/14579005/diff/7001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:112:
nsapp_delegate_.reset([[AppShimDelegate alloc] init]);
On 2013/05/24 08:43:51, tapted wrote:
> perhaps initWithController rather than the separate appShimController accessor

Done.

https://codereview.chromium.org/14579005/diff/7001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:153: void AppShimController::Quit() {
On 2013/05/24 08:43:51, tapted wrote:
> will it be less confusing if this is called 'Close', or 'CloseChannel'?

Yeah, agreed.

https://codereview.chromium.org/14579005/diff/7001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:165: - (id)init {
On 2013/05/24 08:43:51, tapted wrote:
> init functions all follow the pattern
> 
> - (id) init {
>   if ((self = [super initXXX:YYY])) {
>     /* initialize members */
>   }
>   return self;
> }
> 
> XXX:YYY can be dropped if you want the superclass init with no arguments

Done.

https://codereview.chromium.org/14579005/diff/7001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:166: terminateNow_ = NO;
On 2013/05/24 08:43:51, tapted wrote:
> don't need to initialize objective C members that are zeroes (alloc bzeros the
> initial object)

Done.

https://codereview.chromium.org/14579005/diff/7001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:177: return NSTerminateCancel;
On 2013/05/24 08:43:51, tapted wrote:
> I think this needs to set a flag like 'terminateRequsted_ = YES' then `return
> NSTerminateLater`, then...

Done.

https://codereview.chromium.org/14579005/diff/7001/chrome/app/chrome_main_app...
chrome/app/chrome_main_app_mode_mac.mm:181: terminateNow_ = YES;
On 2013/05/24 08:43:51, tapted wrote:
> ...then after this check
> 
> if (terminateRequested_) {
>   replyToApplicationShouldTerminate:NSTerminateNow;
>   return;
> }
> 
> 
> reason: returning NSTerminateCancel will immediately cancel a user-initiated
> system shut-down. We don't want to do that.

Done.

https://codereview.chromium.org/14579005/diff/7001/chrome/browser/resources/n...
File chrome/browser/resources/ntp4/apps_page.js (left):

https://codereview.chromium.org/14579005/diff/7001/chrome/browser/resources/n...
chrome/browser/resources/ntp4/apps_page.js:76: if (!(cr.isChromeOS || cr.isMac))
{
On 2013/05/24 08:43:51, tapted wrote:
> this should be in a separate CL

Done.

[tapted, 2013-05-27 01:13:56]

Can you re-upload? It looks like `git cl` didn't get around to uploading the
files properly for the last patchset.

https://codereview.chromium.org/14579005/diff/7001/apps/app_shim/extension_ap...
File apps/app_shim/extension_app_shim_handler_mac.mm (right):

https://codereview.chromium.org/14579005/diff/7001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.mm:95: NSWindow* window =
(*it)->GetNativeWindow();
On 2013/05/27 01:02:47, jackhou1 wrote:
> On 2013/05/24 08:43:51, tapted wrote:
> > What about (*it)->GetBaseWindow()->Close();
> > 
> > That should hit the following in native_app_window_cocoa.mm
> > 
> > void NativeAppWindowCocoa::Close() {
> >   [window() performClose:nil];
> > }
> > 
> > and would avoid this file having to get infected by Objective C
> 
> Close() is not available on the NativeAppWindow interface.

Sure it is! -- should be inherited from ui::BaseWindow. What's the error? (I'm
guessing ~use of incomplete object, NativeAppWindow forward-declared <here>)

> I tried #include
> "chrome/browser/ui/cocoa/extensions/native_app_window_cocoa.h"

You shouldn't need this one. But you will probably need 

"chrome/browser/ui/cocoa/extensions/native_app_window.h"

since it's forward-declared in shell_window.h.

> here but
> presubmit says it's not allowed by checkdeps. Do I add an exception for it? Or
> do you think I should add Close() to the interface?

I'd add an exception for native_app_window.h. NativeAppWindow belongs in
src/apps (eventually), so I think this is the right thing to do, until that gets
moved.

[jackhou1, 2013-05-27 05:04:51]

https://codereview.chromium.org/14579005/diff/7001/apps/app_shim/extension_ap...
File apps/app_shim/extension_app_shim_handler_mac.mm (right):

https://codereview.chromium.org/14579005/diff/7001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.mm:95: NSWindow* window =
(*it)->GetNativeWindow();
On 2013/05/27 01:13:56, tapted wrote:
> On 2013/05/27 01:02:47, jackhou1 wrote:
> > On 2013/05/24 08:43:51, tapted wrote:
> > > What about (*it)->GetBaseWindow()->Close();
> > > 
> > > That should hit the following in native_app_window_cocoa.mm
> > > 
> > > void NativeAppWindowCocoa::Close() {
> > >   [window() performClose:nil];
> > > }
> > > 
> > > and would avoid this file having to get infected by Objective C
> > 
> > Close() is not available on the NativeAppWindow interface.
> 
> Sure it is! -- should be inherited from ui::BaseWindow. What's the error? (I'm
> guessing ~use of incomplete object, NativeAppWindow forward-declared <here>)
> 
> > I tried #include
> > "chrome/browser/ui/cocoa/extensions/native_app_window_cocoa.h"
> 
> You shouldn't need this one. But you will probably need 
> 
> "chrome/browser/ui/cocoa/extensions/native_app_window.h"
> 
> since it's forward-declared in shell_window.h.
> 
> > here but
> > presubmit says it's not allowed by checkdeps. Do I add an exception for it?
Or
> > do you think I should add Close() to the interface?
> 
> I'd add an exception for native_app_window.h. NativeAppWindow belongs in
> src/apps (eventually), so I think this is the right thing to do, until that
gets
> moved.

Done.

[tapted, 2013-05-27 06:44:29]

lgtm with some nits.

https://codereview.chromium.org/14579005/diff/31001/apps/app_shim/app_shim_me...
File apps/app_shim/app_shim_messages.h (right):

https://codereview.chromium.org/14579005/diff/31001/apps/app_shim/app_shim_me...
apps/app_shim/app_shim_messages.h:30: // terminates the channel to the shim, and
the shim exits.
nit: Maybe it is confusing using quit, close and exit in the same comment (all
with different meanings!)

Perhaps something along the lines of, ~"Sent when the shim process receives a
request to terminate. Once all the app's windows have closed, and the extension
is unloaded, the AppShimHost closes the channel. The shim process then completes
the terminate request and exits."

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

https://codereview.chromium.org/14579005/diff/31001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac.cc:17: #include
"content/public/browser/web_contents.h"
is this used?

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

https://codereview.chromium.org/14579005/diff/31001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:80: // Quits the app shim process.
nit: Quits -> Terminates

https://codereview.chromium.org/14579005/diff/31001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:164: if (self = [super init]) {
should have double parentheses around the assignment in if condition.. Compiler
usually has a whinge about this (are you using xcodebuild maybe?)

[jackhou1, 2013-05-28 07:30:21]

https://codereview.chromium.org/14579005/diff/31001/apps/app_shim/app_shim_me...
File apps/app_shim/app_shim_messages.h (right):

https://codereview.chromium.org/14579005/diff/31001/apps/app_shim/app_shim_me...
apps/app_shim/app_shim_messages.h:30: // terminates the channel to the shim, and
the shim exits.
On 2013/05/27 06:44:29, tapted wrote:
> nit: Maybe it is confusing using quit, close and exit in the same comment (all
> with different meanings!)
> 
> Perhaps something along the lines of, ~"Sent when the shim process receives a
> request to terminate. Once all the app's windows have closed, and the
extension
> is unloaded, the AppShimHost closes the channel. The shim process then
completes
> the terminate request and exits."

Done.

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

https://codereview.chromium.org/14579005/diff/31001/apps/app_shim/extension_a...
apps/app_shim/extension_app_shim_handler_mac.cc:17: #include
"content/public/browser/web_contents.h"
On 2013/05/27 06:44:29, tapted wrote:
> is this used?

Nope, removed.

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

https://codereview.chromium.org/14579005/diff/31001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:80: // Quits the app shim process.
On 2013/05/27 06:44:29, tapted wrote:
> nit: Quits -> Terminates

Done.

https://codereview.chromium.org/14579005/diff/31001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:164: if (self = [super init]) {
On 2013/05/27 06:44:29, tapted wrote:
> should have double parentheses around the assignment in if condition..
Compiler
> usually has a whinge about this (are you using xcodebuild maybe?)

Done.

[jackhou1, 2013-05-28 07:33:41]

benwells, could you please review for OWNERS in apps/?

thakis, could you please review for OWNERS in chrome/?

[Nico, 2013-05-28 19:54:33]

I'll wait for benwells, this looks like mostly an apps/ CL. Do you need security
reviews for IPC changes in apps? (If not, is this just an oversight? It's needed
in content/ and chrome/)

https://codereview.chromium.org/14579005/diff/48001/apps/DEPS
File apps/DEPS (right):

https://codereview.chromium.org/14579005/diff/48001/apps/DEPS#newcode14
apps/DEPS:14: "+chrome/browser/ui/extensions/native_app_window.h",
The TODO above says "remove all these", so adding more probably isn't the way to
go?

[benwells, 2013-05-29 04:28:00]

This lgtm. As Nico mentions we should be getting all IPC changes looked at by a
security person (unless they tell us the shim can be excluded from this).

https://codereview.chromium.org/14579005/diff/48001/apps/DEPS
File apps/DEPS (right):

https://codereview.chromium.org/14579005/diff/48001/apps/DEPS#newcode14
apps/DEPS:14: "+chrome/browser/ui/extensions/native_app_window.h",
On 2013/05/28 19:54:33, Nico wrote:
> The TODO above says "remove all these", so adding more probably isn't the way
to
> go?

This is fine for now. native_app_window.h should be in apps, and I'm looking at
moving it out.

[jackhou1, 2013-05-29 04:50:43]

palmer, could you please do security review for app shim IPC changes?

[palmer, 2013-05-29 21:52:21]

LGTM

[Nico, 2013-05-29 23:01:09]

lgtm

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

https://codereview.chromium.org/14579005/diff/53001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:114: [NSApp setDelegate:nsapp_delegate_];
maybe DCHECK that no delegate is set yet?

[jackhou1, 2013-05-29 23:50:26]

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

https://codereview.chromium.org/14579005/diff/53001/chrome/app/chrome_main_ap...
chrome/app/chrome_main_app_mode_mac.mm:114: [NSApp setDelegate:nsapp_delegate_];
On 2013/05/29 23:01:09, Nico wrote:
> maybe DCHECK that no delegate is set yet?

Done.

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

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

[commit-bot: I haz the power, 2013-05-30 06:18:43]

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

[commit-bot: I haz the power, 2013-05-30 06:40:21]

Change committed as 203098