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

ui/views/controls/button/label_button.h

 // 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.
 
 #ifndef UI_VIEWS_CONTROLS_BUTTON_LABEL_BUTTON_H_
 #define UI_VIEWS_CONTROLS_BUTTON_LABEL_BUTTON_H_
 
 #include "base/compiler_specific.h"
 #include "base/memory/scoped_ptr.h"
 #include "third_party/skia/include/core/SkColor.h"
@@ skipping 10 matching lines @@
 
 // LabelButton is a button with text and an icon, it's not focusable by default.
 class VIEWS_EXPORT LabelButton : public CustomButton,
                                  public NativeThemeDelegate {
  public:
   // The length of the hover fade animation.
   static const int kHoverAnimationDurationMs;
 
   static const char kViewClassName[];
 
+  // Construct an uninitialized LabelButton with a Listener. Client code must
+  // also call Init() or InitAsButton() to initialize the (possibly themed)
+  // button borders.
+  explicit LabelButton(ButtonListener* listener);
+
+  // Deprecated (calls virtual methods). TODO(tapted): Remove this overload.
   LabelButton(ButtonListener* listener, const base::string16& text);
+
   ~LabelButton() override;
 
+  // Initializes the LabelButton. If borders have not been set with SetBorder(),
+  // creates default themed borders for the current ButtonStyle. The default
+  // button style defaults to STYLE_TEXTBUTTON, or it be set by a subclass.

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

Can there be a single 'Init(text, style)' instead? If not, let's call this one
InitAsTextButton().

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

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]

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

+  void Init(const base::string16& text);
+
+  // Calls Init() after setting the ButtonStyle to STYLE_BUTTON.
+  void InitAsButton(const base::string16& text);
+
   // Get or set the image shown for the specified button state.
   // GetImage returns the image for STATE_NORMAL if the state's image is empty.
   virtual const gfx::ImageSkia& GetImage(ButtonState for_state);
   void SetImage(ButtonState for_state, const gfx::ImageSkia& image);
 
   // Get or set the text shown on the button.
   const base::string16& GetText() const;
   virtual void SetText(const base::string16& text);
 
   // Set the text color shown for the specified button state.
@@ skipping 22 matching lines @@
   void SetHorizontalAlignment(gfx::HorizontalAlignment alignment);
 
   // Call SetMinSize(gfx::Size()) to clear the monotonically increasing size.
   void SetMinSize(const gfx::Size& min_size);
   void SetMaxSize(const gfx::Size& max_size);
 
   // Get or set the option to handle the return key; false by default.
   bool is_default() const { return is_default_; }
   void SetIsDefault(bool is_default);
 
-  // Get or set the button's overall style; the default is |STYLE_TEXTBUTTON|.
+  // Gets the button's overall style; the default is |STYLE_TEXTBUTTON|.
   ButtonStyle style() const { return style_; }
+
+  // Sets the button style and updates (possibly themed) borders. TODO(tapted):
+  // Make this method protected after callers have switched to InitAsButton().
   void SetStyle(ButtonStyle style);
 
   // Set the spacing between the image and the text. Shrinking the spacing
   // will not shrink the overall button size, as it is monotonically increasing.
   // Call SetMinSize(gfx::Size()) to clear the size if needed.
   void SetImageLabelSpacing(int spacing);
 
   void SetFocusPainter(scoped_ptr<Painter> focus_painter);
   Painter* focus_painter() { return focus_painter_.get(); }
 
   // View:
   void SetBorder(scoped_ptr<Border> border) override;
   gfx::Size GetPreferredSize() const override;
   int GetHeightForWidth(int w) const override;
   void Layout() override;
   const char* GetClassName() const override;
 
  protected:
   ImageView* image() const { return image_; }
   Label* label() const { return label_; }
 
+  // Called by a subclass constructor to cause the Init(const string16&)
+  // overload to use a default style other than STYLE_TEXTBUTTON.
+  void SetDefaultStyle(ButtonStyle style);
+
   // Returns the available area for the label and image. Subclasses can change
   // these bounds if they need room to do manual painting.
   virtual gfx::Rect GetChildAreaBounds();
 
   // View:
   void OnPaint(gfx::Canvas* canvas) override;
   void OnFocus() override;
   void OnBlur() override;
   void OnNativeThemeChanged(const ui::NativeTheme* theme) override;
 
@@ skipping 83 matching lines @@
   int image_label_spacing_;
 
   scoped_ptr<Painter> focus_painter_;
 
   DISALLOW_COPY_AND_ASSIGN(LabelButton);
 };
 
 }  // namespace views
 
 #endif  // UI_VIEWS_CONTROLS_BUTTON_LABEL_BUTTON_H_