Reorder the NativeViews attached to a view via kViewHostKey according to the position of the view

Reorder the NativeViews attached to a view via the kViewHostKey according to the position of the view in the view hierarchy.
 
BUG=237650
Test=NativeWidgetAuraTest.*

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

[pkotwicz, 2013-05-13 02:24:01]

Scott, can you please take a look?
In this CL I added a really simple class to associate the find bar aura::Window
with a "host" view.
If you like the approach I will merge this CL with a new version of
https://codereview.chromium.org/14878003/

I decided against using a NativeViewHost because the implementation was getting
needlessly complicated (In particular rtl considerations in switching
DropdownBarHost::GetDialogPosition() to return bounds wrt to the browser view)

[sky, 2013-05-13 15:53:14]

https://codereview.chromium.org/15114002/diff/1/chrome/browser/ui/views/dropd...
File chrome/browser/ui/views/dropdown_bar_host_aura.cc (right):

https://codereview.chromium.org/15114002/diff/1/chrome/browser/ui/views/dropd...
chrome/browser/ui/views/dropdown_bar_host_aura.cc:34: if (!host_view_.get()) {
Could we instead always create a View for this in BrowserView and this code
would set the mapping property?

https://codereview.chromium.org/15114002/diff/1/ui/aura/client/aura_constants.cc
File ui/aura/client/aura_constants.cc (right):

https://codereview.chromium.org/15114002/diff/1/ui/aura/client/aura_constants...
ui/aura/client/aura_constants.cc:9: #include "ui/views/view.h"
This would introduce a cycle between aura->views and views->aura . Can
kHostViewKey be in views since that is the only place that needs it?

[pkotwicz, 2013-05-13 20:20:44]

Scott, can you please take another look?

https://codereview.chromium.org/15114002/diff/1/chrome/browser/ui/views/dropd...
File chrome/browser/ui/views/dropdown_bar_host_aura.cc (right):

https://codereview.chromium.org/15114002/diff/1/chrome/browser/ui/views/dropd...
chrome/browser/ui/views/dropdown_bar_host_aura.cc:34: if (!host_view_.get()) {
I think that creating the view here makes more sense. I think we should hook
this view up as a proper NativeViewHost before we always create in
BrowserView::Init(). (I agree that your proposal is the correct long term vision
though)

Creating this view on an "as-needed" basis is consistent with FindBarController
being created on an "as-needed" basis

[sky, 2013-05-13 22:43:05]

https://codereview.chromium.org/15114002/diff/18002/ui/views/view_constants_a...
File ui/views/view_constants_aura.h (right):

https://codereview.chromium.org/15114002/diff/18002/ui/views/view_constants_a...
ui/views/view_constants_aura.h:14: // A property key to indicate the window's
host view. If specified, the host
This is a bit confusing. In particular you make it sound like the 'host view' is
doing the determination. That isn't quite right. I think you mean something
like:
'The z-order of the window is made to match that of the z-order of the view
relative to other views with layers and/or windows.'

https://codereview.chromium.org/15114002/diff/18002/ui/views/views.gyp
File ui/views/views.gyp (right):

https://codereview.chromium.org/15114002/diff/18002/ui/views/views.gyp#newcod...
ui/views/views.gyp:333: 'view_constants_aura.cc',
Fix the order here.

[sky, 2013-05-13 22:43:11]

I'm ok with the rest of it.

[sky, 2013-05-13 22:54:17]

https://codereview.chromium.org/15114002/diff/18002/chrome/browser/ui/views/d...
File chrome/browser/ui/views/dropdown_bar_host_aura.cc (right):

https://codereview.chromium.org/15114002/diff/18002/chrome/browser/ui/views/d...
chrome/browser/ui/views/dropdown_bar_host_aura.cc:40:
browser_view_->AddChildView(host_view_.get());
One reason I like binding to an existing view owned by BrowserView is that the
z-order is easy to understand. When you start dynamically adding views it gets
harder to reason about. I prefer creating and binding to a long lived view in
Browserview.

[pkotwicz, 2013-05-21 21:27:58]

Scott, can you please take a look?

This is the views portion of the z-order change. I will upload the changes to
chrome/browser/ui/views/ & NativeViewHost once this CL goes through

[sky, 2013-05-21 23:43:55]

https://codereview.chromium.org/15114002/diff/30001/ui/views/widget/native_wi...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/15114002/diff/30001/ui/views/widget/native_wi...
ui/views/widget/native_widget_aura.cc:1069: void
NativeWidgetPrivate::ReorderLayers(gfx::NativeView parent,
I think all this code should be moved into a standalone object. Additionally
this object should implement a WindowObserver so it can automatically update
things when a window is added/removed as well as having a method to explicitly
update order NativeViewHost position in view hieararchy changes.

https://codereview.chromium.org/15114002/diff/30001/ui/views/widget/native_wi...
ui/views/widget/native_widget_aura.cc:1078: std::map<View*, aura::Window*>
hosted_windows;
This method is long, break it up where you can. For example,
GetViewsWithAssociatedWindow or something would be nice for this part of the
code.

https://codereview.chromium.org/15114002/diff/30001/ui/views/widget/native_wi...
ui/views/widget/native_widget_aura.cc:1093: 
Can't you early out if hosted_windows is empty?

https://codereview.chromium.org/15114002/diff/30001/ui/views/widget/native_wi...
ui/views/widget/native_widget_aura.cc:1098: // Temporarily modify the ids of the
views attached to a window to simplify
Yuck! Don't muck with the ids like this.

[pkotwicz, 2013-05-23 02:28:07]

Scott, can you please take another look?

Moved code for reordering layers and windows to NativeWidgetLayerReordererAura.

Fixed ordering (and added a test) for when a view with an attached window is
moved from one parent view with a layer to another.

https://codereview.chromium.org/15114002/diff/30001/ui/views/widget/native_wi...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/15114002/diff/30001/ui/views/widget/native_wi...
ui/views/widget/native_widget_aura.cc:1078: std::map<View*, aura::Window*>
hosted_windows;
On 2013/05/21 23:43:55, sky wrote:
> This method is long, break it up where you can. For example,
> GetViewsWithAssociatedWindow or something would be nice for this part of the
> code.

Done.

https://codereview.chromium.org/15114002/diff/30001/ui/views/widget/native_wi...
ui/views/widget/native_widget_aura.cc:1093: 
We cannot early out here because there may be views with layers (and we need to
reorder those). Furthermore, we cannot simply call View::ReorderChildLayers()
because we need to stack the view layers below the layers for any windows with a
NULL layer delegate.

https://codereview.chromium.org/15114002/diff/30001/ui/views/widget/native_wi...
ui/views/widget/native_widget_aura.cc:1098: // Temporarily modify the ids of the
views attached to a window to simplify
No more modifying of the ids

https://codereview.chromium.org/15114002/diff/35001/ui/views/widget/native_wi...
File ui/views/widget/native_widget_layer_reorderer_aura.cc (right):

https://codereview.chromium.org/15114002/diff/35001/ui/views/widget/native_wi...
ui/views/widget/native_widget_layer_reorderer_aura.cc:49: } else if
(hosts.find(view) != hosts.end()) {
I think that this is ok because |hosts| is small in practice.

[sky, 2013-05-23 15:36:48]

https://codereview.chromium.org/15114002/diff/30001/ui/views/widget/native_wi...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/15114002/diff/30001/ui/views/widget/native_wi...
ui/views/widget/native_widget_aura.cc:1093: 
On 2013/05/23 02:28:07, pkotwicz wrote:
> We cannot early out here because there may be views with layers (and we need
to
> reorder those). Furthermore, we cannot simply call View::ReorderChildLayers()
> because we need to stack the view layers below the layers for any windows with
a
> NULL layer delegate.
> 

Why don't we change ReorderLayers/ReorderChildLayers to iterate backwards and
stack at bottom. This should take care of this case and simplify things, right?

https://codereview.chromium.org/15114002/diff/35001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/15114002/diff/35001/ui/views/view.cc#newcode1442
ui/views/view.cc:1442: if (GetWidget()) {
cache call to GetWidget

https://codereview.chromium.org/15114002/diff/35001/ui/views/view.cc#newcode1447
ui/views/view.cc:1447: if (!v || GetWidget()->HasNativeViewAttachedToViewTree())
HasNativeViewAttachedToViewTree doesn't make sense for windows.

I think a better approach here is to always call GetWidget()->ReorderLayers(v):

void Widget::ReorderLayers(View* parent_with_layer) {
  if (!parent_width_layer && native_widget_->GetLayer())
    root_view_->ReorderChildLayers(native_widget_->GetLayer());
  if (native_widget_->ShouldReorderViewsWithLayers(parent_with_layer))
    // call off to your new function here.
}

GetLayer() is a new method in NativeWidgetPrivate. It replaces
CalculateOffsetToAncestorWithLayer() which doesn't make sense anymore.

ShouldReorderViewsWithLayers is effectively what you have in
HasNativeViewAttachedToViewTree.

I also think this (WIdget::ReorderLayers) should be invoked after the children
are reordered. That way the view layers are all ordered correctly when you
attempt to reorder for the NativeViewHosts.

[pkotwicz, 2013-05-23 17:21:44]

https://codereview.chromium.org/15114002/diff/30001/ui/views/widget/native_wi...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/15114002/diff/30001/ui/views/widget/native_wi...
ui/views/widget/native_widget_aura.cc:1093: 
The change you are suggesting would result in changing the default z-order of
calling RecreateLayer().
The new z-order would put the old layer at the top and the view/window with the
new layer at the bottom.
I don't think that changing the default z-order as a result of RecraeteLayer()
is a big but wanted to pass it by you.

[sky, 2013-05-23 17:33:35]

https://codereview.chromium.org/15114002/diff/30001/ui/views/widget/native_wi...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/15114002/diff/30001/ui/views/widget/native_wi...
ui/views/widget/native_widget_aura.cc:1093: 
On 2013/05/23 17:21:45, pkotwicz wrote:
> The change you are suggesting would result in changing the default z-order of
> calling RecreateLayer().
> The new z-order would put the old layer at the top and the view/window with
the
> new layer at the bottom.
> I don't think that changing the default z-order as a result of RecraeteLayer()
> is a big but wanted to pass it by you.
> 

I think that better matches expectations, right? RecreateLayer is typically used
for animations where you want the old layer to stay at the top. You'll have to
verify that though.

[pkotwicz, 2013-05-24 02:46:54]

sky@ can you take another look? I believe I implemented all of your suggestions.

I renamed Widget::ReorderLayers() to
Widget::ReorderLayersForAttachedNativeViews() because I think that name is more
representative.
In particular, Widget::ReorderLayersForAttachedNativeViews() will not do any
reordering if there are no views with attached NativeViews.

[pkotwicz, 2013-05-24 02:49:33]

I do not pass in a view to Widget::ReorderLayersForAttachedNativeViews() or use
Widget::ShouldReorderViewsWithLayers() because it does not gain us anything in
terms of performance and I am unsure if it makes the code any clearer either.

[sky, 2013-05-24 15:57:26]

https://codereview.chromium.org/15114002/diff/47001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/15114002/diff/47001/ui/views/view.cc#newcode1476
ui/views/view.cc:1476: if (layer() && layer() != parent_layer) {
It's subtle that we iterate backward. And subtle code is worth a comment.

https://codereview.chromium.org/15114002/diff/47001/ui/views/view_constants_a...
File ui/views/view_constants_aura.h (right):

https://codereview.chromium.org/15114002/diff/47001/ui/views/view_constants_a...
ui/views/view_constants_aura.h:21: VIEWS_EXPORT extern const
aura::WindowProperty<View*>* const kHostViewKey;
This is confusing and makes it sound as though the window will always be on top
of views with layers, which isn't right. How about:

A property key to indicate the view the window is associated with. If specified
the z-order of the view, relative to other views, dictates the z-order of the
window and its associated layer.

https://codereview.chromium.org/15114002/diff/47001/ui/views/widget/native_wi...
File ui/views/widget/native_widget_layer_reorderer_aura.cc (right):

https://codereview.chromium.org/15114002/diff/47001/ui/views/widget/native_wi...
ui/views/widget/native_widget_layer_reorderer_aura.cc:111:
window_->layer()->StackAtBottom(layer);
Don't we only care about items in hosted_windows here?

https://codereview.chromium.org/15114002/diff/47001/ui/views/widget/native_wi...
ui/views/widget/native_widget_layer_reorderer_aura.cc:116:
window->RemoveObserver(this);
It's subtle and easy to get wrong that you're observing both window_ and all its
children. Please document this some where. In fact consider forking off a
separate class just to handle the property change case, which should make the
code less error prone.

https://codereview.chromium.org/15114002/diff/47001/ui/views/widget/native_wi...
ui/views/widget/native_widget_layer_reorderer_aura.cc:133: if (window->parent()
== window_)
Why do you care about this case?

https://codereview.chromium.org/15114002/diff/47001/ui/views/widget/native_wi...
File ui/views/widget/native_widget_layer_reorderer_aura.h (right):

https://codereview.chromium.org/15114002/diff/47001/ui/views/widget/native_wi...
ui/views/widget/native_widget_layer_reorderer_aura.h:25: class
NativeWidgetLayerReordererAura : public aura::WindowObserver {
This is specific to aura::Window, so maybe the name should reflect that:
WindowReorderer, WindowPositionManager, something.

[pkotwicz, 2013-05-26 04:12:15]

Scott, can you please take another look?

https://codereview.chromium.org/15114002/diff/47001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/15114002/diff/47001/ui/views/view.cc#newcode1476
ui/views/view.cc:1476: if (layer() && layer() != parent_layer) {
On 2013/05/24 15:57:26, sky wrote:
> It's subtle that we iterate backward. And subtle code is worth a comment.

Done.

https://codereview.chromium.org/15114002/diff/47001/ui/views/view_constants_a...
File ui/views/view_constants_aura.h (right):

https://codereview.chromium.org/15114002/diff/47001/ui/views/view_constants_a...
ui/views/view_constants_aura.h:21: VIEWS_EXPORT extern const
aura::WindowProperty<View*>* const kHostViewKey;
On 2013/05/24 15:57:26, sky wrote:
> This is confusing and makes it sound as though the window will always be on
top
> of views with layers, which isn't right. How about:
> 
> A property key to indicate the view the window is associated with. If
specified
> the z-order of the view, relative to other views, dictates the z-order of the
> window and its associated layer.

Done.

https://codereview.chromium.org/15114002/diff/47001/ui/views/widget/native_wi...
File ui/views/widget/native_widget_layer_reorderer_aura.cc (right):

https://codereview.chromium.org/15114002/diff/47001/ui/views/widget/native_wi...
ui/views/widget/native_widget_layer_reorderer_aura.cc:111:
window_->layer()->StackAtBottom(layer);
IMHO, it's simpler (and safer) to reorder both layers owned by views and layers
associated to a view. I added a comment documenting this.

https://codereview.chromium.org/15114002/diff/47001/ui/views/widget/native_wi...
ui/views/widget/native_widget_layer_reorderer_aura.cc:116:
window->RemoveObserver(this);
Added a separate class

https://codereview.chromium.org/15114002/diff/47001/ui/views/widget/native_wi...
ui/views/widget/native_widget_layer_reorderer_aura.cc:133: if (window->parent()
== window_)
This is no longer relevant because I separated the window observer for |window_|
and the window observer for |window_|'s children into a seperate class

https://codereview.chromium.org/15114002/diff/47001/ui/views/widget/native_wi...
File ui/views/widget/native_widget_layer_reorderer_aura.h (right):

https://codereview.chromium.org/15114002/diff/47001/ui/views/widget/native_wi...
ui/views/widget/native_widget_layer_reorderer_aura.h:25: class
NativeWidgetLayerReordererAura : public aura::WindowObserver {
Renamed to NativeWidgetWindowReordererAura

[sky, 2013-05-28 14:03:50]

https://codereview.chromium.org/15114002/diff/58024/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/15114002/diff/58024/ui/aura/window.cc#newcode166
ui/aura/window.cc:166: // Install new layer as a sibling of the old layer,
stacked below it.
Document why it's important to stack below rather than above.

https://codereview.chromium.org/15114002/diff/58024/ui/views/widget/native_wi...
File ui/views/widget/native_widget_window_reorderer_aura.h (right):

https://codereview.chromium.org/15114002/diff/58024/ui/views/widget/native_wi...
ui/views/widget/native_widget_window_reorderer_aura.h:25: class
NativeWidgetWindowReordererAura : public aura::WindowObserver {
Again, I think this name is overly verbose. WindowReorderer is all you really
need here.

[pkotwicz, 2013-05-28 15:14:59]

Scott, can you please take another look?

https://codereview.chromium.org/15114002/diff/58024/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/15114002/diff/58024/ui/aura/window.cc#newcode166
ui/aura/window.cc:166: // Install new layer as a sibling of the old layer,
stacked below it.
This is actually a bad diff, this change is not part of this CL.

https://codereview.chromium.org/15114002/diff/58024/ui/views/widget/native_wi...
File ui/views/widget/native_widget_window_reorderer_aura.h (right):

https://codereview.chromium.org/15114002/diff/58024/ui/views/widget/native_wi...
ui/views/widget/native_widget_window_reorderer_aura.h:25: class
NativeWidgetWindowReordererAura : public aura::WindowObserver {
Renamed!

[sky, 2013-05-28 15:41:49]

LGTM

[commit-bot: I haz the power, 2013-05-29 16:18:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/15114002/84001

[commit-bot: I haz the power, 2013-05-29 16:25:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/15114002/90002

[commit-bot: I haz the power, 2013-05-29 22:02:18]

Change committed as 202987