Combobox: Rename styles to STYLE_NORMAL and STYLE_ACTION and modify behaviors

Combobox: Rename styles to STYLE_NORMAL and STYLE_ACTION and modify behaviors

This patch makes the combobox with STYLE_ACTION have the width same as the first item, whose index is 0.

When SYTLE_ACTION is used, the selected index remain 0 always. Besides, keyboard events don't change the index. Only the click event changes the index, and the index will be set 0 back after handling the event.

BUG=333210
TEST=views_unittests --gtest_filter=ComboboxTest.*

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

[hajimehoshi, 2014-01-17 11:23:01]

Can you take a look?  Thank you in advance.

(I said I would add a new style, but I changed my mind.)

[sky, 2014-01-17 16:01:54]

https://codereview.chromium.org/141523005/diff/1/ui/views/controls/combobox/c...
File ui/views/controls/combobox/combobox.h (right):

https://codereview.chromium.org/141523005/diff/1/ui/views/controls/combobox/c...
ui/views/controls/combobox/combobox.h:164: // STYLE_NOTIFY_ON_CLICK, this is
ignored.
AFAIK you are the only using STYLE_NOTIFY_ON_CLICK, so make the behavior you
want the default.

[hajimehoshi, 2014-01-20 03:06:14]

Thanks! PTAL

https://codereview.chromium.org/141523005/diff/1/ui/views/controls/combobox/c...
File ui/views/controls/combobox/combobox.h (right):

https://codereview.chromium.org/141523005/diff/1/ui/views/controls/combobox/c...
ui/views/controls/combobox/combobox.h:164: // STYLE_NOTIFY_ON_CLICK, this is
ignored.
On 2014/01/17 16:01:55, sky wrote:
> AFAIK you are the only using STYLE_NOTIFY_ON_CLICK, so make the behavior you
> want the default.

Done.

[sky, 2014-01-21 15:47:24]

Don't you need to update the docs? And how about test coverage?

[hajimehoshi, 2014-01-22 05:00:00]

Sure. PTAL

[sky, 2014-01-22 16:04:07]

https://codereview.chromium.org/141523005/diff/110001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/141523005/diff/110001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:592: if (style_ != STYLE_NOTIFY_ON_CLICK
|| i == 0)
I don't understand this part of the change. Shouldn't the width always be the
max of all the content? Otherwise if the selected index changes the text is
going to be clipped.

https://codereview.chromium.org/141523005/diff/110001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.h (right):

https://codereview.chromium.org/141523005/diff/110001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.h:42: // (STYLE_NOTIFY_ON_CLICK). When using
STYLE_NOTIFY_ON_CLICK, the listener is
Sorry, this comment doesn't really help. Nuke it.

[hajimehoshi, 2014-01-22 16:22:47]

Please read crbug/333210. The UI team wants to change the behavior of a combobox
with STYLE_NOTIFY_ON_CLICK so that the combobox has the width calculated by the
first item and clicking a menu item raises the notify event immediately.
Clicking a menu item doesn't change the content of the combobox. I was not sure
this is an acceptable behavior as a combobox, but tried to implement this.

[sky, 2014-01-22 20:15:36]

Ok, please update description for the style indicating that it makes the width
only that of the first item. LGTM

[sky, 2014-01-23 01:11:22]

https://codereview.chromium.org/141523005/diff/110001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/141523005/diff/110001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:592: if (style_ != STYLE_NOTIFY_ON_CLICK
|| i == 0)
On 2014/01/22 16:04:07, sky wrote:
> I don't understand this part of the change. Shouldn't the width always be the
> max of all the content? Otherwise if the selected index changes the text is
> going to be clipped.

Sorry, one question here. If you do this and the selected index changes, then
isn't there a very high liklihood of the text being clipped? I'm wondering if
for this style ExecuteCommand shouldn't change the selected_index_? Or maybe
reset it after notifying listener?

[hajimehoshi, 2014-01-23 02:16:32]

https://codereview.chromium.org/141523005/diff/110001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/141523005/diff/110001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:592: if (style_ != STYLE_NOTIFY_ON_CLICK
|| i == 0)
On 2014/01/23 01:11:23, sky wrote:
> On 2014/01/22 16:04:07, sky wrote:
> > I don't understand this part of the change. Shouldn't the width always be
the
> > max of all the content? Otherwise if the selected index changes the text is
> > going to be clipped.
> 
> Sorry, one question here. If you do this and the selected index changes, then
> isn't there a very high liklihood of the text being clipped? I'm wondering if
> for this style ExecuteCommand shouldn't change the selected_index_? Or maybe
> reset it after notifying listener?

For the current usage of Translate bubble, the bubble will be closed at the same
time the command is executed, so I didn't care about how the content should be
changed. So, I think ExecuteCommand shouldn't change the selected_index_.

[hajimehoshi, 2014-01-23 04:47:37]

On 2014/01/23 02:16:32, hajimehoshi wrote:
>
https://codereview.chromium.org/141523005/diff/110001/ui/views/controls/combo...
> File ui/views/controls/combobox/combobox.cc (right):
> 
>
https://codereview.chromium.org/141523005/diff/110001/ui/views/controls/combo...
> ui/views/controls/combobox/combobox.cc:592: if (style_ !=
STYLE_NOTIFY_ON_CLICK
> || i == 0)
> On 2014/01/23 01:11:23, sky wrote:
> > On 2014/01/22 16:04:07, sky wrote:
> > > I don't understand this part of the change. Shouldn't the width always be
> the
> > > max of all the content? Otherwise if the selected index changes the text
is
> > > going to be clipped.
> > 
> > Sorry, one question here. If you do this and the selected index changes,
then
> > isn't there a very high liklihood of the text being clipped? I'm wondering
if
> > for this style ExecuteCommand shouldn't change the selected_index_? Or maybe
> > reset it after notifying listener?
> 
> For the current usage of Translate bubble, the bubble will be closed at the
same
> time the command is executed, so I didn't care about how the content should be
> changed. So, I think ExecuteCommand shouldn't change the selected_index_.

Sorry, ComboboxListener::OnComboboxTextButtonClicked uses the selected index, so
the index should be changed. I found a problem that pressing down key changed
the content with the clipped text.
* Modified PaintText to use only the first item whatever the current selected
index is.
* Modified HandleEvent to have the event source. If the click event is not
raised from a menu item (like keyboard or mouse), the selected_index_ is set to
0 before notifying to the listener.
PTAL

[sky, 2014-01-23 15:40:25]

https://codereview.chromium.org/141523005/diff/210001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/141523005/diff/210001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:592: if (style_ != STYLE_NOTIFY_ON_CLICK
|| i == 0)
Based on your description I think this should use selected_index_ and not 0.

https://codereview.chromium.org/141523005/diff/210001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:629: if (style_ ==
STYLE_SHOW_DROP_DOWN_ON_CLICK)
This shouldn't be needed. If a consumer wants to force the selected index to 0,
it should do that, not combobox.

https://codereview.chromium.org/141523005/diff/210001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:803: if (source !=
CLICK_EVENT_SOURCE_MENU_ITEM)
Same comment as above. Your code should change selection, not combobox.

[hajimehoshi, 2014-01-24 04:10:55]

Thank you! To prevent the keyboard events from changing the item, I modified
OnKeyPressed, but as the other parts are doing, should this be controlled by the
consumer?

https://codereview.chromium.org/141523005/diff/210001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/141523005/diff/210001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:592: if (style_ != STYLE_NOTIFY_ON_CLICK
|| i == 0)
On 2014/01/23 15:40:25, sky wrote:
> Based on your description I think this should use selected_index_ and not 0.

Done.

https://codereview.chromium.org/141523005/diff/210001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:629: if (style_ ==
STYLE_SHOW_DROP_DOWN_ON_CLICK)
On 2014/01/23 15:40:25, sky wrote:
> This shouldn't be needed. If a consumer wants to force the selected index to
0,
> it should do that, not combobox.

Done.

https://codereview.chromium.org/141523005/diff/210001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:803: if (source !=
CLICK_EVENT_SOURCE_MENU_ITEM)
On 2014/01/23 15:40:25, sky wrote:
> Same comment as above. Your code should change selection, not combobox.

Done.

[sky, 2014-01-24 16:43:09]

LGTM

[sky, 2014-01-24 17:05:39]

https://codereview.chromium.org/141523005/diff/290002/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.h (right):

https://codereview.chromium.org/141523005/diff/290002/ui/views/controls/combo...
ui/views/controls/combobox/combobox.h:39: // Combobox has two distinct parts,
the drop down arrow and the text. When the
I think you've convinced me that for the style you care about, the selected
index should remain 0 always. So, go back to your earlier patch where you had
this logic and update this second sentence to something like: Combobox offers
two distinct behaviors:
. STYLE_NORMAL: typical combobox, clicking on the text and/or button shows the
drop down, arrow keys change selection, selected index can be changed by the
user to something other than the first item.
. STYLE_ACTION: clicking on the text notifies the listener. The menu can be
shown only by clicking on the arrow. The selected index is always reverted to 0
after the listener is notified.

I'm not too thrilled with STYLE_ACTION, but it's the best I can think of.

[hajimehoshi, 2014-01-24 18:07:09]

https://codereview.chromium.org/141523005/diff/290002/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.h (right):

https://codereview.chromium.org/141523005/diff/290002/ui/views/controls/combo...
ui/views/controls/combobox/combobox.h:39: // Combobox has two distinct parts,
the drop down arrow and the text. When the
On 2014/01/24 17:05:39, sky wrote:
> I think you've convinced me that for the style you care about, the selected
> index should remain 0 always. So, go back to your earlier patch where you had
> this logic and update this second sentence to something like: Combobox offers
> two distinct behaviors:
> . STYLE_NORMAL: typical combobox, clicking on the text and/or button shows the
> drop down, arrow keys change selection, selected index can be changed by the
> user to something other than the first item.
> . STYLE_ACTION: clicking on the text notifies the listener. The menu can be
> shown only by clicking on the arrow. The selected index is always reverted to
0
> after the listener is notified.
> 
> I'm not too thrilled with STYLE_ACTION, but it's the best I can think of.

Patch #3: The selected index is not cared.
Patch #4: The first item is always rendered whichever the selected index is.
Before notifying the click event, the selected index is set to 0 if the event is
fired from a keyboard.
Patch #5: The selected index can be changed by selecting a menu item or calling
SetSelectedIndex from the consumer. The width is based on the selected index.
Keyboard events doesn't change the index.

Hmm, all of them doesn't seem to be the logic that the selected index remain 0
always. Anyway, I'll fix this patch to keep the selected index 0, basing on
patch #5. Thanks.

[hajimehoshi, 2014-01-28 04:40:14]

On 2014/01/24 18:07:09, hajimehoshi wrote:
>
https://codereview.chromium.org/141523005/diff/290002/ui/views/controls/combo...
> File ui/views/controls/combobox/combobox.h (right):
> 
>
https://codereview.chromium.org/141523005/diff/290002/ui/views/controls/combo...
> ui/views/controls/combobox/combobox.h:39: // Combobox has two distinct parts,
> the drop down arrow and the text. When the
> On 2014/01/24 17:05:39, sky wrote:
> > I think you've convinced me that for the style you care about, the selected
> > index should remain 0 always. So, go back to your earlier patch where you
had
> > this logic and update this second sentence to something like: Combobox
offers
> > two distinct behaviors:
> > . STYLE_NORMAL: typical combobox, clicking on the text and/or button shows
the
> > drop down, arrow keys change selection, selected index can be changed by the
> > user to something other than the first item.
> > . STYLE_ACTION: clicking on the text notifies the listener. The menu can be
> > shown only by clicking on the arrow. The selected index is always reverted
to
> 0
> > after the listener is notified.
> > 
> > I'm not too thrilled with STYLE_ACTION, but it's the best I can think of.
> 
> Patch #3: The selected index is not cared.
> Patch #4: The first item is always rendered whichever the selected index is.
> Before notifying the click event, the selected index is set to 0 if the event
is
> fired from a keyboard.
> Patch #5: The selected index can be changed by selecting a menu item or
calling
> SetSelectedIndex from the consumer. The width is based on the selected index.
> Keyboard events doesn't change the index.
> 
> Hmm, all of them doesn't seem to be the logic that the selected index remain 0
> always. Anyway, I'll fix this patch to keep the selected index 0, basing on
> patch #5. Thanks.

Let me get this straight. There were two similar ideas to implement the action
combobox:
1. Keep |selected_index_| 0 and render the selected item.
2. Render the first (0th) item whichever |selected_index_| is.

Finally, we have chosen 1. Now SetSelectedIndex and SelectValue don't change the
index. I added the test for them. Sorry to keep bothering you. PTAL

[sky, 2014-01-28 14:57:13]

https://codereview.chromium.org/141523005/diff/630001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/141523005/diff/630001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:370: if (style_ == STYLE_ACTION)
Isn't the selectionchanged enough here? By that I mean why do we need the
OnTextButtonClicked too? Seems like this should just notify the selection
changed, then reset selection. Also, I think you have to handle the case of the
selection changed deleting this.

https://codereview.chromium.org/141523005/diff/630001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:813: selected_index_ = 0;
Same worry about the possibility of this being deleted.

[hajimehoshi, 2014-01-29 05:28:37]

https://codereview.chromium.org/141523005/diff/630001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/141523005/diff/630001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:370: if (style_ == STYLE_ACTION)
On 2014/01/28 14:57:14, sky wrote:
> Isn't the selectionchanged enough here? By that I mean why do we need the
> OnTextButtonClicked too? Seems like this should just notify the selection
> changed, then reset selection. 

How about pressing enter or space key? In this case, SelectionChanged should be
called with 0? I don't think this is natural...

> Also, I think you have to handle the case of the
> selection changed deleting this.

Oh, right. Maybe this problem is similar to
https://codereview.chromium.org/104573003? Regarding the possibility that the
consumer deletes the combobox during handling the click event, it seems to be
impossible to revert the selected index to 0.

How about adding an argument to OnComboboxTextButtonClicked, which indicates the
action index the user wants to cause? |selected_index_| wouldn't be changed and
remain 0 in this case.

[sky, 2014-01-29 17:16:56]

https://codereview.chromium.org/141523005/diff/630001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/141523005/diff/630001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:370: if (style_ == STYLE_ACTION)
On 2014/01/29 05:28:38, hajimehoshi wrote:
> On 2014/01/28 14:57:14, sky wrote:
> > Isn't the selectionchanged enough here? By that I mean why do we need the
> > OnTextButtonClicked too? Seems like this should just notify the selection
> > changed, then reset selection. 
> 
> How about pressing enter or space key? In this case, SelectionChanged should
be
> called with 0? I don't think this is natural...

If the menu is open and you press space/enter SelectionChanged is invoked. So it
doesn't seem that unreasonable to me to send it for the cases you care about
too.

> > Also, I think you have to handle the case of the
> > selection changed deleting this.
> 
> Oh, right. Maybe this problem is similar to
> https://codereview.chromium.org/104573003? Regarding the possibility that the
> consumer deletes the combobox during handling the click event, it seems to be
> impossible to revert the selected index to 0.

It just means you need a weak ref.

> How about adding an argument to OnComboboxTextButtonClicked, which indicates
the
> action index the user wants to cause? |selected_index_| wouldn't be changed
and
> remain 0 in this case.

Actually, I think you should nuke OnComboboxTextButtonClicked and only send
SelectionChanged. Perhaps renaming SelectionChanged to PerformAction with the
index.

[hajimehoshi, 2014-01-29 21:32:28]

https://codereview.chromium.org/141523005/diff/630001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/141523005/diff/630001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:370: if (style_ == STYLE_ACTION)
On 2014/01/29 17:16:56, sky wrote:
> On 2014/01/29 05:28:38, hajimehoshi wrote:
> > On 2014/01/28 14:57:14, sky wrote:
> > > Isn't the selectionchanged enough here? By that I mean why do we need the
> > > OnTextButtonClicked too? Seems like this should just notify the selection
> > > changed, then reset selection. 
> > 
> > How about pressing enter or space key? In this case, SelectionChanged should
> be
> > called with 0? I don't think this is natural...
> 
> If the menu is open and you press space/enter SelectionChanged is invoked. So
it
> doesn't seem that unreasonable to me to send it for the cases you care about
> too.

I mean that if the action combobox is focused and enter or space is pressed
without opening the menu, the first action will be fired, but I'm not sure
SelectionChanged with |selected_index_| 0 is appropriate in that case.

[sky, 2014-01-29 21:34:53]

That's why I suggested renaming SelectionChanged.

  -Scott

On Wed, Jan 29, 2014 at 1:32 PM,  <hajimehoshi@chromium.org> wrote:
>
>
https://codereview.chromium.org/141523005/diff/630001/ui/views/controls/combo...
> File ui/views/controls/combobox/combobox.cc (right):
>
>
https://codereview.chromium.org/141523005/diff/630001/ui/views/controls/combo...
> ui/views/controls/combobox/combobox.cc:370: if (style_ == STYLE_ACTION)
> On 2014/01/29 17:16:56, sky wrote:
>>
>> On 2014/01/29 05:28:38, hajimehoshi wrote:
>> > On 2014/01/28 14:57:14, sky wrote:
>> > > Isn't the selectionchanged enough here? By that I mean why do we
>
> need the
>>
>> > > OnTextButtonClicked too? Seems like this should just notify the
>
> selection
>>
>> > > changed, then reset selection.
>> >
>> > How about pressing enter or space key? In this case,
>
> SelectionChanged should
>>
>> be
>> > called with 0? I don't think this is natural...
>
>
>> If the menu is open and you press space/enter SelectionChanged is
>
> invoked. So it
>>
>> doesn't seem that unreasonable to me to send it for the cases you care
>
> about
>>
>> too.
>
>
> I mean that if the action combobox is focused and enter or space is
> pressed without opening the menu, the first action will be fired, but
> I'm not sure SelectionChanged with |selected_index_| 0 is appropriate in
> that case.
>
> https://codereview.chromium.org/141523005/

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

[hajimehoshi, 2014-01-30 03:10:23]

+kenjibaheux (CC)

> That's why I suggested renaming SelectionChanged.

Got it, thanks.

[hajimehoshi, 2014-01-30 08:43:30]

PTAL

[sky, 2014-02-03 16:58:18]

https://codereview.chromium.org/141523005/diff/690001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/141523005/diff/690001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:495: if (style_ != STYLE_ACTION) {
nit: combine with if on 494.

https://codereview.chromium.org/141523005/diff/690001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:777:
base::Bind(&Combobox::NotifyPerformAction,
You should not delay the notification. Instead before calling listener grab
reference from the weak factory. After calling the listener if the reference
points to NULL, you know Combobox has been destroyed.

https://codereview.chromium.org/141523005/diff/690001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox_listener.h (right):

https://codereview.chromium.org/141523005/diff/690001/ui/views/controls/combo...
ui/views/controls/combobox/combobox_listener.h:14: // An interface implemented
by an object to let it know that the selected index
Interface used to notify consumers when something interesting happens to a
Combobox.

https://codereview.chromium.org/141523005/diff/690001/ui/views/controls/combo...
ui/views/controls/combobox/combobox_listener.h:18: // Handles events when the
selected index is changed (STYLE_NORMAL) or the
Invoked when the user does the appropriate gesture that some action should be
performed. For both STYLE_NORMAL and STYLE_ACTION this is invoked if the user
clicks on the menu button and then clicks an item. For STYLE_NORMAL this is also
invoked when the menu is not showing and the does a gesture to change the
selection (for example, presses the home or end keys).
This is not invoked when the menu is shown and the user changes the selection
without closing the menu.

https://codereview.chromium.org/141523005/diff/690001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox_unittest.cc (right):

https://codereview.chromium.org/141523005/diff/690001/ui/views/controls/combo...
ui/views/controls/combobox/combobox_unittest.cc:492:
base::MessageLoop::current()->RunUntilIdle();
This shouldn't be necessary any where in this file.

[hajimehoshi, 2014-02-04 04:06:32]

Thank you!

https://codereview.chromium.org/141523005/diff/690001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/141523005/diff/690001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:495: if (style_ != STYLE_ACTION) {
On 2014/02/03 16:58:19, sky wrote:
> nit: combine with if on 494.

Done.

https://codereview.chromium.org/141523005/diff/690001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:777:
base::Bind(&Combobox::NotifyPerformAction,
On 2014/02/03 16:58:19, sky wrote:
> You should not delay the notification. Instead before calling listener grab
> reference from the weak factory. After calling the listener if the reference
> points to NULL, you know Combobox has been destroyed.

Done.

https://codereview.chromium.org/141523005/diff/690001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox_listener.h (right):

https://codereview.chromium.org/141523005/diff/690001/ui/views/controls/combo...
ui/views/controls/combobox/combobox_listener.h:14: // An interface implemented
by an object to let it know that the selected index
On 2014/02/03 16:58:19, sky wrote:
> Interface used to notify consumers when something interesting happens to a
> Combobox.

Done.

https://codereview.chromium.org/141523005/diff/690001/ui/views/controls/combo...
ui/views/controls/combobox/combobox_listener.h:18: // Handles events when the
selected index is changed (STYLE_NORMAL) or the
On 2014/02/03 16:58:19, sky wrote:
> Invoked when the user does the appropriate gesture that some action should be
> performed. For both STYLE_NORMAL and STYLE_ACTION this is invoked if the user
> clicks on the menu button and then clicks an item. For STYLE_NORMAL this is
also
> invoked when the menu is not showing and the does a gesture to change the
> selection (for example, presses the home or end keys).
> This is not invoked when the menu is shown and the user changes the selection
> without closing the menu.

Done.

https://codereview.chromium.org/141523005/diff/690001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox_unittest.cc (right):

https://codereview.chromium.org/141523005/diff/690001/ui/views/controls/combo...
ui/views/controls/combobox/combobox_unittest.cc:492:
base::MessageLoop::current()->RunUntilIdle();
On 2014/02/03 16:58:19, sky wrote:
> This shouldn't be necessary any where in this file.

Done.

[sky, 2014-02-04 15:02:32]

LGTM

[hajimehoshi, 2014-02-04 15:23:27]

The CQ bit was checked by hajimehoshi@chromium.org

[commit-bot: I haz the power, 2014-02-04 15:23:54]

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

[commit-bot: I haz the power, 2014-02-04 15:48:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-04 15:48:12]

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&...

[skia-commit-bot, 2014-02-04 15:48:16]

CQ bit was unchecked on CL. Ignoring.

[hajimehoshi, 2014-02-05 02:23:52]

The CQ bit was checked by hajimehoshi@chromium.org

[commit-bot: I haz the power, 2014-02-05 03:46:20]

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

[commit-bot: I haz the power, 2014-02-05 06:14:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-05 06:14:17]

Retried try job too often on win_x64_rel for step(s) base_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...

[hajimehoshi, 2014-02-05 06:38:52]

The CQ bit was checked by hajimehoshi@chromium.org

[commit-bot: I haz the power, 2014-02-05 06:44:15]

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

[commit-bot: I haz the power, 2014-02-05 08:50:15]

Change committed as 248931