Cocoa: Support constrained windows for platform apps

Cocoa: Support constrained windows for platform apps

This CL adds support for constrained windows to ShellWindowController which is used by platform apps.

BUG=140593
TEST=Verified that constrained windows on tab windows still work. Ran the media gallery test app and verified that opening constrained windows inside a shell window worked.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151191

[Avi (use Gerrit), 2012-08-04 22:50:04]

lgtm

[jeremya, 2012-08-05 22:32:47]

Awesome! I thought this would take a bunch more code than it did :)

https://chromiumcodereview.appspot.com/10823176/diff/2001/chrome/browser/ui/c...
File chrome/browser/ui/cocoa/constrained_window_mac.mm (right):

https://chromiumcodereview.appspot.com/10823176/diff/2001/chrome/browser/ui/c...
chrome/browser/ui/cocoa/constrained_window_mac.mm:116: window = [window
parentWindow];
If the window is the TopLevelNativeWindow, will it have a parent?

https://chromiumcodereview.appspot.com/10823176/diff/2001/chrome/browser/ui/c...
chrome/browser/ui/cocoa/constrained_window_mac.mm:119:
DCHECK(window_controller);
Are we guaranteed a controller that conforms to ConstrainedWindowSupport?
Perhaps it would be better to bail out here if window_controller is nil. (I'm
thinking of panels and HTML notifications.)

https://chromiumcodereview.appspot.com/10823176/diff/2001/chrome/browser/ui/c...
File chrome/browser/ui/cocoa/extensions/shell_window_cocoa.h (right):

https://chromiumcodereview.appspot.com/10823176/diff/2001/chrome/browser/ui/c...
chrome/browser/ui/cocoa/extensions/shell_window_cocoa.h:27: // Manages per-tab
sheets.
nit: there are no tabs in shell windows :) perhaps "per-window"?

https://chromiumcodereview.appspot.com/10823176/diff/2001/chrome/browser/ui/c...
File chrome/browser/ui/cocoa/extensions/shell_window_cocoa.mm (right):

https://chromiumcodereview.appspot.com/10823176/diff/2001/chrome/browser/ui/c...
chrome/browser/ui/cocoa/extensions/shell_window_cocoa.mm:12: #include
"chrome/browser/ui/tab_contents/tab_contents.h"
I don't think you need this include.

https://chromiumcodereview.appspot.com/10823176/diff/2001/chrome/browser/ui/c...
chrome/browser/ui/cocoa/extensions/shell_window_cocoa.mm:68: -
(void)removeConstrainedWindow:(ConstrainedWindowMac*)window {
Is this NOTREACHED()?

[Robert Sesek, 2012-08-06 22:18:34]

LGTM

https://chromiumcodereview.appspot.com/10823176/diff/2001/chrome/browser/ui/c...
File chrome/browser/ui/cocoa/constrained_window_mac.mm (right):

https://chromiumcodereview.appspot.com/10823176/diff/2001/chrome/browser/ui/c...
chrome/browser/ui/cocoa/constrained_window_mac.mm:112:
@protocol(ConstrainedWindowSupport)]) {
nit: indent4 from here

https://chromiumcodereview.appspot.com/10823176/diff/2001/chrome/browser/ui/c...
chrome/browser/ui/cocoa/constrained_window_mac.mm:119:
DCHECK(window_controller);
On 2012/08/05 22:32:47, jeremya wrote:
> Are we guaranteed a controller that conforms to ConstrainedWindowSupport?
> Perhaps it would be better to bail out here if window_controller is nil. (I'm
> thinking of panels and HTML notifications.)

That'll happen with the if-check below because you'd be sending a message to
nil.

https://chromiumcodereview.appspot.com/10823176/diff/2001/chrome/browser/ui/c...
File chrome/browser/ui/cocoa/extensions/shell_window_cocoa.h (right):

https://chromiumcodereview.appspot.com/10823176/diff/2001/chrome/browser/ui/c...
chrome/browser/ui/cocoa/extensions/shell_window_cocoa.h:26: ShellWindowCocoa*
shellWindow_; // Weak; owns self.
nit: two spaces btwn ; and //

[commit-bot: I haz the power, 2012-08-08 22:51:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10823176/9001

[commit-bot: I haz the power, 2012-08-08 23:32:33]

Try job failure for 10823176-9001 (retry) on android for steps "compile, build"
(clobber build).
It's a second try, previously, steps "compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[sail, 2012-08-09 00:25:53]

http://codereview.chromium.org/10823176/diff/2001/chrome/browser/ui/cocoa/con...
File chrome/browser/ui/cocoa/constrained_window_mac.mm (right):

http://codereview.chromium.org/10823176/diff/2001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/constrained_window_mac.mm:112:
@protocol(ConstrainedWindowSupport)]) {
On 2012/08/06 22:18:34, rsesek wrote:
> nit: indent4 from here

Done.

http://codereview.chromium.org/10823176/diff/2001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/constrained_window_mac.mm:116: window = [window
parentWindow];
On 2012/08/05 22:32:47, jeremya wrote:
> If the window is the TopLevelNativeWindow, will it have a parent?

No, the parent will be nil.

http://codereview.chromium.org/10823176/diff/2001/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/shell_window_cocoa.h (right):

http://codereview.chromium.org/10823176/diff/2001/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/shell_window_cocoa.h:26: ShellWindowCocoa*
shellWindow_; // Weak; owns self.
On 2012/08/06 22:18:34, rsesek wrote:
> nit: two spaces btwn ; and //

Done.

http://codereview.chromium.org/10823176/diff/2001/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/shell_window_cocoa.h:27: // Manages per-tab
sheets.
On 2012/08/05 22:32:47, jeremya wrote:
> nit: there are no tabs in shell windows :) perhaps "per-window"?

Done.

[commit-bot: I haz the power, 2012-08-09 00:28:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10823176/9001

[jeremya, 2012-08-09 00:32:48]

http://codereview.chromium.org/10823176/diff/2001/chrome/browser/ui/cocoa/con...
File chrome/browser/ui/cocoa/constrained_window_mac.mm (right):

http://codereview.chromium.org/10823176/diff/2001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/constrained_window_mac.mm:116: window = [window
parentWindow];
On 2012/08/09 00:25:54, sail wrote:
> On 2012/08/05 22:32:47, jeremya wrote:
> > If the window is the TopLevelNativeWindow, will it have a parent?
> 
> No, the parent will be nil.

Then why check here? if [window parentWindow] is always nil, there's no need for
the loop, right?

[sail, 2012-08-09 00:40:42]

http://codereview.chromium.org/10823176/diff/2001/chrome/browser/ui/cocoa/con...
File chrome/browser/ui/cocoa/constrained_window_mac.mm (right):

http://codereview.chromium.org/10823176/diff/2001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/constrained_window_mac.mm:116: window = [window
parentWindow];
On 2012/08/09 00:32:48, jeremya wrote:
> On 2012/08/09 00:25:54, sail wrote:
> > On 2012/08/05 22:32:47, jeremya wrote:
> > > If the window is the TopLevelNativeWindow, will it have a parent?
> > 
> > No, the parent will be nil.
> 
> Then why check here? if [window parentWindow] is always nil, there's no need
for
> the loop, right?

The idea is that we want the window controller of the top most window.

For example, if this window is a bubble, then we get the window controller of
the window hosting the bubble.

[commit-bot: I haz the power, 2012-08-10 18:05:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10823176/9001

[commit-bot: I haz the power, 2012-08-10 18:06:01]

Failed to apply patch for
chrome/browser/ui/cocoa/extensions/shell_window_cocoa.h:
While running patch -p1 --forward --force;
patching file chrome/browser/ui/cocoa/extensions/shell_window_cocoa.h
Hunk #1 FAILED at 8.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/ui/cocoa/extensions/shell_window_cocoa.h.rej

[commit-bot: I haz the power, 2012-08-10 18:19:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10823176/2003

[commit-bot: I haz the power, 2012-08-10 18:27:19]

Try job failure for 10823176-2003 on linux_rel for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-08-10 18:59:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10823176/2003

[commit-bot: I haz the power, 2012-08-10 20:12:48]

Try job failure for 10823176-2003 (retry) on mac_rel for steps "browser_tests,
unit_tests".
It's a second try, previously, steps "browser_tests, unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[sail, 2012-08-10 23:09:37]

Changed the DCHECK(window_controller) to DCHECK(!window || window_controller).
This handles the case where a constrained window is shown for a background tab.

[commit-bot: I haz the power, 2012-08-10 23:10:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10823176/7004

[commit-bot: I haz the power, 2012-08-11 01:35:58]

Try job failure for 10823176-7004 (retry) on win_rel for step "browser_tests".
It's a second try, previously, steps "interactive_ui_tests, browser_tests"
failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-08-11 04:26:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10823176/7004

[commit-bot: I haz the power, 2012-08-12 01:57:06]

Change committed as 151191