Revert "Implement |GetChildrenInZOrder| in browser and top container views"

This reverts commit e3d54350.

Reason for revert: Broke detached bookmark bar

Original change's description:
> Implement |GetChildrenInZOrder| in browser and top container views
> 
> Currently, we ensure that the bookmark bar view is always the first
> child view of its parent to ensure that toolbar inkdrops are drawn on
> top of it. Unfortunately, this breaks keyboard traversal order.
> 
> This change implements |GetChildrenInZOrder| on the two views that
> host the bookmark bar: browser view and top container view. This
> ensures that the bookmark bar is painted first, removing the need
> to manipulate child view order.
> 
> Bug: 712347
> Change-Id: If8f2aabe816a86b7ec29ff41779783c0f05a6393
> Reviewed-on: https://chromium-review.googlesource.com/1052854
> Commit-Queue: Leonard Grey <lgrey@chromium.org>
> Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#557537}

TBR=ellyjones@chromium.org,lgrey@chromium.org

Change-Id: I43286f3f1a771a8835f72d28c2333f67c9ef6b7b
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 712347
Reviewed-on: https://chromium-review.googlesource.com/1054594Reviewed-by: Leonard Grey <lgrey@chromium.org>
Commit-Queue: Leonard Grey <lgrey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557857}

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

@@ -966,29 +966,6 @@ void BrowserView::SetToolbarButtonProvider(ToolbarButtonProvider* provider) {
   toolbar_button_provider_ = provider;
 }
 
-const views::View::Views BrowserView::DesiredPaintOrderForViews(
-    const views::View::Views views) {
-  // 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 toolbar
-  // 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.
-  auto it = std::find(views.cbegin(), views.cend(), bookmark_bar_view_.get());
-  if (it == views.cend())
-    return views;
-
-  views::View::Views result = {*it};
-  for (auto* view : views) {
-    if (view != *it)
-      result.push_back(view);
-  }
-  return result;
-}
-
-views::View::Views BrowserView::GetChildrenInZOrder() {
-  return DesiredPaintOrderForViews(views::View::GetChildrenInZOrder());
-}
-
 LocationBar* BrowserView::GetLocationBar() const {
   return GetLocationBarView();
 }
@@ -2310,10 +2287,7 @@ bool BrowserView::MaybeShowBookmarkBar(WebContents* contents) {
       new_parent = top_container_;
   }
   if (new_parent != bookmark_bar_view_->parent()) {
-    if (new_parent == nullptr)
-      bookmark_bar_view_->parent()->RemoveChildView(bookmark_bar_view_.get());
-    else
-      new_parent->AddChildView(bookmark_bar_view_.get());
+    SetBookmarkBarParent(new_parent);
     needs_layout = true;
   }
 
@@ -2325,6 +2299,38 @@ bool BrowserView::MaybeShowBookmarkBar(WebContents* contents) {
   return needs_layout;
 }
 
+void BrowserView::SetBookmarkBarParent(views::View* new_parent) {
+  // 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 toolbar
+  // 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.
+  // TODO(bruthig): Consider a more general mechanism for manipulating the
+  // z-order of the ink drops.
+
+  if (new_parent == this) {
+    // BookmarkBarView is detached.
+    const int top_container_index = GetIndexOf(top_container_);
+    DCHECK_GE(top_container_index, 0);
+    // |top_container_| contains the toolbar, so putting the bookmark bar ahead
+    // of it will ensure it's drawn before the toolbar.
+    AddChildViewAt(bookmark_bar_view_.get(), top_container_index);
+  } else if (new_parent == top_container_) {
+    // BookmarkBarView is attached.
+
+    // The toolbar is a child of |top_container_|, so making the bookmark bar
+    // the first child ensures it's drawn before the toolbar.
+    new_parent->AddChildViewAt(bookmark_bar_view_.get(), 0);
+  } else {
+    DCHECK(!new_parent);
+    // 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) {
   // TODO(beng): Remove this function once the interface between
   //             InfoBarContainer, DownloadShelfView and WebContents and this

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

@@ -266,13 +266,6 @@ class BrowserView : public BrowserWindow,
     return toolbar_button_provider_;
   }
 
-  // Returns |views| reordered in the order in which they should be painted,
-  // suitable for returning in |GetChildrenInZOrder|. This is a public method
-  // so that |TopContainerView| can call into it, as it has no knowledge of
-  // what its subviews are.
-  const views::View::Views DesiredPaintOrderForViews(
-      const views::View::Views views);
-
   // Overridden from BrowserWindow:
   void Show() override;
   void ShowInactive() override;
@@ -458,7 +451,6 @@ class BrowserView : public BrowserWindow,
 
   // Overridden from views::View:
   const char* GetClassName() const override;
-  View::Views GetChildrenInZOrder() override;
   void Layout() override;
   void OnGestureEvent(ui::GestureEvent* event) override;
   void ViewHierarchyChanged(
@@ -546,6 +538,11 @@ class BrowserView : public BrowserWindow,
   // |contents| can be null.
   bool MaybeShowBookmarkBar(content::WebContents* contents);
 
+  // Moves the bookmark bar view to the specified parent, which may be null,
+  // |this|, or |top_container_|. Ensures that |top_container_| stays in front
+  // of |bookmark_bar_view_|.
+  void SetBookmarkBarParent(views::View* new_parent);
+
   // Prepare to show an Info Bar for the specified WebContents. Returns
   // true if there is an Info Bar to show and one is supported for this Browser
   // type, and there should be a subsequent re-layout to show it.

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

@@ -96,6 +96,15 @@ TEST_F(BrowserViewTest, BrowserViewLayout) {
   EXPECT_EQ(top_container, browser_view()->GetBookmarkBarView()->parent());
   EXPECT_EQ(browser_view(), browser_view()->infobar_container()->parent());
 
+  // Find bar host is at the front of the view hierarchy, followed by the
+  // infobar container and then top container.
+  EXPECT_EQ(browser_view()->child_count() - 1,
+            browser_view()->GetIndexOf(browser_view()->find_bar_host_view()));
+  EXPECT_EQ(browser_view()->child_count() - 2,
+            browser_view()->GetIndexOf(browser_view()->infobar_container()));
+  EXPECT_EQ(browser_view()->child_count() - 3,
+            browser_view()->GetIndexOf(top_container));
+
   // Verify basic layout.
   EXPECT_EQ(0, top_container->x());
   EXPECT_EQ(0, top_container->y());
@@ -136,6 +145,15 @@ TEST_F(BrowserViewTest, BrowserViewLayout) {
   EXPECT_TRUE(bookmark_bar->IsDetached());
   EXPECT_EQ(browser_view(), bookmark_bar->parent());
 
+  // Find bar host is still at the front of the view hierarchy, followed by the
+  // infobar container and then top container.
+  EXPECT_EQ(browser_view()->child_count() - 1,
+            browser_view()->GetIndexOf(browser_view()->find_bar_host_view()));
+  EXPECT_EQ(browser_view()->child_count() - 2,
+            browser_view()->GetIndexOf(browser_view()->infobar_container()));
+  EXPECT_EQ(browser_view()->child_count() - 3,
+            browser_view()->GetIndexOf(top_container));
+
   // Bookmark bar layout on NTP.
   EXPECT_EQ(0, bookmark_bar->x());
   EXPECT_EQ(tabstrip->bounds().bottom() + toolbar->height(), bookmark_bar->y());
@@ -157,31 +175,6 @@ TEST_F(BrowserViewTest, BrowserViewLayout) {
   BookmarkBarView::DisableAnimationsForTesting(false);
 }
 
-TEST_F(BrowserViewTest, BookmarkBarPaintsFirst) {
-  // The bookmark bar should be first in the z-order of its parent so that
-  // any ink drops from the toolbar can overlap it.
-
-  BookmarkBarView::DisableAnimationsForTesting(true);
-
-  Browser* browser = browser_view()->browser();
-
-  // Attached bookmark bar.
-  AddTab(browser, GURL("about:blank"));
-  chrome::ExecuteCommand(browser, IDC_SHOW_BOOKMARK_BAR);
-  EXPECT_EQ(browser_view()->top_container()->GetChildrenInZOrder()[0],
-            browser_view()->GetBookmarkBarView());
-
-  // If "Always Show Bookmark Bar" is off, the NTP displays the detached
-  // bookmark bar.
-  chrome::ExecuteCommand(browser, IDC_SHOW_BOOKMARK_BAR);
-  NavigateAndCommitActiveTabWithTitle(browser, GURL(chrome::kChromeUINewTabURL),
-                                      base::string16());
-  EXPECT_EQ(browser_view()->GetChildrenInZOrder()[0],
-            browser_view()->GetBookmarkBarView());
-
-  BookmarkBarView::DisableAnimationsForTesting(false);
-}
-
 // Test that repeated accelerators are processed or ignored depending on the
 // commands that they refer to. The behavior for different commands is dictated
 // by IsCommandRepeatable() in chrome/browser/ui/views/accelerator_table.h.

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

@@ -45,8 +45,3 @@ void TopContainerView::PaintChildren(const views::PaintInfo& paint_info) {
 void TopContainerView::ChildPreferredSizeChanged(views::View* child) {
   PreferredSizeChanged();
 }
-
-views::View::Views TopContainerView::GetChildrenInZOrder() {
-  return browser_view_->DesiredPaintOrderForViews(
-      views::View::GetChildrenInZOrder());
-}

chrome/browser/ui/views/frame/top_container_view.h

@@ -23,7 +23,6 @@ class TopContainerView : public views::View {
   const char* GetClassName() const override;
   void PaintChildren(const views::PaintInfo& paint_info) override;
   void ChildPreferredSizeChanged(views::View* child) override;
-  views::View::Views GetChildrenInZOrder() override;
 
  private:
   // The parent of this view. Not owned.