views::LabelButton should not call virtual methods from its constructor

views::LabelButton should not call virtual methods from its constructor

LabelButton::SetText() and LabelButton::CreateDefaultBorder() are both
virtual, and both called from the LabelButton constructor (the latter
via SetStyle()). This is typically frowned upon. It also means Button
types that override CreateDefaultBorder do not get their override called
when the LabelButton constructor calls SetStyle():

These overrides currently only "work" because when the view hierarchy is
first added to the Widget, PropagateNativeThemeChanged() is triggered
which re-initializes the border (typically for a third time).

This setup also results in some sub-optimal initialization paths:

- SetStyle(..), when used, is called just after construction, or from a
subclass constructor. It overrides the SetStyle call done in the
LabelButton constructor which is now wasted work. The wasted work
includes creating a LabelButtonBorder with 18 ImageSkias in it, fetched
from the ResourceBundle.

- For buttons with custom borders (or a null border), the SetStyle(..)
call first creates a themed border which is then destroyed with a
SetBorder call.

In these cases, there's no way for a subclass to prevent the behavior
because an overide of CreateDefaultBorder can't be called via the
LabelButton constructor.

To fix, add InitAs...() methods, which replace LabelButton::SetStyle().
The style was always part of the initialization (i.e. client code that 
changes the default style all calls SetStyle() just after construction).
However, currently the only way to alter the style is to first
initialize with the default style, then replace it.

Client code that currently calls SetStyle(STYLE_BUTTON) now just calls
InitAsButton(label). Client code that used the default style
(STYLE_TEXTBUTTON) now needs to calls InitAsTextbutton(label). By
calling SetBorder() before Init(), a LabelButton can now skip generating
themed borders altogether.

This CL is Part 1 of N, which migrates code compiled along with
`views_unittests`. Adds overloads so that `chrome` and other targets can
be addressed in follow-ups (and the overloads removed).

BUG=506729

[tapted, 2015-07-06 23:49:06]

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

[tapted, 2015-07-07 00:10:09]

Hi sadrul, please take a look.

(I'm working on themed buttons for Mac. While tracing, I ran into this, and I
think it needs to be fixed, but it'll take a few CLs).

Thanks!

[tapted, 2015-07-07 00:12:29]

(oh, and patchset 5 has a preview of how the `chrome` target would be fixed, but
it's too much for 1 CL).

[sadrul, 2015-07-08 05:59:11]

https://codereview.chromium.org/1216673005/diff/140001/ui/views/bubble/bubble...
File ui/views/bubble/bubble_frame_view.cc (left):

https://codereview.chromium.org/1216673005/diff/140001/ui/views/bubble/bubble...
ui/views/bubble/bubble_frame_view.cc:107: close->SetBorder(nullptr);
What happens if this call to SetBorder() happens after the call to Init()?

https://codereview.chromium.org/1216673005/diff/140001/ui/views/controls/butt...
File ui/views/controls/button/blue_button.cc (right):

https://codereview.chromium.org/1216673005/diff/140001/ui/views/controls/butt...
ui/views/controls/button/blue_button.cc:26: InitAsButton(text);
This is going to call virtual functions from the ctor, right? Maybe mark
BlueButton as 'final'?

https://codereview.chromium.org/1216673005/diff/140001/ui/views/controls/butt...
File ui/views/controls/button/label_button.h (right):

https://codereview.chromium.org/1216673005/diff/140001/ui/views/controls/butt...
ui/views/controls/button/label_button.h:43: // button style defaults to
STYLE_TEXTBUTTON, or it be set by a subclass.
Can there be a single 'Init(text, style)' instead? If not, let's call this one
InitAsTextButton().

[tapted, 2015-07-08 06:31:33]

(no new code yet - just a question wrt InitAsTextButton)

https://codereview.chromium.org/1216673005/diff/140001/ui/views/bubble/bubble...
File ui/views/bubble/bubble_frame_view.cc (left):

https://codereview.chromium.org/1216673005/diff/140001/ui/views/bubble/bubble...
ui/views/bubble/bubble_frame_view.cc:107: close->SetBorder(nullptr);
On 2015/07/08 05:59:11, sadrul wrote:
> What happens if this call to SetBorder() happens after the call to Init()?

That still works fine - it just wastes work because Init() will add a themed
border, which SetBorder will then replace. (Init() after a SetBorder() avoids
creating a themed border because border_is_themed_border_ is false after
SetBorder is called by something other other than UpdateThemedBorder())

https://codereview.chromium.org/1216673005/diff/140001/ui/views/controls/butt...
File ui/views/controls/button/blue_button.cc (right):

https://codereview.chromium.org/1216673005/diff/140001/ui/views/controls/butt...
ui/views/controls/button/blue_button.cc:26: InitAsButton(text);
On 2015/07/08 05:59:11, sadrul wrote:
> This is going to call virtual functions from the ctor, right? Maybe mark
> BlueButton as 'final'?

Yes. Unfortunately lots of things inherit from BlueButton :/. I have a TODO in
blue_button.h to remove this overload in a follow-up. patchset 5 has it already
removed, but the diff was getting too big.

https://codereview.chromium.org/1216673005/diff/140001/ui/views/controls/butt...
File ui/views/controls/button/label_button.h (right):

https://codereview.chromium.org/1216673005/diff/140001/ui/views/controls/butt...
ui/views/controls/button/label_button.h:43: // button style defaults to
STYLE_TEXTBUTTON, or it be set by a subclass.
On 2015/07/08 05:59:11, sadrul wrote:
> Can there be a single 'Init(text, style)' instead? If not, let's call this one
> InitAsTextButton().

I actually started with Init(style, text) in Patchset 2. But everything that
called it was passing STYLE_BUTTON. This was because a `default` style was
supported -- everything wants "default" or STYLE_BUTTON.

Then I tossed up InitAsTextButton for a long time too.. I went with `Init`
because it allows a subclass (like BlueButton) to change the default to
STYLE_BUTTON, without a caller having to know that they should call InitAsButton
instead of InitAsTextButton. This will come up more in follow-ups CLs, once
BlueButton's constructor no longer calls virtual methods.

Having `Init`+`SetDefaultStyle` also means that if, in future, a subclass finds
a need to override Init(), it's clear which Init() it should override. That is,
we can guarantee that InitAsFoo() always calls Init(), which may be overridden.

This can also be true for the `Init(text, style)`, flavour, but it means feeding
a lot of STYLE_TEXTBUTTON arguments into places that don't currently need to
care about it.

So, does that change your mind :) ?

If not, I'm fine with InitAsTextButton too. It would just mean discarding the
concept of a default style. I could get rid of SetDefaultStyle as well. (and if
the override-Init() thing comes up in future, it could be achieved with a
protected Init() method or some other solution).

[tapted, 2015-07-09 00:53:48]

actually I tried out InitAsTextbutton() (and discarded the notion of a "default"
style). I think it's a good improvement. PTAL.

https://codereview.chromium.org/1216673005/diff/140001/ui/views/controls/butt...
File ui/views/controls/button/blue_button.cc (right):

https://codereview.chromium.org/1216673005/diff/140001/ui/views/controls/butt...
ui/views/controls/button/blue_button.cc:26: InitAsButton(text);
On 2015/07/08 06:31:33, tapted wrote:
> On 2015/07/08 05:59:11, sadrul wrote:
> > This is going to call virtual functions from the ctor, right? Maybe mark
> > BlueButton as 'final'?
> 
> Yes. Unfortunately lots of things inherit from BlueButton :/. I have a TODO in
> blue_button.h to remove this overload in a follow-up. patchset 5 has it
already
> removed, but the diff was getting too big.

Actually "lots" was an exaggeration :). There's just one child class in a test,
and it's straightforward to remove. Probably had it confused with MenuButton
which has a ~dozen subclass types.

So, Done! BlueButton now final with a comment, and the constructor is no longer
overloaded.

https://codereview.chromium.org/1216673005/diff/140001/ui/views/controls/butt...
File ui/views/controls/button/label_button.h (right):

https://codereview.chromium.org/1216673005/diff/140001/ui/views/controls/butt...
ui/views/controls/button/label_button.h:43: // button style defaults to
STYLE_TEXTBUTTON, or it be set by a subclass.
On 2015/07/08 06:31:33, tapted wrote:
> On 2015/07/08 05:59:11, sadrul wrote:
> > Can there be a single 'Init(text, style)' instead? If not, let's call this
one
> > InitAsTextButton().

Done. (went with Textbutton over TextButton to match the enum)

[tapted, 2015-07-09 11:30:24]

After burying myself in callgraphs all day (context: http://crbug.com/508410 ),
I think I've got a simpler solution cooking in my head. (Basically, don't call
virtual methods from SetStyle() - pretty sure there's a way to make it all work,
so long as we guarantee SetStyle is never called while the LabelButton is
already in a Widget, and that's easy to DCHECK). SetText() is also virtual, but
in my adventures I'm pretty sure I found only one override, and there should be
a way to support what it needs in a different way.

I think I can resolve both the wasted SetBorder() (i.e. when it's done after
Init()), and the repeated/redundant UpdateThemedBorder() calls. Let me put that
together tomorrow.

[tapted, 2015-07-10 03:29:22]

On 2015/07/09 11:30:24, tapted wrote:
> After burying myself in callgraphs all day (context: http://crbug.com/508410
),
> I think I've got a simpler solution cooking in my head. (Basically, don't call
> virtual methods from SetStyle() - pretty sure there's a way to make it all
work,
> so long as we guarantee SetStyle is never called while the LabelButton is
> already in a Widget, and that's easy to DCHECK). SetText() is also virtual,
but
> in my adventures I'm pretty sure I found only one override, and there should
be
> a way to support what it needs in a different way.
> 
> I think I can resolve both the wasted SetBorder() (i.e. when it's done after
> Init()), and the repeated/redundant UpdateThemedBorder() calls. Let me put
that
> together tomorrow.

created https://codereview.chromium.org/1228213003/ . I'll close this.