Add the button style for combobox

Add the button style for combobox

Added a new style for combobox, which enables the users to click the text part of a combobox.
Please see the screenshots at crbug/305494.

BUG=305494
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=240057

[hajimehoshi, 2013-11-05 10:10:26]

Can you take a look?  Thank you in advance.

https://codereview.chromium.org/59383003/diff/80001/ui/views/controls/button/...
File ui/views/controls/button/multi_target_button.cc (right):

https://codereview.chromium.org/59383003/diff/80001/ui/views/controls/button/...
ui/views/controls/button/multi_target_button.cc:21: new
views::BoxLayout(views::BoxLayout::kHorizontal, 0, 0, -4));
-4 is to remove the space between two buttons. Would you tell me where such kind
of width is defined?

[sky, 2013-11-05 16:29:36]

Why are we trying to do a new button here and not one that looks like the rest
of our UI?

https://codereview.chromium.org/59383003/diff/80001/chrome/browser/ui/transla...
File chrome/browser/ui/translate/translate_denial_menu_model_observer.h (right):

https://codereview.chromium.org/59383003/diff/80001/chrome/browser/ui/transla...
chrome/browser/ui/translate/translate_denial_menu_model_observer.h:19: };
protected virtual destructor.

https://codereview.chromium.org/59383003/diff/80001/ui/views/controls/button/...
File ui/views/controls/button/multi_target_button.cc (right):

https://codereview.chromium.org/59383003/diff/80001/ui/views/controls/button/...
ui/views/controls/button/multi_target_button.cc:23: button_ = new
views::LabelButton(this, menu_model->GetLabelAt(0));
Using two buttons side by side is problematic because the borders aren't always
going to line up. This should be a single button. In fact you likely want to
subclass an existing button and customize the painting to get the effect you're
after.

[kenjibaheux, 2013-11-06 03:15:34]

> Why are we trying to do a new button here and not one that looks like the rest
> of our UI?

This design is based on an already reviewed design for the Save password UX:
https://code.google.com/p/chromium/issues/detail?id=299955#c4

Rebecca and Ainslie told us to use the same design for Translate.

[kenjibaheux, 2013-11-06 03:20:37]

https://codereview.chromium.org/59383003/diff/80001/ui/views/controls/button/...
> ui/views/controls/button/multi_target_button.cc:23: button_ = new
> views::LabelButton(this, menu_model->GetLabelAt(0));
> Using two buttons side by side is problematic because the borders aren't
always
> going to line up.

Could you tell us the conditions that would result in a bad alignment?

[sky, 2013-11-06 14:36:39]

You're taking two buttons and placing them side by side. How well the
two look next to each other is highly dependent upon the borders they
use, and I guarantee will break when we start changing borders as we
often do. Additionally you should not need to create a new button for
this. We should either make all comboboxs look like this or if we need
both looks then make it an option on combobox. Is there a reason we
don't want *all* comboboxs to look like this?

  -Scott

On Tue, Nov 5, 2013 at 7:20 PM,  <kenjibaheux@chromium.org> wrote:
>
>
https://codereview.chromium.org/59383003/diff/80001/ui/views/controls/button/...
>>
>> ui/views/controls/button/multi_target_button.cc:23: button_ = new
>> views::LabelButton(this, menu_model->GetLabelAt(0));
>> Using two buttons side by side is problematic because the borders aren't
>
> always
>>
>> going to line up.
>
>
> Could you tell us the conditions that would result in a bad alignment?
>
> https://codereview.chromium.org/59383003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kenjibaheux, 2013-11-07 04:39:06]

On 2013/11/06 14:36:39, sky wrote:
> Additionally you should not need to create a new button for
> this. We should either make all comboboxs look like this or if we need
> both looks then make it an option on combobox. Is there a reason we
> don't want *all* comboboxs to look like this?

This new control behaves differently than a combobox. Also, this new behavior
only make senses for some use cases. So, we don't want all combobox to look like
this.

Actually, this control is closer to a button. It's sometime called "split
button":
 1. If you click on the visible (default) action, you will perform that action.
 2. If you click on the arrow, you will be able to pick and perform a different
action.


Whereas with a traditional combobox, you are only allowed you to pick an option:
there is no actions involved at all. So, in the non-editable case, clicking
anywhere (visible item, arrow) will open up the dropdown list of items.


Given this explanation, should this new "split button" still be a child of
combobox?
Depending on this, Hajime will think about the alignment issue.

[sky, 2013-11-07 17:06:41]

Wow, are you saying we have a view that looks exactly like a combobox
but behaves differently than a combobox? Hm, that makes no sense.
I'll ping Alex about this.

  -Scott

On Wed, Nov 6, 2013 at 8:39 PM,  <kenjibaheux@chromium.org> wrote:
> On 2013/11/06 14:36:39, sky wrote:
>>
>> Additionally you should not need to create a new button for
>> this. We should either make all comboboxs look like this or if we need
>> both looks then make it an option on combobox. Is there a reason we
>> don't want *all* comboboxs to look like this?
>
>
> This new control behaves differently than a combobox. Also, this new
> behavior
> only make senses for some use cases. So, we don't want all combobox to look
> like
> this.
>
> Actually, this control is closer to a button. It's sometime called "split
> button":
>  1. If you click on the visible (default) action, you will perform that
> action.
>  2. If you click on the arrow, you will be able to pick and perform a
> different
> action.
>
>
> Whereas with a traditional combobox, you are only allowed you to pick an
> option:
> there is no actions involved at all. So, in the non-editable case, clicking
> anywhere (visible item, arrow) will open up the dropdown list of items.
>
>
> Given this explanation, should this new "split button" still be a child of
> combobox?
> Depending on this, Hajime will think about the alignment issue.
>
> https://codereview.chromium.org/59383003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2013-11-08 18:03:59]

On 2013/11/07 17:06:41, sky wrote:
> Wow, are you saying we have a view that looks exactly like a combobox
> but behaves differently than a combobox? Hm, that makes no sense.
> I'll ping Alex about this.
> 
>   -Scott
> 
> On Wed, Nov 6, 2013 at 8:39 PM,  <mailto:kenjibaheux@chromium.org> wrote:
> > On 2013/11/06 14:36:39, sky wrote:
> >>
> >> Additionally you should not need to create a new button for
> >> this. We should either make all comboboxs look like this or if we need
> >> both looks then make it an option on combobox. Is there a reason we
> >> don't want *all* comboboxs to look like this?
> >
> >
> > This new control behaves differently than a combobox. Also, this new
> > behavior
> > only make senses for some use cases. So, we don't want all combobox to look
> > like
> > this.
> >
> > Actually, this control is closer to a button. It's sometime called "split
> > button":
> >  1. If you click on the visible (default) action, you will perform that
> > action.
> >  2. If you click on the arrow, you will be able to pick and perform a
> > different
> > action.
> >
> >
> > Whereas with a traditional combobox, you are only allowed you to pick an
> > option:
> > there is no actions involved at all. So, in the non-editable case, clicking
> > anywhere (visible item, arrow) will open up the dropdown list of items.
> >
> >
> > Given this explanation, should this new "split button" still be a child of
> > combobox?
> > Depending on this, Hajime will think about the alignment issue.
> >
> > https://codereview.chromium.org/59383003/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Ok, sounds like we want this.

I'm still of the opinion this should be functionality added to combobox though.
Add the ability to enable the clickable mode, which triggers drawing the new
style, and add a new method to the listener that is notified when clicked.

[hajimehoshi, 2013-11-11 08:46:54]

On 2013/11/08 18:03:59, sky wrote:
> On 2013/11/07 17:06:41, sky wrote:
> > Wow, are you saying we have a view that looks exactly like a combobox
> > but behaves differently than a combobox? Hm, that makes no sense.
> > I'll ping Alex about this.
> > 
> >   -Scott
> > 
> > On Wed, Nov 6, 2013 at 8:39 PM,  <mailto:kenjibaheux@chromium.org> wrote:
> > > On 2013/11/06 14:36:39, sky wrote:
> > >>
> > >> Additionally you should not need to create a new button for
> > >> this. We should either make all comboboxs look like this or if we need
> > >> both looks then make it an option on combobox. Is there a reason we
> > >> don't want *all* comboboxs to look like this?
> > >
> > >
> > > This new control behaves differently than a combobox. Also, this new
> > > behavior
> > > only make senses for some use cases. So, we don't want all combobox to
look
> > > like
> > > this.
> > >
> > > Actually, this control is closer to a button. It's sometime called "split
> > > button":
> > >  1. If you click on the visible (default) action, you will perform that
> > > action.
> > >  2. If you click on the arrow, you will be able to pick and perform a
> > > different
> > > action.
> > >
> > >
> > > Whereas with a traditional combobox, you are only allowed you to pick an
> > > option:
> > > there is no actions involved at all. So, in the non-editable case,
clicking
> > > anywhere (visible item, arrow) will open up the dropdown list of items.
> > >
> > >
> > > Given this explanation, should this new "split button" still be a child of
> > > combobox?
> > > Depending on this, Hajime will think about the alignment issue.
> > >
> > > https://codereview.chromium.org/59383003/
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> Ok, sounds like we want this.
> 
> I'm still of the opinion this should be functionality added to combobox
though.
> Add the ability to enable the clickable mode, which triggers drawing the new
> style, and add a new method to the listener that is notified when clicked.

BTW, to perform rendering a button-like border and background, manipulating the
states of two buttons, e.g. 'pressed' or 'hovered', is needed, therefore,
Combobox will have CustomButtons or their offspring as member variables, right?
If so, this is not so different from creating an original view including
buttons. Otherwise, is there any way to render them without button instances in
account of two buttons' states?

[hajimehoshi, 2013-11-11 08:50:46]

Oops, I sent an incomplete message.

Thank you for confirming the necessary of the new control!

[sky, 2013-11-11 15:56:02]

You can always override OnPaint and paint whatever you need.

On Mon, Nov 11, 2013 at 12:50 AM,  <hajimehoshi@chromium.org> wrote:
> Oops, I sent an incomplete message.
>
> Thank you for confirming the necessary of the new control!
>
>
> https://codereview.chromium.org/59383003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hajimehoshi, 2013-11-29 11:03:00]

PTAL

[sky, 2013-12-02 21:28:54]

The size of this diff, and what appears to be very similar code to that of the
button classes has me concerned. If we go with two distinct buttons but have
this class do the actual painting would it be smaller?

https://codereview.chromium.org/59383003/diff/240001/ui/views/controls/combob...
File ui/views/controls/combobox/combobox.h (right):

https://codereview.chromium.org/59383003/diff/240001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.h:34: // A non-editable combobox (aka a
drop-down list or selector).
Add description here of different behaviors. Maybe something like:
Combobox has two distinct parts, the drop down arrow and the text. When the user
clicks on the text the drop down is either shown (STYLE_SHOW_DROP_DOWN_ON_CLICK)
or the listener is notified (STYLE_NOTIFY_ON_CLICK).

https://codereview.chromium.org/59383003/diff/240001/ui/views/controls/combob...
File ui/views/controls/combobox/combobox_listener.h (right):

https://codereview.chromium.org/59383003/diff/240001/ui/views/controls/combob...
ui/views/controls/combobox/combobox_listener.h:20: virtual void
OnTextButtonClicked(Combobox* combobox) {}
OnComboboxTextButtonClicked

https://codereview.chromium.org/59383003/diff/240001/ui/views/controls/focusa...
File ui/views/controls/focusable_border.h (right):

https://codereview.chromium.org/59383003/diff/240001/ui/views/controls/focusa...
ui/views/controls/focusable_border.h:26: void ResetInsets();
I don't want to add anything more to this class. Can you recreate the border if
you need to?

https://codereview.chromium.org/59383003/diff/240001/ui/views/painter.h
File ui/views/painter.h (right):

https://codereview.chromium.org/59383003/diff/240001/ui/views/painter.h#newco...
ui/views/painter.h:26: #define IMAGE_GRID2(x, y) { \
Why do you need a new macro for this? Can't you embed the y in the name, eg
IDR_COMBOBOX_BUTTON_BOTTOM_H and IDR_COMBOBOX_BUTTON_BOTTOM_P ?

[hajimehoshi, 2013-12-03 10:30:52]

Thank you. I added 'TransparentButton' which is used when the button style is
used. This may be fixed to use the buttons whatever the style is. PTAL

https://codereview.chromium.org/59383003/diff/240001/ui/views/controls/combob...
File ui/views/controls/combobox/combobox.h (right):

https://codereview.chromium.org/59383003/diff/240001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.h:34: // A non-editable combobox (aka a
drop-down list or selector).
On 2013/12/02 21:28:55, sky wrote:
> Add description here of different behaviors. Maybe something like:
> Combobox has two distinct parts, the drop down arrow and the text. When the
user
> clicks on the text the drop down is either shown
(STYLE_SHOW_DROP_DOWN_ON_CLICK)
> or the listener is notified (STYLE_NOTIFY_ON_CLICK).

Done. (also changed enum Style).

https://codereview.chromium.org/59383003/diff/240001/ui/views/controls/combob...
File ui/views/controls/combobox/combobox_listener.h (right):

https://codereview.chromium.org/59383003/diff/240001/ui/views/controls/combob...
ui/views/controls/combobox/combobox_listener.h:20: virtual void
OnTextButtonClicked(Combobox* combobox) {}
On 2013/12/02 21:28:55, sky wrote:
> OnComboboxTextButtonClicked

Done.

https://codereview.chromium.org/59383003/diff/240001/ui/views/controls/focusa...
File ui/views/controls/focusable_border.h (right):

https://codereview.chromium.org/59383003/diff/240001/ui/views/controls/focusa...
ui/views/controls/focusable_border.h:26: void ResetInsets();
On 2013/12/02 21:28:55, sky wrote:
> I don't want to add anything more to this class. Can you recreate the border
if
> you need to?

Done.

https://codereview.chromium.org/59383003/diff/240001/ui/views/painter.h
File ui/views/painter.h (right):

https://codereview.chromium.org/59383003/diff/240001/ui/views/painter.h#newco...
ui/views/painter.h:26: #define IMAGE_GRID2(x, y) { \
On 2013/12/02 21:28:55, sky wrote:
> Why do you need a new macro for this? Can't you embed the y in the name, eg
> IDR_COMBOBOX_BUTTON_BOTTOM_H and IDR_COMBOBOX_BUTTON_BOTTOM_P ?

Done.

[sky, 2013-12-03 20:52:29]

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:103: // gfx::AnimationDelegate:
Does SchedulePaint() on this always need to force a SchedulePaint on the parent
the size of the parent?

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:178: ids = GetBodyButtonImageIds(focused,
state);
This and GetMenuButtonImageIds should return the num_ids too (less error prone).

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:188: int id = ids[i];
nit: no need for the temporary here.

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:195: void PaintTandemImages(gfx::Canvas*
canvas,
Isn't tandem too, yet this paints three.

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:215: void PaintTextButton(
This code looks a lot like that offered by Painter. Specifically imagegrid or
HorizontalPainter. How do they differ?

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:275: text_button_(new
TransparentButton(this)),
Is there a reason these need to be always created?

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:303: AddChildView(arrow_button_);
You should also make these non focusable.

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:323: text_border_ = new
FocusableBorder();
Why do you create a new border here?

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:391:
arrow_button_->SetBounds(arrow_button_x, 0, arrow_button_width, height);
This should be done in Layout.

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:609: // TODO(hajimehoshi): Fix the
problem that the arrow button blinks when
DCHECK sender is what you expect.

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:712: bool focused = HasFocus() &&
IsAccessibilityFocusable();
You should only need check HasFocus here.

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
File ui/views/controls/combobox/combobox.h (right):

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.h:205: CustomButton* text_button_;
Document ownership.

[hajimehoshi, 2013-12-04 06:30:01]

Thank you!

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:103: // gfx::AnimationDelegate:
On 2013/12/03 20:52:30, sky wrote:
> Does SchedulePaint() on this always need to force a SchedulePaint on the
parent
> the size of the parent?

Done. (ScheudulePaintInRect with its bounds should be fine)

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:178: ids = GetBodyButtonImageIds(focused,
state);
On 2013/12/03 20:52:30, sky wrote:
> This and GetMenuButtonImageIds should return the num_ids too (less error
prone).

Done.

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:188: int id = ids[i];
On 2013/12/03 20:52:30, sky wrote:
> nit: no need for the temporary here.

Done.

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:195: void PaintTandemImages(gfx::Canvas*
canvas,
On 2013/12/03 20:52:30, sky wrote:
> Isn't tandem too, yet this paints three.

Done.

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:215: void PaintTextButton(
On 2013/12/03 20:52:30, sky wrote:
> This code looks a lot like that offered by Painter. Specifically imagegrid or
> HorizontalPainter. How do they differ?

Done. Menu button images remain. Is it better to create VerticalPainter?

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:275: text_button_(new
TransparentButton(this)),
On 2013/12/03 20:52:30, sky wrote:
> Is there a reason these need to be always created?

Done. (It could be simpler that the buttons always exist.)

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:303: AddChildView(arrow_button_);
On 2013/12/03 20:52:30, sky wrote:
> You should also make these non focusable.

Done.

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:323: text_border_ = new
FocusableBorder();
On 2013/12/03 20:52:30, sky wrote:
> Why do you create a new border here?

It's because there is no way to reset the insets of the FocusableBorder. I'll
add a comment.

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:391:
arrow_button_->SetBounds(arrow_button_x, 0, arrow_button_width, height);
On 2013/12/03 20:52:30, sky wrote:
> This should be done in Layout.

Done.

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:609: // TODO(hajimehoshi): Fix the
problem that the arrow button blinks when
On 2013/12/03 20:52:30, sky wrote:
> DCHECK sender is what you expect.

Done.

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:712: bool focused = HasFocus() &&
IsAccessibilityFocusable();
On 2013/12/03 20:52:30, sky wrote:
> You should only need check HasFocus here.

Done.

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
File ui/views/controls/combobox/combobox.h (right):

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.h:205: CustomButton* text_button_;
On 2013/12/03 20:52:30, sky wrote:
> Document ownership.

Done.

[sky, 2013-12-04 16:34:04]

Also, please add unit test coverage here.

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:103: // gfx::AnimationDelegate:
On 2013/12/04 06:30:02, hajimehoshi wrote:
> On 2013/12/03 20:52:30, sky wrote:
> > Does SchedulePaint() on this always need to force a SchedulePaint on the
> parent
> > the size of the parent?
> 
> Done. (ScheudulePaintInRect with its bounds should be fine)

Why is that? Don't you need to force the whole combobox to redraw? For example,
presumably a mouse press on one of the buttons needs to redraw the whole
combobox, right?

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:323: text_border_ = new
FocusableBorder();
On 2013/12/04 06:30:02, hajimehoshi wrote:
> On 2013/12/03 20:52:30, sky wrote:
> > Why do you create a new border here?
> 
> It's because there is no way to reset the insets of the FocusableBorder. I'll
> add a comment.

FocusableBorder has a SetInsets, can't you use it? That said, since SetBorder is
public and would effectively delete text_border_ out from under you it's best to
reset. So, go with resetting but you should also remove text_border_ completely.

https://codereview.chromium.org/59383003/diff/300001/ui/views/controls/combob...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/59383003/diff/300001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:138: const int*
GetBodyButtonImageIds(bool focused, Button::ButtonState state,
nit: wrap state to its own line.

https://codereview.chromium.org/59383003/diff/300001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:158: const int*
GetMenuButtonImageIds(bool focused, Button::ButtonState state,
nit: wrap state to its own line.

https://codereview.chromium.org/59383003/diff/300001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:179: bool focused, Button::ButtonState
state) {
nit: wrap state to its own line.

https://codereview.chromium.org/59383003/diff/300001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:191: // Paints three images in a row at
the given location.
This isn't a row, but a column. Name this PaintImagesVertically and document the
center is stretched.

https://codereview.chromium.org/59383003/diff/300001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:241: text_button_(NULL),
Sorry for not noticing last time around. Why can't we always create these and
always use them? The only difference between the two modes is slight rendering
differences and keyboard handling, right?

https://codereview.chromium.org/59383003/diff/300001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:292: RemoveChildView(text_button_);
delete text_button_; delete arrow_button_; does the right thing.

You also need to set text_button/arrow_button_ to NULL.

https://codereview.chromium.org/59383003/diff/300001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:373: std::max(kMinComboboxWidth,
content_size_.width()) + insets.width();
I don't think you should use content_size_ here. It does not necessarily match
the actual bounds. Instead layout the arrow button aligned to the right side,
then give the textbutton the rest of the space. That way things work all the
time, even if less/more space is given.

https://codereview.chromium.org/59383003/diff/300001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:563: break;
I believe you need to deal with focus here.

https://codereview.chromium.org/59383003/diff/300001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:607: UpdateFromModel();
Why are you invoking UpdateFromModel here?

https://codereview.chromium.org/59383003/diff/300001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:702: canvas->Save();
Use ScopedCanvas as necessary in here.

https://codereview.chromium.org/59383003/diff/300001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:798:
arrow_button_->SetState(original_state);
nit: no {}

[hajimehoshi, 2013-12-05 08:05:56]

Thank you!

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:103: // gfx::AnimationDelegate:
On 2013/12/04 16:34:05, sky wrote:
> On 2013/12/04 06:30:02, hajimehoshi wrote:
> > On 2013/12/03 20:52:30, sky wrote:
> > > Does SchedulePaint() on this always need to force a SchedulePaint on the
> > parent
> > > the size of the parent?
> > 
> > Done. (ScheudulePaintInRect with its bounds should be fine)
> 
> Why is that? Don't you need to force the whole combobox to redraw? For
example,
> presumably a mouse press on one of the buttons needs to redraw the whole
> combobox, right?

Each button has its own animation value only for hovering, so I don't think
pressing a mouse button needs redrawing the whole combobox. You mean that
pressing one button should stop all hovering animations of two buttons on the
combobox?

https://codereview.chromium.org/59383003/diff/280001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:323: text_border_ = new
FocusableBorder();
On 2013/12/04 16:34:05, sky wrote:
> On 2013/12/04 06:30:02, hajimehoshi wrote:
> > On 2013/12/03 20:52:30, sky wrote:
> > > Why do you create a new border here?
> > 
> > It's because there is no way to reset the insets of the FocusableBorder.
I'll
> > add a comment.
> 
> FocusableBorder has a SetInsets, can't you use it? That said, since SetBorder
is
> public and would effectively delete text_border_ out from under you it's best
to
> reset. So, go with resetting but you should also remove text_border_
completely.

The default insets values of FocusableBorder is internally hidden and I should
make them public if I use SetInsets. (You were against adding a method
ResetInsets to FocusableBorder at the other review).

IIUC, the border is owned by the base view and set_border with a new border
deletes the old border, so my current change works correctly, right? If I delete
the border here explicitly, set_border will cause a double free error.

https://codereview.chromium.org/59383003/diff/300001/ui/views/controls/combob...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/59383003/diff/300001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:138: const int*
GetBodyButtonImageIds(bool focused, Button::ButtonState state,
On 2013/12/04 16:34:05, sky wrote:
> nit: wrap state to its own line.

Done.

https://codereview.chromium.org/59383003/diff/300001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:158: const int*
GetMenuButtonImageIds(bool focused, Button::ButtonState state,
On 2013/12/04 16:34:05, sky wrote:
> nit: wrap state to its own line.

Done.

https://codereview.chromium.org/59383003/diff/300001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:179: bool focused, Button::ButtonState
state) {
On 2013/12/04 16:34:05, sky wrote:
> nit: wrap state to its own line.

Done.

https://codereview.chromium.org/59383003/diff/300001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:191: // Paints three images in a row at
the given location.
On 2013/12/04 16:34:05, sky wrote:
> This isn't a row, but a column. Name this PaintImagesVertically and document
the
> center is stretched.

Done.

https://codereview.chromium.org/59383003/diff/300001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:241: text_button_(NULL),
On 2013/12/04 16:34:05, sky wrote:
> Sorry for not noticing last time around. Why can't we always create these and
> always use them? The only difference between the two modes is slight rendering
> differences and keyboard handling, right?

Right.  Done.

I guess OnMousePressed, OnMouseDragged and OnGestureEvent are no longer needed
so I'll remove them.

https://codereview.chromium.org/59383003/diff/300001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:292: RemoveChildView(text_button_);
On 2013/12/04 16:34:05, sky wrote:
> delete text_button_; delete arrow_button_; does the right thing.
> 
> You also need to set text_button/arrow_button_ to NULL.

Done.

https://codereview.chromium.org/59383003/diff/300001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:373: std::max(kMinComboboxWidth,
content_size_.width()) + insets.width();
On 2013/12/04 16:34:05, sky wrote:
> I don't think you should use content_size_ here. It does not necessarily match
> the actual bounds. Instead layout the arrow button aligned to the right side,
> then give the textbutton the rest of the space. That way things work all the
> time, even if less/more space is given.

Done.

https://codereview.chromium.org/59383003/diff/300001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:563: break;
On 2013/12/04 16:34:05, sky wrote:
> I believe you need to deal with focus here.

I intend to create another CL to add images to render the focused buttons. We
think that focus border should be put on the whole of the combobox, not on each
button, so we prepared the other images which are not like buttons. The reason
why I didn't include them is that including them make this CL too big.

https://codereview.chromium.org/59383003/diff/300001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:607: UpdateFromModel();
Sorry but this wasn't needed. Done.

https://codereview.chromium.org/59383003/diff/300001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:702: canvas->Save();
On 2013/12/04 16:34:05, sky wrote:
> Use ScopedCanvas as necessary in here.

Done.

https://codereview.chromium.org/59383003/diff/300001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:798:
arrow_button_->SetState(original_state);
On 2013/12/04 16:34:05, sky wrote:
> nit: no {}

Done.

[sky, 2013-12-05 16:46:32]

https://codereview.chromium.org/59383003/diff/330119/ui/views/controls/combob...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/59383003/diff/330119/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:306: text_border_->SetInsets(8, 13, 8,
13);
What I'm saying is you should get rid of text_border_ as a member and recreate
anytime you need to change anything.

https://codereview.chromium.org/59383003/diff/330119/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:364: int height = content_size_.height()
+ insets.height();
Shouldn't you use height() here rather than calculating a height that may not be
the actual height?

https://codereview.chromium.org/59383003/diff/330119/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:382: text_button_->SetBounds(0, 0,
text_button_width, height);
std::max(0, text_button_width)

https://codereview.chromium.org/59383003/diff/330119/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:690: int text_button_width = width() -
arrow_button_images[0]->width();
Is there a reason you need this code here rather than in the button classes? If
you do need it here add a comment why and use the bounds of the buttons rather
than recalculating.

https://codereview.chromium.org/59383003/diff/330119/ui/views/controls/combob...
File ui/views/controls/combobox/combobox_unittest.cc (right):

https://codereview.chromium.org/59383003/diff/330119/ui/views/controls/combob...
ui/views/controls/combobox/combobox_unittest.cc:103: bool
on_selected_index_changed_;
private and add accessors/setters.

https://codereview.chromium.org/59383003/diff/330119/ui/views/controls/combob...
ui/views/controls/combobox/combobox_unittest.cc:371: TEST_F(ComboboxTest,
NotifyOnClick) {
Add coverage of mouse events too.

[hajimehoshi, 2013-12-06 12:27:03]

Thank you!

https://codereview.chromium.org/59383003/diff/330119/ui/views/controls/combob...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/59383003/diff/330119/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:306: text_border_->SetInsets(8, 13, 8,
13);
On 2013/12/05 16:46:32, sky wrote:
> What I'm saying is you should get rid of text_border_ as a member and recreate
> anytime you need to change anything.

Done. (Added UpdateBorder)

https://codereview.chromium.org/59383003/diff/330119/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:364: int height = content_size_.height()
+ insets.height();
On 2013/12/05 16:46:32, sky wrote:
> Shouldn't you use height() here rather than calculating a height that may not
be
> the actual height?

height() was enough. Done.

https://codereview.chromium.org/59383003/diff/330119/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:382: text_button_->SetBounds(0, 0,
text_button_width, height);
On 2013/12/05 16:46:32, sky wrote:
> std::max(0, text_button_width)

Done.

https://codereview.chromium.org/59383003/diff/330119/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:690: int text_button_width = width() -
arrow_button_images[0]->width();
On 2013/12/05 16:46:32, sky wrote:
> Is there a reason you need this code here rather than in the button classes?
If
> you do need it here add a comment why and use the bounds of the buttons rather
> than recalculating.

Oh, I just didn't realize this... Thanks. I removed arrow_button_x as well.

https://codereview.chromium.org/59383003/diff/330119/ui/views/controls/combob...
File ui/views/controls/combobox/combobox_unittest.cc (right):

https://codereview.chromium.org/59383003/diff/330119/ui/views/controls/combob...
ui/views/controls/combobox/combobox_unittest.cc:103: bool
on_selected_index_changed_;
On 2013/12/05 16:46:32, sky wrote:
> private and add accessors/setters.

Done.

https://codereview.chromium.org/59383003/diff/330119/ui/views/controls/combob...
ui/views/controls/combobox/combobox_unittest.cc:371: TEST_F(ComboboxTest,
NotifyOnClick) {
On 2013/12/05 16:46:32, sky wrote:
> Add coverage of mouse events too.

Done. (Added a class ComboboxMenuRunner)

[sky, 2013-12-06 17:12:22]

https://codereview.chromium.org/59383003/diff/470001/ui/views/controls/combob...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/59383003/diff/470001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:122:
parent()->SchedulePaintInRect(bounds());
CustomButton::SetState does a SchedulePaint already, are you sure you need this?

https://codereview.chromium.org/59383003/diff/470001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:127:
parent()->SchedulePaintInRect(bounds());
Same thing here.

https://codereview.chromium.org/59383003/diff/470001/ui/views/controls/combob...
File ui/views/controls/combobox/combobox_menu_runner.h (right):

https://codereview.chromium.org/59383003/diff/470001/ui/views/controls/combob...
ui/views/controls/combobox/combobox_menu_runner.h:21: class ComboboxMenuRunner {
I like this, but I think it should be done at the MenuRunner level. That way we
can do a similar thing for menus.

[hajimehoshi, 2013-12-09 07:44:27]

Thank you!

https://codereview.chromium.org/59383003/diff/470001/ui/views/controls/combob...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/59383003/diff/470001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:122:
parent()->SchedulePaintInRect(bounds());
On 2013/12/06 17:12:23, sky wrote:
> CustomButton::SetState does a SchedulePaint already, are you sure you need
this?

Ah, you're right. This isn't needed. Done.

https://codereview.chromium.org/59383003/diff/470001/ui/views/controls/combob...
ui/views/controls/combobox/combobox.cc:127:
parent()->SchedulePaintInRect(bounds());
On 2013/12/06 17:12:23, sky wrote:
> Same thing here.

Done.

https://codereview.chromium.org/59383003/diff/470001/ui/views/controls/combob...
File ui/views/controls/combobox/combobox_menu_runner.h (right):

https://codereview.chromium.org/59383003/diff/470001/ui/views/controls/combob...
ui/views/controls/combobox/combobox_menu_runner.h:21: class ComboboxMenuRunner {
On 2013/12/06 17:12:23, sky wrote:
> I like this, but I think it should be done at the MenuRunner level. That way
we
> can do a similar thing for menus.

Done. (Added MenuRunnerCore to implement RunMenuAt. To be honest, I'm not
confident about the class name.)

[sky, 2013-12-09 14:54:03]

https://codereview.chromium.org/59383003/diff/520001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_runner.h (right):

https://codereview.chromium.org/59383003/diff/520001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_runner.h:116: void set_runner_core(MenuRunnerCore*
runner_core) {
Make this private and only settable by way of a test friend api, eg
MenuRunnerTestApi or MenuRunnerTestHelper. Said class is only compiled for test
code. Also, can this be a scoped_ptr?

https://codereview.chromium.org/59383003/diff/520001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_runner_core.h (right):

https://codereview.chromium.org/59383003/diff/520001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_runner_core.h:17: class VIEWS_EXPORT MenuRunnerCore
{
How about MenuRunnerImpl?

[hajimehoshi, 2013-12-09 14:58:30]

Thank you! Just a note to tell you that:

https://codereview.chromium.org/59383003/diff/520001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_runner_core.h (right):

https://codereview.chromium.org/59383003/diff/520001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_runner_core.h:17: class VIEWS_EXPORT MenuRunnerCore
{
On 2013/12/09 14:54:04, sky wrote:
> How about MenuRunnerImpl?

(internal::)MenuRunnerImpl already exists.

[sky, 2013-12-09 15:07:17]

MenuRunnerHandler?

On Mon, Dec 9, 2013 at 6:58 AM,  <hajimehoshi@chromium.org> wrote:
> Thank you! Just a note to tell you that:
>
>
>
>
https://codereview.chromium.org/59383003/diff/520001/ui/views/controls/menu/m...
> File ui/views/controls/menu/menu_runner_core.h (right):
>
>
https://codereview.chromium.org/59383003/diff/520001/ui/views/controls/menu/m...
> ui/views/controls/menu/menu_runner_core.h:17: class VIEWS_EXPORT
> MenuRunnerCore {
> On 2013/12/09 14:54:04, sky wrote:
>>
>> How about MenuRunnerImpl?
>
>
> (internal::)MenuRunnerImpl already exists.
>
> https://codereview.chromium.org/59383003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hajimehoshi, 2013-12-10 06:04:23]

Thank you!

https://codereview.chromium.org/59383003/diff/520001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_runner.h (right):

https://codereview.chromium.org/59383003/diff/520001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_runner.h:116: void set_runner_core(MenuRunnerCore*
runner_core) {
On 2013/12/09 14:54:04, sky wrote:
> Make this private and only settable by way of a test friend api, eg
> MenuRunnerTestApi or MenuRunnerTestHelper. Said class is only compiled for
test
> code. Also, can this be a scoped_ptr?

Done.

https://codereview.chromium.org/59383003/diff/520001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_runner_core.h (right):

https://codereview.chromium.org/59383003/diff/520001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_runner_core.h:17: class VIEWS_EXPORT MenuRunnerCore
{
On 2013/12/09 14:58:31, hajimehoshi wrote:
> On 2013/12/09 14:54:04, sky wrote:
> > How about MenuRunnerImpl?
> 
> (internal::)MenuRunnerImpl already exists.

Done. (MenuRunnerHandler)

[sky, 2013-12-10 16:56:05]

LGTM with the following changes.

https://codereview.chromium.org/59383003/diff/540001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_runner.h (right):

https://codereview.chromium.org/59383003/diff/540001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_runner.h:124: void
SetRunnerHandler(MenuRunnerHandler* runner_handler);
scoped_ptr

https://codereview.chromium.org/59383003/diff/540001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_runner_handler.h (right):

https://codereview.chromium.org/59383003/diff/540001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_runner_handler.h:16: // An interface which defines
RunMenuAt.
Used internally by MenuRunner to show the menu. Can be set in tests (see
MenuRunnerTestApi) for mocking running of the menu.

https://codereview.chromium.org/59383003/diff/540001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_runner_handler.h:25: virtual ~MenuRunnerHandler() {}
destructor should be first.

https://codereview.chromium.org/59383003/diff/540001/ui/views/test/menu_runne...
File ui/views/test/menu_runner_test_api.h (right):

https://codereview.chromium.org/59383003/diff/540001/ui/views/test/menu_runne...
ui/views/test/menu_runner_test_api.h:24: void
SetRunnerHandler(MenuRunnerHandler* runner_handler);
scoped_ptr, also name this SetMenuRunerHandler.

[hajimehoshi, 2013-12-11 04:07:03]

Thank you!

https://codereview.chromium.org/59383003/diff/540001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_runner.h (right):

https://codereview.chromium.org/59383003/diff/540001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_runner.h:124: void
SetRunnerHandler(MenuRunnerHandler* runner_handler);
On 2013/12/10 16:56:06, sky wrote:
> scoped_ptr

Done.

https://codereview.chromium.org/59383003/diff/540001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_runner_handler.h (right):

https://codereview.chromium.org/59383003/diff/540001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_runner_handler.h:16: // An interface which defines
RunMenuAt.
On 2013/12/10 16:56:06, sky wrote:
> Used internally by MenuRunner to show the menu. Can be set in tests (see
> MenuRunnerTestApi) for mocking running of the menu.

Done.

https://codereview.chromium.org/59383003/diff/540001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_runner_handler.h:25: virtual ~MenuRunnerHandler() {}
On 2013/12/10 16:56:06, sky wrote:
> destructor should be first.

Done.

https://codereview.chromium.org/59383003/diff/540001/ui/views/test/menu_runne...
File ui/views/test/menu_runner_test_api.h (right):

https://codereview.chromium.org/59383003/diff/540001/ui/views/test/menu_runne...
ui/views/test/menu_runner_test_api.h:24: void
SetRunnerHandler(MenuRunnerHandler* runner_handler);
On 2013/12/10 16:56:06, sky wrote:
> scoped_ptr, also name this SetMenuRunerHandler.

Done.

[commit-bot: I haz the power, 2013-12-11 04:07:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/59383003/580001

[commit-bot: I haz the power, 2013-12-11 04:08:21]

Failed to apply patch for ui/views/controls/combobox/combobox.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ui/views/controls/combobox/combobox.cc
  Hunk #1 FAILED at 12.
  Hunk #2 succeeded at 40 (offset 1 line).
  Hunk #3 succeeded at 51 (offset 1 line).
  Hunk #4 succeeded at 82 (offset 1 line).
  Hunk #5 succeeded at 114 (offset 1 line).
  Hunk #6 succeeded at 220 (offset 1 line).
  Hunk #7 succeeded at 271 (offset 1 line).
  Hunk #8 succeeded at 311 (offset 1 line).
  Hunk #9 succeeded at 323 (offset 1 line).
  Hunk #10 succeeded at 392 (offset 1 line).
  Hunk #11 succeeded at 410 (offset 1 line).
  Hunk #12 succeeded at 454 (offset 1 line).
  Hunk #13 succeeded at 485 (offset 1 line).
  Hunk #14 succeeded at 537 (offset 1 line).
  Hunk #15 succeeded at 587 (offset 1 line).
  Hunk #16 succeeded at 603 (offset 1 line).
  Hunk #17 succeeded at 619 (offset 1 line).
  Hunk #18 succeeded at 631 (offset 1 line).
  Hunk #19 succeeded at 644 (offset 1 line).
  Hunk #20 succeeded at 710 (offset 1 line).
  Hunk #21 succeeded at 724 (offset 1 line).
  Hunk #22 succeeded at 767 (offset 1 line).
  1 out of 22 hunks FAILED -- saving rejects to file
ui/views/controls/combobox/combobox.cc.rej

Patch:       ui/views/controls/combobox/combobox.cc
Index: ui/views/controls/combobox/combobox.cc
diff --git a/ui/views/controls/combobox/combobox.cc
b/ui/views/controls/combobox/combobox.cc
index
a90275329aa1912778bc8d2a8d587c189f8daaf6..7fd9e93bc5f94ca1a7bb159a7ff4f5beeced3ba2
100644
--- a/ui/views/controls/combobox/combobox.cc
+++ b/ui/views/controls/combobox/combobox.cc
@@ -12,16 +12,23 @@
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/events/event.h"
 #include "ui/events/keycodes/keyboard_codes.h"
+#include "ui/gfx/animation/throb_animation.h"
 #include "ui/gfx/canvas.h"
+#include "ui/gfx/image/image.h"
+#include "ui/gfx/scoped_canvas.h"
 #include "ui/native_theme/native_theme.h"
 #include "ui/views/color_constants.h"
+#include "ui/views/controls/button/custom_button.h"
+#include "ui/views/controls/button/label_button.h"
 #include "ui/views/controls/combobox/combobox_listener.h"
 #include "ui/views/controls/focusable_border.h"
 #include "ui/views/controls/menu/menu_runner.h"
+#include "ui/views/controls/menu/menu_runner_handler.h"
 #include "ui/views/controls/menu/submenu_view.h"
 #include "ui/views/controls/prefix_selector.h"
 #include "ui/views/ime/input_method.h"
 #include "ui/views/mouse_constants.h"
+#include "ui/views/painter.h"
 #include "ui/views/widget/widget.h"
 
 namespace views {
@@ -39,6 +46,8 @@ const int kMinComboboxWidth = 25;
 // Size of the combobox arrow margins
 const int kDisclosureArrowLeftPadding = 7;
 const int kDisclosureArrowRightPadding = 7;
+const int kDisclosureArrowButtonLeftPadding = 11;
+const int kDisclosureArrowButtonRightPadding = 12;
 
 // Define the id of the first item in the menu (since it needs to be > 0)
 const int kFirstMenuItemId = 1000;
@@ -48,6 +57,19 @@ const SkColor kInvalidTextColor = SK_ColorWHITE;
 // Used to indicate that no item is currently selected by the user.
 const int kNoSelection = -1;
 
+const int kBodyButtonImages[] = IMAGE_GRID(IDR_COMBOBOX_BUTTON);
+const int kHoveredBodyButtonImages[] = IMAGE_GRID(IDR_COMBOBOX_BUTTON_H);
+const int kPressedBodyButtonImages[] = IMAGE_GRID(IDR_COMBOBOX_BUTTON_P);
+
+#define MENU_IMAGE_GRID(x) { \
+    x ## _MENU_TOP, x ## _MENU_CENTER, x ## _MENU_BOTTOM, }
+
+const int kMenuButtonImages[] = MENU_IMAGE_GRID(IDR_COMBOBOX_BUTTON);
+const int kHoveredMenuButtonImages[] = MENU_IMAGE_GRID(IDR_COMBOBOX_BUTTON_H);
+const int kPressedMenuButtonImages[] = MENU_IMAGE_GRID(IDR_COMBOBOX_BUTTON_P);
+
+#undef MENU_IMAGE_GRID
+
 // The background to use for invalid comboboxes.
 class InvalidBackground : public Background {
  public:
@@ -66,6 +88,23 @@ class InvalidBackground : public Background {
   DISALLOW_COPY_AND_ASSIGN(InvalidBackground);
 };
 
+// The transparent button which holds a button state but is not rendered.
+class TransparentButton : public CustomButton {
+ public:
+  TransparentButton(ButtonListener* listener)
+      : CustomButton(listener) {
+    SetAnimationDuration(LabelButton::kHoverAnimationDurationMs);
+  }
+  virtual ~TransparentButton() {}
+
+  double GetAnimationValue() const {
+    return hover_animation_->GetCurrentValue();
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TransparentButton);
+};
+
 // Returns the next or previous valid index (depending on |increment|'s value).
 // Skips separator or disabled indices. Returns -1 if there is no valid
adjacent
 // index.
@@ -81,6 +120,102 @@ int GetAdjacentIndex(ui::ComboboxModel* model, int
increment, int index) {
   return kNoSelection;
 }
 
+// Returns the image resource ids of an array for the body button.
+//
+// TODO(hajimehoshi): This function should return the images for the 'disabled'
+// status. (crbug/270052)
+//
+// TODO(hajimehoshi): Currently, |focused| is ignored. This should return the
+// images for the 'focused' status. (crbug/270052)
+const int* GetBodyButtonImageIds(bool focused,
+                                 Button::ButtonState state,
+                                 size_t* num) {
+  DCHECK(num);
+  *num = 9;
+  switch (state) {
+    case Button::STATE_DISABLED:
+      return kBodyButtonImages;
+    case Button::STATE_NORMAL:
+      return kBodyButtonImages;
+    case Button::STATE_HOVERED:
+      return kHoveredBodyButtonImages;
+    case Button::STATE_PRESSED:
+      return kPressedBodyButtonImages;
+    default:
+      NOTREACHED();
+  }
+  return NULL;
+}
+
+// Returns the image resource ids of an array for the menu button.
+const int* GetMenuButtonImageIds(bool focused,
+                                 Button::ButtonState state,
+                                 size_t* num) {
+  DCHECK(num);
+  *num = 3;
+  switch (state) {
+    case Button::STATE_DISABLED:
+      return kMenuButtonImages;
+    case Button::STATE_NORMAL:
+      return kMenuButtonImages;
+    case Button::STATE_HOVERED:
+      return kHoveredMenuButtonImages;
+    case Button::STATE_PRESSED:
+      return kPressedMenuButtonImages;
+    default:
+      NOTREACHED();
+  }
+  return NULL;
+}
+
+// Returns the images for the menu buttons.
+std::vector<const gfx::ImageSkia*> GetMenuButtonImages(
+    bool focused,
+    Button::ButtonState state) {
+  const int* ids;
+  size_t num_ids;
+  ids = GetMenuButtonImageIds(focused, state, &num_ids);
+  std::vector<const gfx::ImageSkia*> images;
+  images.reserve(num_ids);
+  ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
+  for (size_t i = 0; i < num_ids; i++)
+    images.push_back(rb.GetImageSkiaNamed(ids[i]));
+  return images;
+}
+
+// Paints three images in a column at the given location. The center image is
+// stretched so as to fit the given height.
+void PaintImagesVertically(gfx::Canvas* canvas,
+                           const gfx::ImageSkia& top_image,
+                           const gfx::ImageSkia& center_image,
+                           const gfx::ImageSkia& bottom_image,
+                           int x, int y, int width, int height) {
+  canvas->DrawImageInt(top_image,
+                       0, 0, top_image.width(), top_image.height(),
+                       x, y, width, top_image.height(), false);
+  y += top_image.height();
+  int center_height = height - top_image.height() - bottom_image.height();
+  canvas->DrawImageInt(center_image,
+                       0, 0, center_image.width(), center_image.height(),
+                       x, y, width, center_height, false);
+  y += center_height;
+  canvas->DrawImageInt(bottom_image,
+                       0, 0, bottom_image.width(), bottom_image.height(),
+                       x, y, width, bottom_image.height(), false);
+}
+
+// Paints the arrow button.
+void PaintArrowButton(
+    gfx::Canvas* canvas,
+    const std::vector<const gfx::ImageSkia*>& arrow_button_images,
+    int x, int height) {
+  PaintImagesVertically(canvas,
+                        *arrow_button_images[0],
+                        *arrow_button_images[1],
+                        *arrow_button_images[2],
+                        x, 0, arrow_button_images[0]->width(), height);
+}
+
 }  // namespace
 
 // static
@@ -91,17 +226,45 @@ const char Combobox::kViewClassName[] = "views/Combobox";
 
 Combobox::Combobox(ui::ComboboxModel* model)
     : model_(model),
+      style_(STYLE_SHOW_DROP_DOWN_ON_CLICK),
       listener_(NULL),
       selected_index_(model_->GetDefaultIndex()),
       invalid_(false),
-      text_border_(new FocusableBorder()),
       disclosure_arrow_(ui::ResourceBundle::GetSharedInstance().GetImageNamed(
           IDR_MENU_DROPARROW).ToImageSkia()),
-      dropdown_open_(false) {
+      dropdown_open_(false),
+      text_button_(new TransparentButton(this)),
+      arrow_button_(new TransparentButton(this)) {
   model_->AddObserver(this);
   UpdateFromModel();
   set_focusable(true);
-  set_border(text_border_);
+  UpdateBorder();
+
+  // Initialize the button images.
+  Button::ButtonState button_states[] = {
+    Button::STATE_DISABLED,
+    Button::STATE_NORMAL,
+    Button::STATE_HOVERED,
+    Button::STATE_PRESSED,
+  };
+  for (int focused = 0; focused < 2; focused++) {
+    for (size_t state_index = 0; state_index < arraysize(button_states);
+         state_index++) {
+      Button::ButtonState state = button_states[state_index];
+      size_t num;
+      const int* ids = GetBodyButtonImageIds(focused, state, &num);
+      body_button_painters_[focused][state].reset(
+          Painter::CreateImageGridPainter(ids));
+      menu_button_images_[focused][state] = GetMenuButtonImages(focused,
state);
+    }
+  }
+
+  text_button_->SetVisible(true);
+  arrow_button_->SetVisible(true);
+  text_button_->set_focusable(false);
+  arrow_button_->set_focusable(false);
+  AddChildView(text_button_);
+  AddChildView(arrow_button_);
 }
 
 Combobox::~Combobox() {
@@ -114,6 +277,16 @@ const gf…
(message too large)

[commit-bot: I haz the power, 2013-12-11 04:35:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/59383003/600001

[commit-bot: I haz the power, 2013-12-11 04:59:20]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-11 05:39:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/59383003/620001

[commit-bot: I haz the power, 2013-12-11 09:03:32]

Change committed as 240057