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());
 
+    // Create the Widget. This ensures the following calculation correctly

[msw, 2015/07/10 18:40:07]

Hmm, where does the view get added to the widget?

[tapted, 2015/07/13 07:41:21]

Yeah, this test harness is a bit crafty :). ViewEventTestBase is a
views::WidgetDelegate. super::SetUp() then does
Widget::CreateWindowWithContext(), allowing Widget::Init() to get a custom
ContentsView from the test harness at initialization-time.

So during Init(), BookmarkBarViewEventTestBase::CreateContentsView() returns
bb_view_, which is added as the RootView()'s descendant under its ContentsView.

This also means bb_view_ needs to be initialized before a call to 
ViewEventTestBase::SetUp();

Added a comment.

+    // applies borders which may be derived from the Widget's NativeTheme.
+    ViewEventTestBase::SetUp();

[msw, 2015/07/10 18:40:07]

nit: maybe call this at the top of the function?

[tapted, 2015/07/13 07:41:21]

(see above)

+
     // 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());
-
-    ViewEventTestBase::SetUp();
+    window_->SetSize(GetPreferredSize());

[msw, 2015/07/10 18:40:07]

why is this needed now?

[tapted, 2015/07/13 07:41:22]

(tl;dr "moved" this to the GetPreferredSize() test harness override)

I added this line because the "preferred size" reported by the test harness is
cached during SetUp(). So the calculation above that changed it had to be
applied explicitly.

(note this line actually wasn't quite right - it worked on DesktopLinux, but on
other platforms there were test failures because the mouse event to open the
menu would "miss" the window because Widget::SetSize() sets the frame size, but
GetPreferredSize() is for the client view, so the window was not tall enough.

Patchset 5 just fixes this line to include the frame view and has green runs)


But it had me asking: "does this mean the Widget might layout improperly during
Init". So I moved the max-width calculation to GetPreferredSize() to check (and
make this line obsolete). Again, the test harness is a bit tricky to follow...

Widget::Init() *does* call SetInitialBounds after adding the ContentView to the
hierarchy and calling OnNativeThemeChanged, etc. However, the test harness was
caching the preferred size too early. To fix, we can update ViewEventTestBase to
delegate the preferred size rather than cache it (Done in Patchset 6+)

   }
 
   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();
 
@@ skipping 1738 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)