Re-order the BookmarkBarView to be behind the ToolbarView

chrome/browser/ui/views/frame/browser_view.cc

Index: chrome/browser/ui/views/frame/browser_view.cc
diff --git a/chrome/browser/ui/views/frame/browser_view.cc b/chrome/browser/ui/views/frame/browser_view.cc
index f0e4383ae15bd76c454c0d9a496ffc9c697c0f0d..0bb9feb4d09107d06916a63aca843feb646da3bc 100644
--- a/chrome/browser/ui/views/frame/browser_view.cc
+++ b/chrome/browser/ui/views/frame/browser_view.cc
@@ -2214,16 +2214,33 @@ bool BrowserView::MaybeShowBookmarkBar(WebContents* contents) {
 }
 
 void BrowserView::SetBookmarkBarParent(views::View* new_parent) {
+  // NOTE: The ink drops hosted by the |toolbar_| exceed the bounds of the
+  // |toolbar_| so in order to have them visible above the |bookmark_bar_view_|
+  // we need to explicitly add the |bookmark_bar_view_| as a child view at a
+  // specific index because that is what determines z-order.

[Peter Kasting, 2016/03/31 22:22:57]

Nit: How about this.  TODO is optional, I don't know if you actually want to
make that change someday or not.

Because children are drawn in order, the child order also affects z-order:
earlier children will appear "below" later ones.  This is important for ink
drops, which are drawn with the z-order of the view that parents them.  Ink
drops in the toolbar can spread beyond the toolbar bounds, so if the bookmark
bar is attached, we want it to be below the toolbar so the ink drops draw atop
it.  This doesn't cause a problem for interactions with the bookmark bar, since
it does not host any ink drops that spread beyond its bounds.  If it did, we
would need to change how ink drops are drawn so they use a transparent layer
above all children here.  TODO(bruthig): Use such a layer in all cases,
eliminating the need for any complexity here.

[bruthig, 2016/04/01 16:15:12]

Modified slightly but done.

+
   if (new_parent == this) {
-    // Add it underneath |top_container_| or at the end if top container isn't
-    // found.
+    // BookmarkBarView is detached.
     int top_container_index = GetIndexOf(top_container_);
-    if (top_container_index >= 0)
+    if (top_container_index >= 0) {
+      // |this| parents the |top_container_| and the |top_container_| parents
+      // the |toolbar_| so we have to stack the |bookmark_bar_view_| behind the
+      // |top_container_|.

[Peter Kasting, 2016/03/31 22:22:56]

Nit: How about:

|top_container_| contains the toolbar, so putting the bookmark bar ahead if it
will ensure it's drawn before the toolbar.

[bruthig, 2016/04/01 16:15:12]

Done.

       AddChildViewAt(bookmark_bar_view_.get(), top_container_index);
-    else
+    } else {
+      // This is not an expected case and should only happen if
+      // BrowserView::InitViews() has not been called. Presumably the |toolbar_|
+      // is not visible so the z-order does not matter.

[Peter Kasting, 2016/03/31 22:22:57]

Seems like if this is true we should remove this case and if necessary just
DCHECK_GE(top_container_index, 0);.

[bruthig, 2016/04/01 16:15:12]

Done.

       AddChildView(bookmark_bar_view_.get());
+    }
+  } else if (new_parent == top_container_) {
+    // BookmarkBarView is detached.

[Peter Kasting, 2016/03/31 22:22:57]

Do you mean "attached"?

[bruthig, 2016/04/01 16:15:12]

Done.

+
+    // The |top_container_| parents the |toolbar_| too so we stack the
+    // |bookmark_bar_view_| first so it is behind the |toolbar_|.

[Peter Kasting, 2016/03/31 22:22:57]

Nit: How about:

The toolbar is a child of |top_container_|, so making the bookmark bar the first
child ensures it's drawn before the toolbar.

[bruthig, 2016/04/01 16:15:12]

Done.

+    new_parent->AddChildViewAt(bookmark_bar_view_.get(), 0);
   } else if (new_parent) {
-    // No special stacking is required for other parents.
+    // This is not an expected case so we will default to no special stacking.

[Peter Kasting, 2016/03/31 22:22:57]

Seems like if this is true we should remove this conditional arm and
DCHECK(!new_parent); in the else below.

[bruthig, 2016/04/01 16:15:12]

Done.

     new_parent->AddChildView(bookmark_bar_view_.get());
   } else {
     // Bookmark bar is being detached from all views because it is hidden.