Add new views::LabelButton and LabelButtonBorder.

Add new views::LabelButton and LabelButtonBorder.
(a simplified replacement for TextButton[Base], etc.)

Add the LabelButton class, based on TextButton.
(simplify, cleanup, re-use views controls, etc.)
Add a supporting LabelButtonBorder class.
Add unit tests and a views example in ButtonExample.
Add ImageView interactive flag for hit-testing.
Make Label::text() return a const string16 ref.

TODO(followup): Replace TextButton[Base] use, etc.
TODO(followup): Fix CanvasSkia multi-line label painting.

BUG=155363
TEST=views examples use of the new LabelButton, etc.
R=sky@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162245

[msw, 2012-10-10 22:43:37]

Hey Scott, this is low-pri, but please take a look; thanks!

[sky, 2012-10-11 00:39:43]

Peter was expressing his unhappiness with the current set of button classes
recently. Ben and myself agreed with him that there are far too many classes
related to drawing buttons. Additionally there is a far amount of overlap.

I was inclined to go with a monolithic class that contains all the functionality
we use (there really isn't that much difference between the classes). Peter may
have had different ideas. You interested in cleaning up all the button classes?

[msw, 2012-10-11 00:54:26]

On 2012/10/11 00:39:43, sky wrote:
> Peter was expressing his unhappiness with the current set of button classes
> recently. Ben and myself agreed with him that there are far too many classes
> related to drawing buttons. Additionally there is a far amount of overlap.
> 
> I was inclined to go with a monolithic class that contains all the
functionality
> we use (there really isn't that much difference between the classes). Peter
may
> have had different ideas. You interested in cleaning up all the button
classes?

Absolutely! This new (IMO, better) LabelButton could replace most/all TextButton
uses as-is. Others would be a little more work; but I definitely like
consolidation/polish and views work :)

[sky, 2012-10-11 16:39:06]

What about MenuButton and ImageButton? Would they be folded in too?

  -Scott

On Wed, Oct 10, 2012 at 5:54 PM,  <msw@chromium.org> wrote:
> On 2012/10/11 00:39:43, sky wrote:
>>
>> Peter was expressing his unhappiness with the current set of button
>> classes
>> recently. Ben and myself agreed with him that there are far too many
>> classes
>> related to drawing buttons. Additionally there is a far amount of overlap.
>
>
>> I was inclined to go with a monolithic class that contains all the
>
> functionality
>>
>> we use (there really isn't that much difference between the classes).
>> Peter
>
> may
>>
>> have had different ideas. You interested in cleaning up all the button
>
> classes?
>
> Absolutely! This new (IMO, better) LabelButton could replace most/all
> TextButton
> uses as-is. Others would be a little more work; but I definitely like
> consolidation/polish and views work :)
>
> https://codereview.chromium.org/11068012/

[msw, 2012-10-11 19:18:21]

> What about MenuButton and ImageButton? Would they be folded in too?
Probably; each adds to LabelButton, and menus will take some thought:
ImageButton: SetOverlayImage, SetBackground(?), v.alignment.
MenuButton: merge Listener, offset(?), marker==LabelButton::image_.
ButtonDropDown: (add ButtonListener::ButtonHeldDown?), MenuModel*(?).
I'd also cleanup Checkbox/Radio, styling(?) and overall use/tests.

[sky, 2012-10-11 19:32:01]

I like this approach. Longer term can we fold Button and CustomButton into this
and only have one?

https://codereview.chromium.org/11068012/diff/10026/ui/views/controls/button/...
File ui/views/controls/button/button_border.h (right):

https://codereview.chromium.org/11068012/diff/10026/ui/views/controls/button/...
ui/views/controls/button/button_border.h:17: class VIEWS_EXPORT ButtonBorder :
public Border {
Should this be named LabelButtonBorder?

https://codereview.chromium.org/11068012/diff/10026/ui/views/controls/button/...
File ui/views/controls/button/label_button.h (right):

https://codereview.chromium.org/11068012/diff/10026/ui/views/controls/button/...
ui/views/controls/button/label_button.h:33: Label* GetLabel() const;
Is there a need to expose this? I think the label and imageview are
implementation details.

[msw, 2012-10-11 20:27:01]

Done; please take another look. Yeah, I'd start by merging Button and
CustomButton, then consider merging LabelButton.

https://codereview.chromium.org/11068012/diff/10026/ui/views/controls/button/...
File ui/views/controls/button/button_border.h (right):

https://codereview.chromium.org/11068012/diff/10026/ui/views/controls/button/...
ui/views/controls/button/button_border.h:17: class VIEWS_EXPORT ButtonBorder :
public Border {
On 2012/10/11 19:32:01, sky wrote:
> Should this be named LabelButtonBorder?

Done.

https://codereview.chromium.org/11068012/diff/10026/ui/views/controls/button/...
File ui/views/controls/button/label_button.h (right):

https://codereview.chromium.org/11068012/diff/10026/ui/views/controls/button/...
ui/views/controls/button/label_button.h:33: Label* GetLabel() const;
On 2012/10/11 19:32:01, sky wrote:
> Is there a need to expose this? I think the label and imageview are
> implementation details.

Done; (required adding GetText() and [G|S]etTextMultiLine()).
Replacing TextButton will also require exposing Label's shadowing.
I hoped to avoid exposing Label functions, but it's a trade-off.

[sky, 2012-10-11 23:01:56]

https://codereview.chromium.org/11068012/diff/10027/ui/views/controls/button/...
File ui/views/controls/button/label_button.cc (right):

https://codereview.chromium.org/11068012/diff/10027/ui/views/controls/button/...
ui/views/controls/button/label_button.cc:152: gfx::Size size =
label_->GetPreferredSize();
Can't you call to the layoutmanager then enlarge as necessary?

https://codereview.chromium.org/11068012/diff/10027/ui/views/controls/button/...
ui/views/controls/button/label_button.cc:238: SetState(BS_DISABLED);
As long as someone can subclass StateChanged this makes me nervous.

https://codereview.chromium.org/11068012/diff/10027/ui/views/controls/button/...
ui/views/controls/button/label_button.cc:241: GridLayout* layout =
GridLayout::CreatePanel(this);
Why are you using CreatePanel here? Since you blow away the insets on 243 this
shouldn't be needed.

https://codereview.chromium.org/11068012/diff/10027/ui/views/controls/button/...
File ui/views/controls/button/label_button.h (right):

https://codereview.chromium.org/11068012/diff/10027/ui/views/controls/button/...
ui/views/controls/button/label_button.h:49: void set_min_size(gfx::Size
min_size) { min_size_ = min_size; }
const gfx::Size&

https://codereview.chromium.org/11068012/diff/10027/ui/views/controls/button/...
ui/views/controls/button/label_button.h:55: bool native_theme() const { return
native_theme_; }
It isn't readily obvious what this does. Document it, along with what the
default is.

[msw, 2012-10-15 16:48:33]

Changed to a custom Layout/GetPreferredSize; etc. Please take another look.

https://codereview.chromium.org/11068012/diff/10027/ui/views/controls/button/...
File ui/views/controls/button/label_button.cc (right):

https://codereview.chromium.org/11068012/diff/10027/ui/views/controls/button/...
ui/views/controls/button/label_button.cc:152: gfx::Size size =
label_->GetPreferredSize();
On 2012/10/11 23:01:56, sky wrote:
> Can't you call to the layoutmanager then enlarge as necessary?

This seems necessary for the new custom Layout() approach.

https://codereview.chromium.org/11068012/diff/10027/ui/views/controls/button/...
ui/views/controls/button/label_button.cc:238: SetState(BS_DISABLED);
On 2012/10/11 23:01:56, sky wrote:
> As long as someone can subclass StateChanged this makes me nervous.

This is moot with the new custom Layout() approach.

https://codereview.chromium.org/11068012/diff/10027/ui/views/controls/button/...
ui/views/controls/button/label_button.cc:241: GridLayout* layout =
GridLayout::CreatePanel(this);
On 2012/10/11 23:01:56, sky wrote:
> Why are you using CreatePanel here? Since you blow away the insets on 243 this
> shouldn't be needed.

This is moot with the new custom Layout() approach.

https://codereview.chromium.org/11068012/diff/10027/ui/views/controls/button/...
File ui/views/controls/button/label_button.h (right):

https://codereview.chromium.org/11068012/diff/10027/ui/views/controls/button/...
ui/views/controls/button/label_button.h:49: void set_min_size(gfx::Size
min_size) { min_size_ = min_size; }
On 2012/10/11 23:01:56, sky wrote:
> const gfx::Size&

Done.

https://codereview.chromium.org/11068012/diff/10027/ui/views/controls/button/...
ui/views/controls/button/label_button.h:55: bool native_theme() const { return
native_theme_; }
On 2012/10/11 23:01:56, sky wrote:
> It isn't readily obvious what this does. Document it, along with what the
> default is.

Done.

[sky, 2012-10-16 15:30:10]

LGTM

[commit-bot: I haz the power, 2012-10-16 18:36:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/11068012/20016

[commit-bot: I haz the power, 2012-10-16 21:08:24]

Change committed as 162245