Just set borders once when creating a views::LabelButton

chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc

 // Copyright (c) 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 "base/bind.h"
 #include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/location.h"
 #include "base/message_loop/message_loop.h"
 #include "base/prefs/pref_service.h"
@@ skipping 269 matching lines @@
     local_state_.reset(new ScopedTestingLocalState(
         TestingBrowserProcess::GetGlobal()));
     model_->ClearStore();
 
     bb_view_.reset(new BookmarkBarView(browser_.get(), NULL));
     bb_view_->set_owned_by_client();
     bb_view_->SetPageNavigator(&navigator_);
 
     AddTestData(CreateBigMenu());
 
-    // Calculate the preferred size so that one button doesn't fit, which
-    // triggers the overflow button to appear. We have to do this incrementally
-    // as there isn't a good way to determine the point at which the overflow
-    // button is shown.
-    //
-    // This code looks a bit hacky, but I've written it so that it shouldn't
-    // be dependant upon any of the layout code in BookmarkBarView. Instead
-    // we brute force search for a size that triggers the overflow button.
-    bb_view_pref_ = bb_view_->GetPreferredSize();
-    bb_view_pref_.set_width(1000);
-    do {
-      bb_view_pref_.set_width(bb_view_pref_.width() - 25);
-      bb_view_->SetBounds(0, 0, bb_view_pref_.width(), bb_view_pref_.height());
-      bb_view_->Layout();
-    } while (GetBookmarkButton(6)->visible());
+    // Create the Widget. Note the initial size is given by GetPreferredSize()
+    // during initialization. This occurs after the WidgetDelegate provides
+    // |bb_view_| as the contents view and adds it to the hierarchy.
+    ViewEventTestBase::SetUp();
 
-    ViewEventTestBase::SetUp();
+    // Verify the layout triggered by the initial size preserves the overflow
+    // state calculated in GetPreferredSize().
+    EXPECT_TRUE(GetBookmarkButton(5)->visible());
+    EXPECT_FALSE(GetBookmarkButton(6)->visible());
   }
 
   void TearDown() override {
     // Destroy everything, then run the message loop to ensure we delete all
     // Tasks and fully shut down.
     browser_->tab_strip_model()->CloseAllTabs();
     bb_view_.reset();
     browser_.reset();
     profile_.reset();
 
     // Run the message loop to ensure we delete allTasks and fully shut down.
     base::MessageLoopForUI* loop = base::MessageLoopForUI::current();
     base::MessageLoopForUI::ScopedNestableTaskAllower allow_nested(loop);
     base::RunLoop run_loop;
     loop->task_runner()->PostTask(FROM_HERE, run_loop.QuitClosure());
     run_loop.Run();
 
     ViewEventTestBase::TearDown();
     BookmarkBarView::DisableAnimationsForTesting(false);
     constrained_window::SetConstrainedWindowViewsClient(nullptr);
 
     browser_content_client_.reset();
     content_client_.reset();
     content::SetContentClient(NULL);
   }
 
  protected:
   views::View* CreateContentsView() override { return bb_view_.get(); }
 
-  gfx::Size GetPreferredSize() const override { return bb_view_pref_; }
+  gfx::Size GetPreferredSize() const override {
+    // Calculate the preferred size so that one button doesn't fit, which
+    // triggers the overflow button to appear. We have to do this incrementally
+    // as there isn't a good way to determine the point at which the overflow
+    // button is shown.
+    //
+    // This code looks a bit hacky, but it is written so that it shouldn't

[msw, 2015/07/13 17:54:03]

nit: This code *does* look hacky; it's fine to leave as-is for now, since it's
been working (right?), but why not layout at full size, then get the bounds of
bb_view_->GetBookmarkButton(6), and return a size slightly inset from its right
edge? (or at least remove the size.set_width(1000), since
bb_view_->GetPreferredSize() should give a sufficient width to show the button)

[tapted, 2015/07/14 05:36:12]

I actually tried this while investigating one of the test failures. The problem
is that there are other things to the right of the last button -- the "Other
Bookmarks" button, and potentially other things (ManagedBookmarksButton,
SupervisedBookmarksButton) if a test were to enable those. So the current
approach is more robust to those things, and avoids coupling the test to a
specific layout. And, although I did suspect if for being fragile, it turned out
to be something else. So, it does seem to work fine.

There are, however, some tests disabled, and the whole file is skipped for
official CrOS builds :/ http://crbug.com/431450 http://crbug.com/431427 . I
wouldn't rule this logic out as a culprit, but most of the tests are still
enabled elsewhere, and the trybots are giving pretty good coverage (e.g. lots of
red when I screwed it up :p)

+    // depend on any of the layout code in BookmarkBarView, or extra buttons
+    // added to the right of the bookmarks. Instead, brute force search for a
+    // size that triggers the overflow button.
+    gfx::Size size = bb_view_->GetPreferredSize();
+    size.set_width(1000);
+    do {
+      size.set_width(size.width() - 25);
+      bb_view_->SetBounds(0, 0, size.width(), size.height());
+      bb_view_->Layout();
+    } while (bb_view_->GetBookmarkButton(6)->visible());
+    return size;
+  }
 
   views::LabelButton* GetBookmarkButton(int view_index) {
     return bb_view_->GetBookmarkButton(view_index);
   }
 
   // See comment above class description for what this does.
   virtual bool CreateBigMenu() { return false; }
 
   BookmarkModel* model_;
   scoped_ptr<BookmarkBarView> bb_view_;
@@ skipping 25 matching lines @@
     const BookmarkNode* of = model_->AddFolder(model_->other_node(), 1,
                                                ASCIIToUTF16("OF"));
     model_->AddURL(of, 0, ASCIIToUTF16("ofa"), GURL(test_base + "ofa"));
     model_->AddURL(of, 1, ASCIIToUTF16("ofb"), GURL(test_base + "ofb"));
     const BookmarkNode* of2 = model_->AddFolder(model_->other_node(), 2,
                                                 ASCIIToUTF16("OF2"));
     model_->AddURL(of2, 0, ASCIIToUTF16("of2a"), GURL(test_base + "of2a"));
     model_->AddURL(of2, 1, ASCIIToUTF16("of2b"), GURL(test_base + "of2b"));
   }
 
-  gfx::Size bb_view_pref_;
   scoped_ptr<ChromeContentClient> content_client_;
   scoped_ptr<chrome::ChromeContentBrowserClient> browser_content_client_;
   scoped_ptr<TestingProfile> profile_;
   scoped_ptr<Browser> browser_;
   scoped_ptr<ScopedTestingLocalState> local_state_;
 };
 
 // Clicks on first menu, makes sure button is depressed. Moves mouse to first
 // child, clicks it and makes sure a navigation occurs.
 class BookmarkBarViewTest1 : public BookmarkBarViewEventTestBase {
@@ skipping 1662 matching lines @@
 };
 
 #if defined(OS_WIN)
 // This test times out on Windows. TODO(pkotwicz): Find out why.
 #define MAYBE_CloseSourceBrowserDuringDrag DISABLED_CloseSourceBrowserDuringDrag
 #else
 #define MAYBE_CloseSourceBrowserDuringDrag CloseSourceBrowserDuringDrag
 #endif
 
 VIEW_TEST(BookmarkBarViewTest22, MAYBE_CloseSourceBrowserDuringDrag)