MacViews: Tweak ComboboxTest.GetTextForRowTest to get it to pass.

MacViews: Tweak ComboboxTest.GetTextForRowTest to get it to pass.

In this test, index 0 is set as separator and TextComboboxModel::GetItemAt
asserts that separator items are not reached.
However, the combobox's selected index is 0 (default value) and on the Mac
OnPaint() will be triggered to cause the NOTREACHED to trigger.
TestComboboxModel is now initialized with the separators instead of being 
mutated after, and GetDefaultIndex is implemented to return the first
non-separator index.

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

[msw, 2014-07-07 23:37:22]

https://codereview.chromium.org/376703002/diff/1/ui/views/controls/combobox/c...
File ui/views/controls/combobox/combobox_unittest.cc (left):

https://codereview.chromium.org/376703002/diff/1/ui/views/controls/combobox/c...
ui/views/controls/combobox/combobox_unittest.cc:434: separators.insert(0);
I think it was our intention here to ensure that a 0-index separator didn't
Combobox problems. Rather than revising the test, perhaps we should revise the
Mac-specific code that causes this to fail on that platform? Or perhaps we want
to revise the cross-platform handling of separators in GetItemAt (or the
selected index initialization).

[Andre, 2014-07-08 00:37:19]

On 2014/07/07 23:37:22, msw wrote:
>
https://codereview.chromium.org/376703002/diff/1/ui/views/controls/combobox/c...
> File ui/views/controls/combobox/combobox_unittest.cc (left):
> 
>
https://codereview.chromium.org/376703002/diff/1/ui/views/controls/combobox/c...
> ui/views/controls/combobox/combobox_unittest.cc:434: separators.insert(0);
> I think it was our intention here to ensure that a 0-index separator didn't
> Combobox problems. Rather than revising the test, perhaps we should revise the
> Mac-specific code that causes this to fail on that platform? Or perhaps we
want
> to revise the cross-platform handling of separators in GetItemAt (or the
> selected index initialization).

Interestingly, this test no longer fails after I rebase to master.
Turns out it's http://crrev.com/357933002 where the NSWindow is now created with
defer:YES.

[Andre, 2014-07-08 00:37:57]

On 2014/07/08 00:37:19, Andre wrote:
> On 2014/07/07 23:37:22, msw wrote:
> >
>
https://codereview.chromium.org/376703002/diff/1/ui/views/controls/combobox/c...
> > File ui/views/controls/combobox/combobox_unittest.cc (left):
> > 
> >
>
https://codereview.chromium.org/376703002/diff/1/ui/views/controls/combobox/c...
> > ui/views/controls/combobox/combobox_unittest.cc:434: separators.insert(0);
> > I think it was our intention here to ensure that a 0-index separator didn't
> > Combobox problems. Rather than revising the test, perhaps we should revise
the
> > Mac-specific code that causes this to fail on that platform? Or perhaps we
> want
> > to revise the cross-platform handling of separators in GetItemAt (or the
> > selected index initialization).
> 
> Interestingly, this test no longer fails after I rebase to master.
> Turns out it's http://crrev.com/357933002 where the NSWindow is now created
with
> defer:YES.

With defer:YES, paint no longer triggers for this test.

[tapted, 2014-07-08 00:47:29]

Does adding widget_->Show() (e.g. at the start of ComboboxTest::TearDown())
cause the test to fail on other platforms? Maybe Combobox should DCHECK if a
separator is added at index 0, or automatically move the default selection down?

[Andre, 2014-07-08 20:05:49]

I've changed it so that TestComboboxModel is initialized with the separators
instead of being mutated after, and implemented GetDefaultIndex to return
the first non-separator index.

[msw, 2014-07-08 20:25:03]

Fantastic! lgtm

[Andre, 2014-07-08 20:45:56]

sadrul, can you review as owner?

[sadrul, 2014-07-10 15:24:36]

LGTM

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

https://codereview.chromium.org/376703002/diff/100001/ui/views/controls/combo...
ui/views/controls/combobox/combobox_unittest.cc:215: void InitCombobox(const
std::set<int>* separators = NULL) {
I believe default parameters are not allowed. Just send NULL from the rest of
the calls.

[Andre, 2014-07-10 17:44:27]

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

https://codereview.chromium.org/376703002/diff/100001/ui/views/controls/combo...
ui/views/controls/combobox/combobox_unittest.cc:215: void InitCombobox(const
std::set<int>* separators = NULL) {
On 2014/07/10 15:24:35, sadrul wrote:
> I believe default parameters are not allowed. Just send NULL from the rest of
> the calls.

Done.

[Andre, 2014-07-10 17:44:33]

The CQ bit was checked by andresantoso@chromium.org

[commit-bot: I haz the power, 2014-07-10 17:45:38]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/andresantoso@chromium.org/376703002/12...

[commit-bot: I haz the power, 2014-07-10 19:54:42]

Change committed as 282405