Don't allow Checkboxes to participate in equal-sized-buttons in Harmony dialogs.

Don't allow Checkboxes to participate in equal-sized-buttons in Harmony dialogs.

Currently when a checkbox is the extra view, the checkbox tends to cause
the other buttons in a dialog to get bigger. This is because Checkbox
extends LabelButton, so the check using AsCustomButton() will classify
it the same way.

To fix, explicitly filter out Checkboxes from the equal-sizing logic in
DialogClientView.

BUG=671820
Review-Url: https://codereview.chromium.org/2855623004
Cr-Commit-Position: refs/heads/master@{#469230}
Committed: https://chromium.googlesource.com/chromium/src/+/7fb6670abe4d5f8d6caa26f39bf147c3844a3ec8

[tapted, 2017-05-02 07:41:29]

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

[commit-bot: I haz the power, 2017-05-02 07:41:39]

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, 2017-05-02 08:31:15]

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

[commit-bot: I haz the power, 2017-05-02 08:31:16]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-05-02 12:15:10]

Description was changed from

==========
Don't allow Checkboxes to participate in equal-sized-buttons in Harmony dialogs.

BUG=
==========

to

==========
Don't allow Checkboxes to participate in equal-sized-buttons in Harmony dialogs.

Currently when a checkbox is the extra view, the checkbox tends to cause
the other buttons in a dialog to get bigger. This is because Checkbox
extends LabelButton, so the check using AsCustomButton() will classify
it the same way.

Instead, require the button type to match exactly in order for it to
participate in the equal-button sizing logic.

BUG=671820
==========

[tapted, 2017-05-02 12:15:10]

tapted@chromium.org changed reviewers:
+ msw@chromium.org

[tapted, 2017-05-02 12:15:37]

Hi Mike, please take a look

[msw, 2017-05-02 17:20:10]

https://codereview.chromium.org/2855623004/diff/1/ui/views/window/dialog_clie...
File ui/views/window/dialog_client_view.cc (right):

https://codereview.chromium.org/2855623004/diff/1/ui/views/window/dialog_clie...
ui/views/window/dialog_client_view.cc:398: if (either_button_view && extra_view_
&&
nit: curlies for multi-line conditional?

https://codereview.chromium.org/2855623004/diff/1/ui/views/window/dialog_clie...
ui/views/window/dialog_client_view.cc:399: either_button_view->GetClassName() !=
extra_view_->GetClassName())
Won't this exclude other cases that we would want to be linked? (eg. ok/cancel
is a MdTextButton/BlueButton, but the extra view is a LabelButton), there are
lots of button subclasses... Maybe just exclude checkbox extra views?

[tapted, 2017-05-03 00:12:57]

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

[commit-bot: I haz the power, 2017-05-03 00:13:20]

Dry run: CQ is trying da patch.

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

[tapted, 2017-05-03 00:13:43]

Description was changed from

==========
Don't allow Checkboxes to participate in equal-sized-buttons in Harmony dialogs.

Currently when a checkbox is the extra view, the checkbox tends to cause
the other buttons in a dialog to get bigger. This is because Checkbox
extends LabelButton, so the check using AsCustomButton() will classify
it the same way.

Instead, require the button type to match exactly in order for it to
participate in the equal-button sizing logic.

BUG=671820
==========

to

==========
Don't allow Checkboxes to participate in equal-sized-buttons in Harmony dialogs.

Currently when a checkbox is the extra view, the checkbox tends to cause
the other buttons in a dialog to get bigger. This is because Checkbox
extends LabelButton, so the check using AsCustomButton() will classify
it the same way.

To fix, explicitly filter out Checkboxes from the equal-sizing logic in
DialogClientView.

BUG=671820
==========

[tapted, 2017-05-03 00:16:59]

https://codereview.chromium.org/2855623004/diff/1/ui/views/window/dialog_clie...
File ui/views/window/dialog_client_view.cc (right):

https://codereview.chromium.org/2855623004/diff/1/ui/views/window/dialog_clie...
ui/views/window/dialog_client_view.cc:398: if (either_button_view && extra_view_
&&
On 2017/05/02 17:20:10, msw wrote:
> nit: curlies for multi-line conditional?

Broke out a separate bool - condition got a little too messy.

https://codereview.chromium.org/2855623004/diff/1/ui/views/window/dialog_clie...
ui/views/window/dialog_client_view.cc:399: either_button_view->GetClassName() !=
extra_view_->GetClassName())
On 2017/05/02 17:20:10, msw wrote:
> Won't this exclude other cases that we would want to be linked? (eg. ok/cancel
> is a MdTextButton/BlueButton, but the extra view is a LabelButton), there are
> lots of button subclasses... Maybe just exclude checkbox extra views?

Done.

I think, because this is only for Harmony, there should only be MdTextButton
(dialogs should be using MdTextButton::CreateSecondaryUi[Blue]Button which will
always be MdTextButton, possibly with SetProminent(true)). And dialogs shouldn't
mix LabelButtons and MdTextButton.

But it could be limiting in other ways, and I like the idea of just excluding
Checkbox (downside is a teeny bit of coupling between DialogClientView and
Checkbox). There is also RadioButton.. but I think we can assume nobody is going
to put that in the extra view slot.

[commit-bot: I haz the power, 2017-05-03 01:04:08]

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

[commit-bot: I haz the power, 2017-05-03 01:04:09]

Dry run: This issue passed the CQ dry run.

[msw, 2017-05-03 18:21:02]

lgtm

https://codereview.chromium.org/2855623004/diff/1/ui/views/window/dialog_clie...
File ui/views/window/dialog_client_view.cc (right):

https://codereview.chromium.org/2855623004/diff/1/ui/views/window/dialog_clie...
ui/views/window/dialog_client_view.cc:399: either_button_view->GetClassName() !=
extra_view_->GetClassName())
On 2017/05/03 00:16:59, tapted wrote:
> On 2017/05/02 17:20:10, msw wrote:
> > Won't this exclude other cases that we would want to be linked? (eg.
ok/cancel
> > is a MdTextButton/BlueButton, but the extra view is a LabelButton), there
are
> > lots of button subclasses... Maybe just exclude checkbox extra views?
> 
> Done.
> 
> I think, because this is only for Harmony, there should only be MdTextButton
> (dialogs should be using MdTextButton::CreateSecondaryUi[Blue]Button which
will
> always be MdTextButton, possibly with SetProminent(true)). And dialogs
shouldn't
> mix LabelButtons and MdTextButton.

Random dialogs might be using LabelButton for the extra view.

https://codereview.chromium.org/2855623004/diff/20001/ui/views/window/dialog_...
File ui/views/window/dialog_client_view_unittest.cc (right):

https://codereview.chromium.org/2855623004/diff/20001/ui/views/window/dialog_...
ui/views/window/dialog_client_view_unittest.cc:402: // Checkbox extends
LabelButton. It should not participate in linking.
nit: "LabelButton, but it"

[tapted, 2017-05-04 00:31:12]

Thanks Mike!

https://codereview.chromium.org/2855623004/diff/20001/ui/views/window/dialog_...
File ui/views/window/dialog_client_view_unittest.cc (right):

https://codereview.chromium.org/2855623004/diff/20001/ui/views/window/dialog_...
ui/views/window/dialog_client_view_unittest.cc:402: // Checkbox extends
LabelButton. It should not participate in linking.
On 2017/05/03 18:21:02, msw wrote:
> nit: "LabelButton, but it"

Done.

[tapted, 2017-05-04 00:31:21]

The CQ bit was checked by tapted@chromium.org

[tapted, 2017-05-04 00:31:22]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2855623004/#ps40001
(title: "update comment")

[commit-bot: I haz the power, 2017-05-04 00:31:43]

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, 2017-05-04 01:24:23]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493857881563890,
"parent_rev": "a2ff3163407b76f7b8bfbc66b96d4bd0e21596b2", "commit_rev":
"7fb6670abe4d5f8d6caa26f39bf147c3844a3ec8"}

[commit-bot: I haz the power, 2017-05-04 01:24:33]

Description was changed from

==========
Don't allow Checkboxes to participate in equal-sized-buttons in Harmony dialogs.

Currently when a checkbox is the extra view, the checkbox tends to cause
the other buttons in a dialog to get bigger. This is because Checkbox
extends LabelButton, so the check using AsCustomButton() will classify
it the same way.

To fix, explicitly filter out Checkboxes from the equal-sizing logic in
DialogClientView.

BUG=671820
==========

to

==========
Don't allow Checkboxes to participate in equal-sized-buttons in Harmony dialogs.

Currently when a checkbox is the extra view, the checkbox tends to cause
the other buttons in a dialog to get bigger. This is because Checkbox
extends LabelButton, so the check using AsCustomButton() will classify
it the same way.

To fix, explicitly filter out Checkboxes from the equal-sizing logic in
DialogClientView.

BUG=671820

Review-Url: https://codereview.chromium.org/2855623004
Cr-Commit-Position: refs/heads/master@{#469230}
Committed:
https://chromium.googlesource.com/chromium/src/+/7fb6670abe4d5f8d6caa26f39bf1...
==========

[commit-bot: I haz the power, 2017-05-04 01:24:34]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/7fb6670abe4d5f8d6caa26f39bf1...