Just set borders once when creating a views::LabelButton

Just set borders once when creating a views::LabelButton

When creating a LabelButton, OnNativeThemeChanged(ui::NativeTheme*) is
currently called 2 or 3 times:

- via a SetStyle(STYLE_TEXTBUTTON) call in the LabelButton constructor
(passes null)
- for STYLE_BUTTON buttons, via a second SetStyle(STYLE_BUTTON) call
shortly after construction (passes null since the button is not yet in a
Widget)
- by an ancestor View when added to a Widget's view hierarchy for the
first time.

Each call creates and sets the button border painters, which are not
simple. The first call also calls virtual functions from a constructor,
which may not even provide a correct border for a subclass. Also,
subclasses that make their own SetBorder() call do not even want borders
provided by LabelButton::UpdateThemedBorder(), but they currently get
them created anyway (which they then replace).

Since the first 1-2 borders that are created will always be replaced
when the view is added to a Widget, don't create borders then.

Instead, SetStyle() now just sets things up for an anticipated call to
OnNativeThemeChanged() to finish creating the borders.

After this, LabelButton no longer calls virtual methods from its
constructor.

BUG=506729, 508410
Committed: https://crrev.com/c6db86a78cba9a415fb75b0a595ad84d9651056c
Cr-Commit-Position: refs/heads/master@{#338796}

[tapted, 2015-07-10 03:23:25]

tapted@chromium.org changed reviewers:
+ sadrul@chromium.org

[tapted, 2015-07-10 03:26:16]

tapted@chromium.org changed reviewers:
+ msw@chromium.org

[tapted, 2015-07-10 03:28:51]

Hi sadrul & msw, please take a look

sadrul: ui/views
msw: c/b/ui/views

[msw, 2015-07-10 18:35:50]

https://codereview.chromium.org/1228213003/diff/60001/ui/views/controls/butto...
File ui/views/controls/button/blue_button_unittest.cc (right):

https://codereview.chromium.org/1228213003/diff/60001/ui/views/controls/butto...
ui/views/controls/button/blue_button_unittest.cc:18: class TestBlueButton :
public BlueButton {
nit: TestBlueButton seems unnecessary, remove it?

https://codereview.chromium.org/1228213003/diff/60001/ui/views/controls/butto...
ui/views/controls/button/blue_button_unittest.cc:38: 
nit: remove blank line

https://codereview.chromium.org/1228213003/diff/60001/ui/views/controls/butto...
ui/views/controls/button/blue_button_unittest.cc:39: // Add to a Widget to pick
up a native theme.
nit: move this comment above the |widget| decl and say something like "The
buttons must be added to a widget to pick up a native theme."

https://codereview.chromium.org/1228213003/diff/60001/ui/views/controls/butto...
ui/views/controls/button/blue_button_unittest.cc:53:
EXPECT_FALSE(gfx::BitmapsAreEqual(button_canvas.ExtractImageRep().sk_bitmap(),
This test is somewhat ridiculous... it should at least check that the buttons
wind up being the same size before comparing their painted appearances.

https://codereview.chromium.org/1228213003/diff/60001/ui/views/controls/butto...
ui/views/controls/button/blue_button_unittest.cc:56: // Make sure it's still
used after the native theme "changes".
nit: maybe remove this second check now?

https://codereview.chromium.org/1228213003/diff/60001/ui/views/controls/butto...
File ui/views/controls/button/label_button_unittest.cc (right):

https://codereview.chromium.org/1228213003/diff/60001/ui/views/controls/butto...
ui/views/controls/button/label_button_unittest.cc:35: LabelButton&
MakeTestButton(const base::string16& text) {
hmm, returning a non-const ref is a bit odd, why not a pointer?
Or maybe have SetUp create a button, tests can do button()->SetText, etc.

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

sorry, a few more

https://codereview.chromium.org/1228213003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc (right):

https://codereview.chromium.org/1228213003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc:290: // Create the
Widget. This ensures the following calculation correctly
Hmm, where does the view get added to the widget?

https://codereview.chromium.org/1228213003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc:292:
ViewEventTestBase::SetUp();
nit: maybe call this at the top of the function?

https://codereview.chromium.org/1228213003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc:309:
window_->SetSize(GetPreferredSize());
why is this needed now?

[tapted, 2015-07-13 03:39:39]

Patchset #5 (id:80001) has been deleted

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

https://codereview.chromium.org/1228213003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc (right):

https://codereview.chromium.org/1228213003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc:290: // Create the
Widget. This ensures the following calculation correctly
On 2015/07/10 18:40:07, msw wrote:
> Hmm, where does the view get added to the widget?

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.

https://codereview.chromium.org/1228213003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc:292:
ViewEventTestBase::SetUp();
On 2015/07/10 18:40:07, msw wrote:
> nit: maybe call this at the top of the function?

(see above)

https://codereview.chromium.org/1228213003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc:309:
window_->SetSize(GetPreferredSize());
On 2015/07/10 18:40:07, msw wrote:
> why is this needed now?

(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+)

https://codereview.chromium.org/1228213003/diff/60001/ui/views/controls/butto...
File ui/views/controls/button/blue_button_unittest.cc (right):

https://codereview.chromium.org/1228213003/diff/60001/ui/views/controls/butto...
ui/views/controls/button/blue_button_unittest.cc:18: class TestBlueButton :
public BlueButton {
On 2015/07/10 18:35:50, msw wrote:
> nit: TestBlueButton seems unnecessary, remove it?

Done (it's only used to promote OnNativeThemeChanged from protected to public,
but that's obsolete with the comment below).

https://codereview.chromium.org/1228213003/diff/60001/ui/views/controls/butto...
ui/views/controls/button/blue_button_unittest.cc:38: 
On 2015/07/10 18:35:50, msw wrote:
> nit: remove blank line

Done.

https://codereview.chromium.org/1228213003/diff/60001/ui/views/controls/butto...
ui/views/controls/button/blue_button_unittest.cc:39: // Add to a Widget to pick
up a native theme.
On 2015/07/10 18:35:50, msw wrote:
> nit: move this comment above the |widget| decl and say something like "The
> buttons must be added to a widget to pick up a native theme."

Done.

https://codereview.chromium.org/1228213003/diff/60001/ui/views/controls/butto...
ui/views/controls/button/blue_button_unittest.cc:53:
EXPECT_FALSE(gfx::BitmapsAreEqual(button_canvas.ExtractImageRep().sk_bitmap(),
On 2015/07/10 18:35:50, msw wrote:
> This test is somewhat ridiculous... it should at least check that the buttons
> wind up being the same size before comparing their painted appearances.

You are right to accuse it :). The size actually didn't match because
BlueButtons are STYLE_BUTTON, but |button| is STYLE_TEXTBUTTON. (and
SetStyle(STYLE_BUTTON) does SetMinSize(70,33)).

So, I've added a SetStyle(STYLE_BUTTON) call to |button| for a more compelling
comparison.

Also added style and FocusPainter checks.

https://codereview.chromium.org/1228213003/diff/60001/ui/views/controls/butto...
ui/views/controls/button/blue_button_unittest.cc:56: // Make sure it's still
used after the native theme "changes".
On 2015/07/10 18:35:50, msw wrote:
> nit: maybe remove this second check now?

Done.

https://codereview.chromium.org/1228213003/diff/60001/ui/views/controls/butto...
File ui/views/controls/button/label_button_unittest.cc (right):

https://codereview.chromium.org/1228213003/diff/60001/ui/views/controls/butto...
ui/views/controls/button/label_button_unittest.cc:35: LabelButton&
MakeTestButton(const base::string16& text) {
On 2015/07/10 18:35:50, msw wrote:
> hmm, returning a non-const ref is a bit odd, why not a pointer?
> Or maybe have SetUp create a button, tests can do button()->SetText, etc.

Heh, this was really just to make the diff less churny. Some of the tests are
for init flows, so I initially didn't want to do the creation in SetUp/TearDown,
but we can do the interesting init flow test without adding to a widget.

Done.

https://codereview.chromium.org/1228213003/diff/160001/ui/views/controls/butt...
File ui/views/controls/button/label_button.cc (left):

https://codereview.chromium.org/1228213003/diff/160001/ui/views/controls/butt...
ui/views/controls/button/label_button.cc:89: SetAccessibleName(text);
After updating the Init tests, it seemed silly to keep this redundant statement.
It didn't really have good test coverage, so I added some, but let me know if
you want that split out.

[msw, 2015-07-13 17:54:04]

lgtm; thanks for adding the ax test.

https://codereview.chromium.org/1228213003/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc (right):

https://codereview.chromium.org/1228213003/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc:334: // This code
looks a bit hacky, but it is written so that it shouldn't
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)

[sadrul, 2015-07-14 03:26:55]

nice! lgtm

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

Thanks!

https://codereview.chromium.org/1228213003/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc (right):

https://codereview.chromium.org/1228213003/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc:334: // This code
looks a bit hacky, but it is written so that it shouldn't
On 2015/07/13 17:54:03, msw wrote:
> 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)

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)

[tapted, 2015-07-14 05:36:24]

tapted@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[tapted, 2015-07-14 05:36:25]

+Paweł. PTAL for OWNERS in chrome/test

[Paweł Hajdan Jr., 2015-07-14 23:31:42]

chrome/test LGTM

[tapted, 2015-07-15 00:09:14]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2015-07-15 00:09:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1228213003/160001

[commit-bot: I haz the power, 2015-07-15 01:20:36]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2015-07-15 01:21:42]

Patchset 8 (id:??) landed as
https://crrev.com/c6db86a78cba9a415fb75b0a595ad84d9651056c
Cr-Commit-Position: refs/heads/master@{#338796}