Desktop aura: Continue threading context through views.

Desktop aura: Continue threading context through views.

These threads a .context parameter through most tests in views_unittests to widget creation (the handful that remain are Hard and will be addressed in a separate patch; I'm trying to keep these more manageable.)

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

[Elliot Glaysher, 2012-11-29 22:05:54]

I will run trybots later.

[Ben Goodger (Google), 2012-11-29 22:16:20]

https://codereview.chromium.org/11416251/diff/10025/ui/views/test/views_test_...
File ui/views/test/views_test_base.cc (right):

https://codereview.chromium.org/11416251/diff/10025/ui/views/test/views_test_...
ui/views/test/views_test_base.cc:64: Widget::InitParams::Type type) {
rather than adding this method, can you instead have this class tell the
TestViewsDelegate the context and have it implement CreateNativeWidget()?

[Elliot Glaysher, 2012-11-29 22:46:28]

https://codereview.chromium.org/11416251/diff/10025/ui/views/test/views_test_...
File ui/views/test/views_test_base.cc (right):

https://codereview.chromium.org/11416251/diff/10025/ui/views/test/views_test_...
ui/views/test/views_test_base.cc:64: Widget::InitParams::Type type) {
On 2012/11/29 22:16:20, Ben Goodger (Google) wrote:
> rather than adding this method, can you instead have this class tell the
> TestViewsDelegate the context and have it implement CreateNativeWidget()?

That's moving where the global state is. I'd like it gone.

So currently, scottmg made it so that a context window is passed around as part
of the InitParams and I think this is the right design choice: the widget
creator is who has the information about where the widget should be created. I
eventually want to add a CHECK to NativeWidgetAura::InitParams to ensure that
either a parent, parent_widget or context has been given because we need some
handle to a aura::RootWindow.

Modifying the TestViewsDelegate, so that it is created on each test and creates
a special NativeWidgetAura, doesn't feel correct because it would be another
divergence from what's being tested since additional data usually isn't passed
in to NativeWidget subclasses at construction time. (I suppose we could move the
context to the NativeWidgetDelegate, but that doesn't seem strictly better.)

We could modify the ViewsDelegate::CreateNativeWidget() interface so that it
also took a context parameter, but that feels more like threading aura specific
context through cross platform code than the way I did it, where everything aura
specific has been centralized here.

[Elliot Glaysher, 2012-11-30 23:48:19]

On 2012/11/29 22:46:28, Elliot Glaysher wrote:
>
https://codereview.chromium.org/11416251/diff/10025/ui/views/test/views_test_...
> File ui/views/test/views_test_base.cc (right):
> 
>
https://codereview.chromium.org/11416251/diff/10025/ui/views/test/views_test_...
> ui/views/test/views_test_base.cc:64: Widget::InitParams::Type type) {
> On 2012/11/29 22:16:20, Ben Goodger (Google) wrote:
> > rather than adding this method, can you instead have this class tell the
> > TestViewsDelegate the context and have it implement CreateNativeWidget()?
> 
> That's moving where the global state is. I'd like it gone.
> 
> So currently, scottmg made it so that a context window is passed around as
part
> of the InitParams and I think this is the right design choice: the widget
> creator is who has the information about where the widget should be created. I
> eventually want to add a CHECK to NativeWidgetAura::InitParams to ensure that
> either a parent, parent_widget or context has been given because we need some
> handle to a aura::RootWindow.
> 
> Modifying the TestViewsDelegate, so that it is created on each test and
creates
> a special NativeWidgetAura, doesn't feel correct because it would be another
> divergence from what's being tested since additional data usually isn't passed
> in to NativeWidget subclasses at construction time. (I suppose we could move
the
> context to the NativeWidgetDelegate, but that doesn't seem strictly better.)
> 
> We could modify the ViewsDelegate::CreateNativeWidget() interface so that it
> also took a context parameter, but that feels more like threading aura
specific
> context through cross platform code than the way I did it, where everything
aura
> specific has been centralized here.

(CR didn't pick up on the few emails from this morning. Odd.)

After playing with trying to unify parent, parent_widget, and context, I've
found that we can't because there's a difference between saying:

params.parent = <RootWindow instance>

and

params.context = <RootWindow instance>

And the difference is that we actually have to differentiate between attempts to
literally parent a NativeWidgetAura's window against a RootWindow and asking the
stacking client to place the window somewhere. This *used* to be handled by
setting parent to NULL, since setting the parent to a RootWindow set the parent
to that RootWindow.

Here are two cases where this matters:

- ash/tooltips/tooltip_controller.cc currently works by manually parenting to
the RootWindow. Using this as a 
- browser_frame.cc also wants to root itself to the root window and if I don't,
there's some weird z-ordering issue when I click on the browser frame in a
chromeos build.

There are probably others. I have no idea why the status bar in ash is busted
when I merge the concept of parent and context, but it does.

However, as far as I can tell, parent and parent_widget are still entirely
redundant and can safely be merged and I'll spin up a patch for that.

[Elliot Glaysher, 2012-12-04 08:21:36]

On Fri, Nov 30, 2012 at 4:11 PM, Ben Goodger (Google) <ben@chromium.org>wrote:

> On Fri, Nov 30, 2012 at 3:48 PM, <erg@chromium.org> wrote:
>
>> - ash/tooltips/tooltip_**controller.cc currently works by manually
>> parenting to
>>  the RootWindow. Using this as a
>>
>
> First of all the code in CreateTooltip looks very fishy. It might work in
> non-Ash Linux desktop-aura, but will certainly produce undesirable outcomes
> in Ash-in-Metro, which is also !OS_CHROMEOS.
>
> Secondly, looking at TooltipController::Tooltip, it creates a widget
> without any real context, but it doesn't need to have no context. It's
> called from TooltipController's TooltipClient impl which certainly has
> context, like the aura::Window spawning the tooltip request or the window
> on the mouse/etc event that spawned the tooltip. That's plenty good
> context. You just have to fix this code a little.
>

I entirely agree with you that the ash code is totally fishy.

But this doesn't address the problem: that while there exists good context,
you're asking me to collapse the concept of context and parent, and this is
a great example of why that distinction is important.

Let's examine all possible cases here, with the assumption that child =
false:

- .parent and .context are, both set to NULL: Right now, this has two
effects: the widget won't be a transient child of any parent and will be
placed into a hierarchy by the StackingClient (and given that ash's
StackingController detects and special cases WINDOW_TYPE_TOOLTIP, it's
probably very important to ash that the tooltip is placed in the correct
container.).

- .parent is NULL and .context is set to a specific RootWindow: This case
is exactly as above, except that we no longer rely on having a global
StackingClient since we have context.

- .parent is a RootWindow (and .context doesn't matter): Since
parent->type() != WINDOW_TYPE_UNKNOWN, the window is parented directly to
the RootWindow, which breaks stuff since ash assumes its StackingController
places things in the right container.

There's a semantic difference between being a parent and being context.

-- Elliot

[Elliot Glaysher, 2012-12-04 18:42:33]

On Mon, Dec 3, 2012 at 4:01 PM, Ben Goodger (Google) <ben@chromium.org>wrote:

> So, upon further noodling I agree you need context in the case where
> parent is NULL, but I think that's probably the only case. I don't
> understand the last example where the parent specified is a RootWindow that
> you don't want to parent to.
>

It was an example of why we can't get rid of context entirely.


> I think it'd be worth having a DCHECK_NE(params.GetParent(),
> params.context); - does that sound reasonable?
>

That sounds reasonable after all changes are made, and maybe reasonable
now. I've put that DCHECK_NE in and am running a set of tryjobs, but I
expect that it will break somewhere. I bet there's some case where we
aren't yet passing .parent AND .context so it'll fail on NULL == NULL.

-- Elliot

[Elliot Glaysher, 2012-12-04 19:04:48]

And the DCHECK_NE did fail. Commented it out with a TODO. Will uncomment it
after the third or fourth patch in this series.

[Ben Goodger (Google), 2012-12-04 22:23:19]

lgtm

[commit-bot: I haz the power, 2012-12-04 22:43:41]

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

[commit-bot: I haz the power, 2012-12-05 01:21:21]

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

[commit-bot: I haz the power, 2012-12-05 07:22:24]

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

[commit-bot: I haz the power, 2012-12-05 09:00:48]

Change committed as 171196