Implement WebContentsModalDialogManagerDelegate for ShellWindow

Implement WebContentsModalDialogManagerDelegate for ShellWindow

Use the web contents modal dialog positioning abstraction for app shell
windows. Implement support for Views and Gtk, stubs for Cocoa.

This abstraction will support the use of WebContents in code outside of
src/chrome/browser by allowing dialogs modal to web contents to be hosted on any
window containing a WebContents and giving the host control over the
positioning, without the host having to know the type of the dialog and vice
versa. The interface has already been implemented for the browser window; this
implements it for the other window that currently hosts WebContents,
ShellWindow.


BUG=157161, 225945
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192878

[Mike Wittman, 2013-04-03 23:11:32]

PTAL

Ben: chrome/browser/ui/views/*
Elliot: chrome/browser/ui/gtk/*
Sailesh: chrome/browser/ui/cocoa/*
Aaron: chrome/browser/ui/extensions/*

[Elliot Glaysher, 2013-04-04 17:03:34]

gtk lgtm

[Ben Goodger (Google), 2013-04-04 17:34:33]

lgtm

https://codereview.chromium.org/13588007/diff/4001/chrome/browser/ui/cocoa/ex...
File chrome/browser/ui/cocoa/extensions/native_app_window_cocoa.h (right):

https://codereview.chromium.org/13588007/diff/4001/chrome/browser/ui/cocoa/ex...
chrome/browser/ui/cocoa/extensions/native_app_window_cocoa.h:118: 
delete nl

[jeremya, 2013-04-04 18:13:34]

Could you expand the commit message a little? What is this needed for?

[Mike Wittman, 2013-04-04 18:32:09]

On 2013/04/04 18:13:34, jeremya wrote:
> Could you expand the commit message a little? What is this needed for?

Done.  Let me know if it's still unclear.

[jeremya, 2013-04-04 19:59:17]

https://codereview.chromium.org/13588007/diff/4001/chrome/browser/ui/gtk/exte...
File chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc (right):

https://codereview.chromium.org/13588007/diff/4001/chrome/browser/ui/gtk/exte...
chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:360:
OnPositionRequiresUpdate());
This code could live in OnNativeWindowChanged(), I think.

https://codereview.chromium.org/13588007/diff/4001/chrome/browser/ui/gtk/exte...
File chrome/browser/ui/gtk/extensions/native_app_window_gtk.h (right):

https://codereview.chromium.org/13588007/diff/4001/chrome/browser/ui/gtk/exte...
chrome/browser/ui/gtk/extensions/native_app_window_gtk.h:10: #include
"base/observer_list.h"
Would it make more sense to have ShellWindow be the HostDelegate, rather than
adding an observer list to each native implementation separately?

[Mike Wittman, 2013-04-04 22:56:07]

https://codereview.chromium.org/13588007/diff/4001/chrome/browser/ui/cocoa/ex...
File chrome/browser/ui/cocoa/extensions/native_app_window_cocoa.h (right):

https://codereview.chromium.org/13588007/diff/4001/chrome/browser/ui/cocoa/ex...
chrome/browser/ui/cocoa/extensions/native_app_window_cocoa.h:118: 
On 2013/04/04 17:34:33, Ben Goodger (Google) wrote:
> delete nl

Done.

https://codereview.chromium.org/13588007/diff/4001/chrome/browser/ui/gtk/exte...
File chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc (right):

https://codereview.chromium.org/13588007/diff/4001/chrome/browser/ui/gtk/exte...
chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:360:
OnPositionRequiresUpdate());
On 2013/04/04 19:59:17, jeremya wrote:
> This code could live in OnNativeWindowChanged(), I think.

See comment in the .h file.  Also, OnNativeWindowChanged() is invoked in
multiple circumstances, only some of which necessitate a position change for the
web contents modal dialog.

https://codereview.chromium.org/13588007/diff/4001/chrome/browser/ui/gtk/exte...
File chrome/browser/ui/gtk/extensions/native_app_window_gtk.h (right):

https://codereview.chromium.org/13588007/diff/4001/chrome/browser/ui/gtk/exte...
chrome/browser/ui/gtk/extensions/native_app_window_gtk.h:10: #include
"base/observer_list.h"
On 2013/04/04 19:59:17, jeremya wrote:
> Would it make more sense to have ShellWindow be the HostDelegate, rather than
> adding an observer list to each native implementation separately?

I don't think so. The NativeWidget subclasses are the natural classes to
implement the HostDelegate since those are where the positioning computation and
knowledge of when the positioning requires updating resides. Doing the
notification in ShellWindow would be undesirable because it would split the
responsibility of implementing the interface across two classes.

[sail, 2013-04-05 15:04:24]

cocoa/* LGTM

[Mike Wittman, 2013-04-05 15:35:13]

-aa who is ooo
+miket

Mike, can you review the chrome/browser/ui/extensions part of this change?

[benwells, 2013-04-07 23:02:46]

c/b/ui/extensions lgtm

[Mike Wittman, 2013-04-08 16:26:54]

On 2013/04/07 23:02:46, benwells wrote:
> c/b/ui/extensions lgtm

Thanks! Comitting.

[commit-bot: I haz the power, 2013-04-08 16:27:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wittman@chromium.org/13588007/14001

[commit-bot: I haz the power, 2013-04-08 16:36:24]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-08 19:24:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wittman@chromium.org/13588007/14001

[commit-bot: I haz the power, 2013-04-08 20:20:04]

Change committed as 192878