Fix the content_browsertests everywhere maybe.

Finally rip the global StackingClient bandaid off.

- Removes the aura::client::SetStackingClient(StackingClient*) interface.
- Moves the ash StackingController from a singleton owned by ash::Shell to one StackingController per RootWindow owned by the ash::RootWindowController. (Also removes a spurious delegate method, where every implementation creates the same object, including tests.)
- Removes the global DesktopStackingClient and related interfaces.
- Fix the ChromeViewsDelegate so that it still sets context in chromeos builds.
- Rename content::ShellStackingClientAsh to content::MinimalAsh to reflect what it really does.

In addition, the following fix ups apply:

- Previously, WebContentsViewAura asserted that it needed a context window. Now if no context window is provided, it isn't added to an aura hierarchy. There are times when that context doesn't exist: various chromeos dialogs that directly invoke WebView and toplevel extension/app windows that are created from background pages that don't have contexts.
- Because of that, added a few of NULL checking to RenderWidgetHostViewAura. (Other methods NULL checked Window::GetRootWindow(), but there were a few that didn't and relied on the old NULL means lookup in the StackingClient behaviour.)

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

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

[Elliot Glaysher, 2013-01-14 22:56:39]

This is it. This should be the final patch that pulls out the global
StackingClient and replaces it with a per-RootWindow StackingClient.

https://codereview.chromium.org/11829040/diff/46048/ui/views/widget/native_wi...
File ui/views/widget/native_widget_aura.cc (left):

https://codereview.chromium.org/11829040/diff/46048/ui/views/widget/native_wi...
ui/views/widget/native_widget_aura.cc:142: // DCHECK_NE(params.GetParent(),
params.context);
This DCHECK_NE is probably incorrect; just up-function, context is set to parent
in some cases.

[Elliot Glaysher, 2013-01-14 23:15:19]

https://codereview.chromium.org/11829040/diff/38006/chrome/browser/ui/views/s...
File chrome/browser/ui/views/simple_message_box_views.cc (right):

https://codereview.chromium.org/11829040/diff/38006/chrome/browser/ui/views/s...
chrome/browser/ui/views/simple_message_box_views.cc:173:
views::Widget::CreateWindow(dialog)->Show();
This is a common thing that I've found: an interface that 95% of the time takes
a real parent used to work with NULL. Now it doesn't. Do you have an opinion on
how this interface should be structured?

[Ben Goodger (Google), 2013-01-15 18:19:52]

lgtm

https://codereview.chromium.org/11829040/diff/38006/chrome/browser/ui/app_mod...
File chrome/browser/ui/app_modal_dialogs/javascript_app_modal_dialog.cc (right):

https://codereview.chromium.org/11829040/diff/38006/chrome/browser/ui/app_mod...
chrome/browser/ui/app_modal_dialogs/javascript_app_modal_dialog.cc:89: // When
we are part of a WebContents that isn't actually being displayed on
Does this happen when a js alert happens before a WC is parented even to the
hidden root window?

https://codereview.chromium.org/11829040/diff/38006/chrome/browser/ui/views/c...
File chrome/browser/ui/views/chrome_views_delegate.cc (right):

https://codereview.chromium.org/11829040/diff/38006/chrome/browser/ui/views/c...
chrome/browser/ui/views/chrome_views_delegate.cc:188: if (params->parent == NULL
&& params->context == NULL && params->top_level) {
!params->parent && !params->context

no braces

https://codereview.chromium.org/11829040/diff/38006/chrome/browser/ui/views/c...
File chrome/browser/ui/views/chrome_web_dialog_view.cc (right):

https://codereview.chromium.org/11829040/diff/38006/chrome/browser/ui/views/c...
chrome/browser/ui/views/chrome_web_dialog_view.cc:25: widget =
views::Widget::CreateWindow(
This is confusing at first glance due to similarity to CreateWindowWithParent, I
wonder if CreateWindow should be called CreateTopLevelWindow?

Fine for that to be a different CL.

https://codereview.chromium.org/11829040/diff/38006/chrome/browser/ui/views/s...
File chrome/browser/ui/views/simple_message_box_views.cc (right):

https://codereview.chromium.org/11829040/diff/38006/chrome/browser/ui/views/s...
chrome/browser/ui/views/simple_message_box_views.cc:173:
views::Widget::CreateWindow(dialog)->Show();
Yeah see my comment on the earlier file.

[Elliot Glaysher, 2013-01-15 19:27:50]

https://codereview.chromium.org/11829040/diff/38006/chrome/browser/ui/app_mod...
File chrome/browser/ui/app_modal_dialogs/javascript_app_modal_dialog.cc (right):

https://codereview.chromium.org/11829040/diff/38006/chrome/browser/ui/app_mod...
chrome/browser/ui/app_modal_dialogs/javascript_app_modal_dialog.cc:89: // When
we are part of a WebContents that isn't actually being displayed on
On 2013/01/15 18:19:52, Ben Goodger (Google) wrote:
> Does this happen when a js alert happens before a WC is parented even to the
> hidden root window?

Which hidden root window? AFAIK, there never was one in pure ash builds, and
it's being removed in this patch on the desktop; the hidden root window is part
of the DesktopStackingClient.

[Ben Goodger (Google), 2013-01-15 19:29:12]

There's a hidden root window that windows with a NULL parent get added to
in the desktop aura builds at least.


On Tue, Jan 15, 2013 at 11:27 AM, <erg@chromium.org> wrote:

>
> https://codereview.chromium.**org/11829040/diff/38006/**
>
chrome/browser/ui/app_modal_**dialogs/javascript_app_modal_**dialog.cc<https://codereview.chromium.org/11829040/diff/38006/chrome/browser/ui/app_modal_dialogs/javascript_app_modal_dialog.cc>
> File chrome/browser/ui/app_modal_**dialogs/javascript_app_modal_**
> dialog.cc
> (right):
>
> https://codereview.chromium.**org/11829040/diff/38006/**
> chrome/browser/ui/app_modal_**dialogs/javascript_app_modal_**
>
dialog.cc#newcode89<https://codereview.chromium.org/11829040/diff/38006/chrome/browser/ui/app_modal_dialogs/javascript_app_modal_dialog.cc#newcode89>
> chrome/browser/ui/app_modal_**dialogs/javascript_app_modal_**dialog.cc:89:
> // When we are part of a WebContents that isn't actually being displayed
> on
> On 2013/01/15 18:19:52, Ben Goodger (Google) wrote:
>
>> Does this happen when a js alert happens before a WC is parented even
>>
> to the
>
>> hidden root window?
>>
>
> Which hidden root window? AFAIK, there never was one in pure ash builds,
> and it's being removed in this patch on the desktop; the hidden root
> window is part of the DesktopStackingClient.
>
>
https://codereview.chromium.**org/11829040/<https://codereview.chromium.org/1...
>

[Elliot Glaysher, 2013-01-15 20:19:55]

On 2013/01/15 19:29:12, Ben Goodger (Google) wrote:
> There's a hidden root window that windows with a NULL parent get added to
> in the desktop aura builds at least.

Yes, that's the RootWindow that's being removed in this patch (See the deletion
of desktop_stacking_client.{cc,h}). One of the first patches in this series was
to make NULL not a valid parent and force users to go through
Window::SetDefaultParentByRootWindow(). That method (and the previous NULL magic
value to SetParent) looked up the window from the global stacking client, which
is now going away.

[commit-bot: I haz the power, 2013-01-15 20:22:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/11829040/45048

[Ben Goodger (Google), 2013-01-15 20:23:15]

ic


On Tue, Jan 15, 2013 at 12:19 PM, <erg@chromium.org> wrote:

> On 2013/01/15 19:29:12, Ben Goodger (Google) wrote:
>
>> There's a hidden root window that windows with a NULL parent get added to
>> in the desktop aura builds at least.
>>
>
> Yes, that's the RootWindow that's being removed in this patch (See the
> deletion
> of desktop_stacking_client.{cc,h}**). One of the first patches in this
> series was
> to make NULL not a valid parent and force users to go through
> Window::**SetDefaultParentByRootWindow()**. That method (and the previous
> NULL magic
> value to SetParent) looked up the window from the global stacking client,
> which
> is now going away.
>
>
https://codereview.chromium.**org/11829040/<https://codereview.chromium.org/1...
>

[commit-bot: I haz the power, 2013-01-15 23:03:44]

Retried try job too often on win7_aura for step(s) interactive_ui_tests,
views_unittests

[Elliot Glaysher, 2013-01-18 20:31:37]

Ben: Enough here has changed that I want your OK. The only real differences are
in render_widget_host_view_aura.cc and *screen_win.cc.

With these changes, RWHVA no longer need to be parented to a RootWindow.

https://codereview.chromium.org/11829040/diff/94002/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/11829040/diff/94002/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:469:
aura::RootWindow* root_window = window_->GetRootWindow();
If you search through this file, you'll see that half the time we do NULL
checking on GetRootWindow(), and half the time we don't. The new changes to this
patch add NULL checks so that we don't crash when we are unparented to an aura
window hierarchy.

https://codereview.chromium.org/11829040/diff/94002/ui/gfx/screen_win.cc
File ui/gfx/screen_win.cc (right):

https://codereview.chromium.org/11829040/diff/94002/ui/gfx/screen_win.cc#newc...
ui/gfx/screen_win.cc:60: if (!window_hwnd) {
I'd like to fix this at a higher level, but there are too many callers for this
to be one patch. (Linux works by completely ignoring |window| alltogether.)

[Ben Goodger (Google), 2013-01-18 23:10:19]

lgtm