Make notifier settings combobox a real combobox.

Make notifier settings combobox a real combobox.

It looks and acts like a combobox but was actually a menu button. I
needed to change this to remove another reference to STYLE_BUTTON, and
generally to improve consistency of appearance and behavior.

I also got sucked into rearranging padding/margins.

BUG=642920
Committed: https://crrev.com/2d4a2889b976a49a743d7ba32797883190af303b
Cr-Commit-Position: refs/heads/master@{#416749}

[Evan Stade, 2016-09-01 00:50:39]

estade@chromium.org changed reviewers:
+ sky@chromium.org

[Evan Stade, 2016-09-01 00:50:39]

https://screenshot.googleplex.com/QYJvHxyOrKq.png

[sky, 2016-09-01 04:13:04]

https://codereview.chromium.org/2300893002/diff/20001/ui/base/models/combobox...
File ui/base/models/combobox_model.h (left):

https://codereview.chromium.org/2300893002/diff/20001/ui/base/models/combobox...
ui/base/models/combobox_model.h:40: virtual ~ComboboxModel() {}
Move to top now. In general I would prefer to keep this protected, but I get why
you want it.

https://codereview.chromium.org/2300893002/diff/20001/ui/message_center/views...
File ui/message_center/views/notifier_settings_view.cc (left):

https://codereview.chromium.org/2300893002/diff/20001/ui/message_center/views...
ui/message_center/views/notifier_settings_view.cc:240: AddCheckItem(i,
group.login_info.empty() ? group.name : group.login_info);
Isn't the check item different than what you get with a combobox?

[Evan Stade, 2016-09-01 20:08:41]

https://codereview.chromium.org/2300893002/diff/20001/ui/base/models/combobox...
File ui/base/models/combobox_model.h (left):

https://codereview.chromium.org/2300893002/diff/20001/ui/base/models/combobox...
ui/base/models/combobox_model.h:40: virtual ~ComboboxModel() {}
On 2016/09/01 04:13:03, sky wrote:
> Move to top now. In general I would prefer to keep this protected, but I get
why
> you want it.

Done.

https://codereview.chromium.org/2300893002/diff/20001/ui/message_center/views...
File ui/message_center/views/notifier_settings_view.cc (left):

https://codereview.chromium.org/2300893002/diff/20001/ui/message_center/views...
ui/message_center/views/notifier_settings_view.cc:240: AddCheckItem(i,
group.login_info.empty() ? group.name : group.login_info);
On 2016/09/01 04:13:03, sky wrote:
> Isn't the check item different than what you get with a combobox?

Normally multiple things can be checked, but in this model only one thing is
checked at a time. Thus the only difference is the lack of a checkmark. This is
another weird inconsistency that we're getting rid of (since normal combobox
selection is indicated by what's in the main control and all menu items look the
same).

[Evan Stade, 2016-09-01 20:09:13]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 20:09:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-01 20:13:09]

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

[commit-bot: I haz the power, 2016-09-01 20:13:10]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Evan Stade, 2016-09-01 20:28:30]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 20:28:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-01 21:34:15]

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

[commit-bot: I haz the power, 2016-09-01 21:34:16]

Dry run: This issue passed the CQ dry run.

[sky, 2016-09-01 21:52:13]

https://codereview.chromium.org/2300893002/diff/20001/ui/message_center/views...
File ui/message_center/views/notifier_settings_view.cc (left):

https://codereview.chromium.org/2300893002/diff/20001/ui/message_center/views...
ui/message_center/views/notifier_settings_view.cc:240: AddCheckItem(i,
group.login_info.empty() ? group.name : group.login_info);
On 2016/09/01 20:08:41, Evan Stade wrote:
> On 2016/09/01 04:13:03, sky wrote:
> > Isn't the check item different than what you get with a combobox?
> 
> Normally multiple things can be checked, but in this model only one thing is
> checked at a time. Thus the only difference is the lack of a checkmark. This
is
> another weird inconsistency that we're getting rid of (since normal combobox
> selection is indicated by what's in the main control and all menu items look
the
> same).

Should you at least change the selection to reflect the item that was previously
checked?

[Evan Stade, 2016-09-06 17:43:50]

https://codereview.chromium.org/2300893002/diff/20001/ui/message_center/views...
File ui/message_center/views/notifier_settings_view.cc (left):

https://codereview.chromium.org/2300893002/diff/20001/ui/message_center/views...
ui/message_center/views/notifier_settings_view.cc:240: AddCheckItem(i,
group.login_info.empty() ? group.name : group.login_info);
On 2016/09/01 21:52:13, sky wrote:
> On 2016/09/01 20:08:41, Evan Stade wrote:
> > On 2016/09/01 04:13:03, sky wrote:
> > > Isn't the check item different than what you get with a combobox?
> > 
> > Normally multiple things can be checked, but in this model only one thing is
> > checked at a time. Thus the only difference is the lack of a checkmark. This
> is
> > another weird inconsistency that we're getting rid of (since normal combobox
> > selection is indicated by what's in the main control and all menu items look
> the
> > same).
> 
> Should you at least change the selection to reflect the item that was
previously
> checked?

not sure what you mean, doesn't that happen automatically in
Combobox::ComboboxMenuModel::ActivatedAt/Combobox::OnPerformAction

[sky, 2016-09-06 20:25:36]

On 2016/09/06 17:43:50, Evan Stade wrote:
>
https://codereview.chromium.org/2300893002/diff/20001/ui/message_center/views...
> File ui/message_center/views/notifier_settings_view.cc (left):
> 
>
https://codereview.chromium.org/2300893002/diff/20001/ui/message_center/views...
> ui/message_center/views/notifier_settings_view.cc:240: AddCheckItem(i,
> group.login_info.empty() ? group.name : group.login_info);
> On 2016/09/01 21:52:13, sky wrote:
> > On 2016/09/01 20:08:41, Evan Stade wrote:
> > > On 2016/09/01 04:13:03, sky wrote:
> > > > Isn't the check item different than what you get with a combobox?
> > > 
> > > Normally multiple things can be checked, but in this model only one thing
is
> > > checked at a time. Thus the only difference is the lack of a checkmark.
This
> > is
> > > another weird inconsistency that we're getting rid of (since normal
combobox
> > > selection is indicated by what's in the main control and all menu items
look
> > the
> > > same).
> > 
> > Should you at least change the selection to reflect the item that was
> previously
> > checked?
> 
> not sure what you mean, doesn't that happen automatically in
> Combobox::ComboboxMenuModel::ActivatedAt/Combobox::OnPerformAction

When something is selected, yes, but don't you want the initial selection
(before the user does anything) to reflect the checked item?

[Evan Stade, 2016-09-06 20:36:23]

On 2016/09/06 20:25:36, sky wrote:
> On 2016/09/06 17:43:50, Evan Stade wrote:
> >
>
https://codereview.chromium.org/2300893002/diff/20001/ui/message_center/views...
> > File ui/message_center/views/notifier_settings_view.cc (left):
> > 
> >
>
https://codereview.chromium.org/2300893002/diff/20001/ui/message_center/views...
> > ui/message_center/views/notifier_settings_view.cc:240: AddCheckItem(i,
> > group.login_info.empty() ? group.name : group.login_info);
> > On 2016/09/01 21:52:13, sky wrote:
> > > On 2016/09/01 20:08:41, Evan Stade wrote:
> > > > On 2016/09/01 04:13:03, sky wrote:
> > > > > Isn't the check item different than what you get with a combobox?
> > > > 
> > > > Normally multiple things can be checked, but in this model only one
thing
> is
> > > > checked at a time. Thus the only difference is the lack of a checkmark.
> This
> > > is
> > > > another weird inconsistency that we're getting rid of (since normal
> combobox
> > > > selection is indicated by what's in the main control and all menu items
> look
> > > the
> > > > same).
> > > 
> > > Should you at least change the selection to reflect the item that was
> > previously
> > > checked?
> > 
> > not sure what you mean, doesn't that happen automatically in
> > Combobox::ComboboxMenuModel::ActivatedAt/Combobox::OnPerformAction
> 
> When something is selected, yes, but don't you want the initial selection
> (before the user does anything) to reflect the checked item?

The default implementation of ComboboxModel::GetDefaultIndex() (which just
returns 0) works in this case.

[sky, 2016-09-06 21:45:51]

Evan says the first item was always checked. So, ok, LGTM

[Evan Stade, 2016-09-06 21:47:11]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-09-06 21:47:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 23:04:12]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-06 23:06:23]

Description was changed from

==========
Make notifier settings combobox a real combobox.

It looks and acts like a combobox but was actually a menu button. I
needed to change this to remove another reference to STYLE_BUTTON, and
generally to improve consistency of appearance and behavior.

I also got sucked into rearranging padding/margins.

BUG=642920
==========

to

==========
Make notifier settings combobox a real combobox.

It looks and acts like a combobox but was actually a menu button. I
needed to change this to remove another reference to STYLE_BUTTON, and
generally to improve consistency of appearance and behavior.

I also got sucked into rearranging padding/margins.

BUG=642920

Committed: https://crrev.com/2d4a2889b976a49a743d7ba32797883190af303b
Cr-Commit-Position: refs/heads/master@{#416749}
==========

[commit-bot: I haz the power, 2016-09-06 23:06:24]

Patchset 4 (id:??) landed as
https://crrev.com/2d4a2889b976a49a743d7ba32797883190af303b
Cr-Commit-Position: refs/heads/master@{#416749}