Re-order the BookmarkBarView to be behind the ToolbarView

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

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/frame/browser_view.h"
 
 #include <stdint.h>
 #include <algorithm>
 #include <utility>
 
@@ skipping 2204 matching lines @@
 
 void BrowserView::SetBookmarkBarParent(views::View* new_parent) {
   if (new_parent == this) {
     // Add it underneath |top_container_| or at the end if top container isn't
     // found.
     int top_container_index = GetIndexOf(top_container_);
     if (top_container_index >= 0)
       AddChildViewAt(bookmark_bar_view_.get(), top_container_index);
     else
       AddChildView(bookmark_bar_view_.get());
+  } else if (new_parent == top_container_) {
+    // Stack the |bookmark_bar_view_| behind the |toolbar_| surface so that
+    // |toolbar_| ink drops are visible on top of the |bookmark_bar_view_|.
+    new_parent->AddChildViewAt(bookmark_bar_view_.get(), 0);

[Peter Kasting, 2016/03/30 21:15:28]

This may be better for sky than me; I'm not familiar with this function.

It seems strange that the comment talks about Z-order but the functionality
actually is about adding before or after other children, which impacts things
like taborder and should normally match the visible hierarchy onscreen.  I'm not
familiar with child order affecting Z order.  I would instead have expected
changes to layout or painting to accomplish that.

[bruthig, 2016/03/30 21:27:00]

As far as I can tell a user cannot tab between the ToolbarView and the
BookmarkBarView, only between child Views within them.  I will add sky@.

[Peter Kasting, 2016/03/30 21:28:42]

Is the taborder between the toolbar children and the bookmark bar children
unaffected by this change?

Even if yes, I'm still a bit mystified how this actually achieves a Z-order
adjustment.

[sky, 2016/03/31 01:07:16]

Tab does wrap within the bookmarkbar. You can cycle through panes with F6. First
hit shift-alt-t, and then use f6 to cycle. 

[bruthig, 2016/03/31 21:12:29]

I'm slightly confused here, are there outstanding concerns?

FTR The F6 pane focus order is defined by the list returned from
BrowserView::GetAccessiblePanes() which is NOT dependent on the order of
BrowserView's children Views.

[Peter Kasting, 2016/03/31 21:20:41]

As long as it's not possible to actually tab directly between toolbar and
bookmark bar view elements (i.e. one has to hit a different key like F6), I'm
not concerned.

I'm still not familiar enough with the rest of this function to necessarily sign
off on this change, though.  What is the top container, and why could we
sometimes be added as a sibling to it and sometimes as a child?  I'd like to
have a more coherent comment on all this stuff elaborating the different cases
where there's a bookmark bar and what the child order needs to be in each one,
mentioning how the child order becomes the paint order and thus the Z order.

[bruthig, 2016/03/31 22:04:23]

That is correct.  Tab's will stay within the same "Pane" and only F6 will move
between the Toolbar, BookmarkBarView, InfoBarContainerView, DownloadShelfView,
ContentsWebView and dev tool view "Panes".
I've added some comments as requested however I feel like these could quickly
drift from correctness and would be more than happy to purge some.  Please let
me know what you think is necessary.

sky@, your input here is welcome too.

As for why the |bookmark_bar_| is sometimes added as a sibling and sometimes as
a child, I have no idea.  sky@, do you have any idea?

   } else if (new_parent) {
     // No special stacking is required for other parents.
     new_parent->AddChildView(bookmark_bar_view_.get());
   } else {
     // Bookmark bar is being detached from all views because it is hidden.
     bookmark_bar_view_->parent()->RemoveChildView(bookmark_bar_view_.get());
   }
 }
 
 bool BrowserView::MaybeShowInfoBar(WebContents* contents) {
@@ skipping 410 matching lines @@
 }
 
 extensions::ActiveTabPermissionGranter*
 BrowserView::GetActiveTabPermissionGranter() {
   content::WebContents* web_contents = GetActiveWebContents();
   if (!web_contents)
     return nullptr;
   return extensions::TabHelper::FromWebContents(web_contents)
       ->active_tab_permission_granter();
 }