MacViews: Ensure cancel buttons don't get default button styling.

MacViews: Ensure cancel buttons don't get default button styling.

r406276 introduced kDialogDefaultButtonCanBeCancel to ensure buttons with the
type DIALOG_BUTTON_CANCEL do not have default button styling on Mac.
Subsequently, r420488 made all default dialog buttons blue on Harfmony. This CL
changes DialogClientView::CreateDialogButton to take
kDialogDefaultButtonCanBeCancel into consideration before applying the default
button styling on Harmony. This ensures Cancel buttons under Harmony on MacViews
do not get the default button styling.

BUG=678165, 605652
TEST= On Mac, enable chrome://flags//secondary-ui-md. Open Collected Cookies
dialog. Ensure close button does not have default button styling.

Committed: https://crrev.com/eaff8e380fa4ef7ad90a5929eb35ce81df786f70
Cr-Commit-Position: refs/heads/master@{#441615}

[karandeepb, 2017-01-04 07:46:50]

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

[karandeepb, 2017-01-04 07:46:51]

PTAL Trent.

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

https://codereview.chromium.org/2615563003/diff/1/ui/views/window/dialog_clie...
ui/views/window/dialog_client_view.cc:329: (type != ui::DIALOG_BUTTON_CANCEL ||
This is similar to what we do in DialogDelegate::UpdateButton. This seems a bit
ugly to me, I think GetDefaultDialogButton should not return CANCEL button on
Mac. However, the problem I think is that GetDefaultDialogButton is virtual. 

One solution may be to have a public non-virtual
GetDefaultDialogButtonPublic(say) on DialogDelegate which delegates to a private
virtual GetDefaultDialogButton. Then GetDefaultDialogButtonPublic can take
kDialogDefaultButtonCanBeCancel into account. Alternatively, the existing
overrides can take kDialogDefaultButtonCanBeCancel into account, but that'll be
error prone.

Also, I haven't modified DialogDelegate::GetInitiallyFocusedView to take
kDialogDefaultButtonCanBeCancel into account, since that would cause no view to
be focused initially for some dialogs.

[tapted, 2017-01-04 23:11:40]

lgtm

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

https://codereview.chromium.org/2615563003/diff/1/ui/views/window/dialog_clie...
ui/views/window/dialog_client_view.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
CL description: Harfmony -> Harmony

https://codereview.chromium.org/2615563003/diff/1/ui/views/window/dialog_clie...
ui/views/window/dialog_client_view.cc:329: (type != ui::DIALOG_BUTTON_CANCEL ||
On 2017/01/04 07:46:51, karandeepb wrote:
> This is similar to what we do in DialogDelegate::UpdateButton. This seems a
bit
> ugly to me, I think GetDefaultDialogButton should not return CANCEL button on
> Mac. However, the problem I think is that GetDefaultDialogButton is virtual. 
>
> One solution may be to have a public non-virtual
> GetDefaultDialogButtonPublic(say) on DialogDelegate which delegates to a
private
> virtual GetDefaultDialogButton. Then GetDefaultDialogButtonPublic can take
> kDialogDefaultButtonCanBeCancel into account. Alternatively, the existing
> overrides can take kDialogDefaultButtonCanBeCancel into account, but that'll
be
> error prone.

IMO the nicer fix would be to merge the Blue/Regular MD Button codepaths some
more, so that the color can be inferred from SetIsDefault(). It looks like, on
MD, the blue button is actually the same class, just with a call to
MdTextButton::SetProminent(true). It's only on non-MD that it's a different
class. So this may get a cleanup when non-MD buttons get torn out.

> 
> Also, I haven't modified DialogDelegate::GetInitiallyFocusedView to take
> kDialogDefaultButtonCanBeCancel into account, since that would cause no view
to
> be focused initially for some dialogs.

I think Patti's change will help move the focus along if someone isn't using
full keyboard access - hopefully we don't have to worry about this.

[karandeepb, 2017-01-05 09:13:49]

The CQ bit was checked by karandeepb@chromium.org

[commit-bot: I haz the power, 2017-01-05 09:13:57]

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-01-05 10:03:34]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1483607629328930, "parent_rev":
"57cb44ea51e5dad9e3344b694d6fa70a54721660", "commit_rev":
"9a14df1e4ced27e71e47222e2926e298dbff3af1"}

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

Description was changed from

==========
MacViews: Ensure cancel buttons don't get default button styling.

r406276 introduced kDialogDefaultButtonCanBeCancel to ensure buttons with the
type DIALOG_BUTTON_CANCEL do not have default button styling on Mac.
Subsequently, r420488 made all default dialog buttons blue on Harfmony. This CL
changes DialogClientView::CreateDialogButton to take
kDialogDefaultButtonCanBeCancel into consideration before applying the default
button styling on Harmony. This ensures Cancel buttons under Harmony on MacViews
do not get the default button styling.

BUG= 678165, 605652
TEST= On Mac, enable chrome://flags//secondary-ui-md. Open Collected Cookies
dialog. Ensure close button does not have default button styling.
==========

to

==========
MacViews: Ensure cancel buttons don't get default button styling.

r406276 introduced kDialogDefaultButtonCanBeCancel to ensure buttons with the
type DIALOG_BUTTON_CANCEL do not have default button styling on Mac.
Subsequently, r420488 made all default dialog buttons blue on Harfmony. This CL
changes DialogClientView::CreateDialogButton to take
kDialogDefaultButtonCanBeCancel into consideration before applying the default
button styling on Harmony. This ensures Cancel buttons under Harmony on MacViews
do not get the default button styling.

BUG= 678165, 605652
TEST= On Mac, enable chrome://flags//secondary-ui-md. Open Collected Cookies
dialog. Ensure close button does not have default button styling.

Review-Url: https://codereview.chromium.org/2615563003
==========

[commit-bot: I haz the power, 2017-01-05 10:04:02]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2017-01-05 10:06:05]

Description was changed from

==========
MacViews: Ensure cancel buttons don't get default button styling.

r406276 introduced kDialogDefaultButtonCanBeCancel to ensure buttons with the
type DIALOG_BUTTON_CANCEL do not have default button styling on Mac.
Subsequently, r420488 made all default dialog buttons blue on Harfmony. This CL
changes DialogClientView::CreateDialogButton to take
kDialogDefaultButtonCanBeCancel into consideration before applying the default
button styling on Harmony. This ensures Cancel buttons under Harmony on MacViews
do not get the default button styling.

BUG= 678165, 605652
TEST= On Mac, enable chrome://flags//secondary-ui-md. Open Collected Cookies
dialog. Ensure close button does not have default button styling.

Review-Url: https://codereview.chromium.org/2615563003
==========

to

==========
MacViews: Ensure cancel buttons don't get default button styling.

r406276 introduced kDialogDefaultButtonCanBeCancel to ensure buttons with the
type DIALOG_BUTTON_CANCEL do not have default button styling on Mac.
Subsequently, r420488 made all default dialog buttons blue on Harfmony. This CL
changes DialogClientView::CreateDialogButton to take
kDialogDefaultButtonCanBeCancel into consideration before applying the default
button styling on Harmony. This ensures Cancel buttons under Harmony on MacViews
do not get the default button styling.

BUG= 678165, 605652
TEST= On Mac, enable chrome://flags//secondary-ui-md. Open Collected Cookies
dialog. Ensure close button does not have default button styling.

Committed: https://crrev.com/eaff8e380fa4ef7ad90a5929eb35ce81df786f70
Cr-Commit-Position: refs/heads/master@{#441615}
==========

[commit-bot: I haz the power, 2017-01-05 10:06:06]

Patchset 1 (id:??) landed as
https://crrev.com/eaff8e380fa4ef7ad90a5929eb35ce81df786f70
Cr-Commit-Position: refs/heads/master@{#441615}