Fix style of profile error dialog.

Fix style of profile error dialog.

Profile error dialog should show a blue button to match a material
design look.

BUG=729749
Review-Url: https://codereview.chromium.org/2922263002
Cr-Commit-Position: refs/heads/master@{#478352}
Committed: https://chromium.googlesource.com/chromium/src/+/bfb873220a103ccee2cd0480782a7a238c55c95d

[afakhry, 2017-06-05 22:23:08]

afakhry@chromium.org changed reviewers:
+ pkasting@chromium.org

[afakhry, 2017-06-05 22:23:08]

pkasting, please take a look. Thank you!

[Peter Kasting, 2017-06-05 22:28:09]

Apparently this is already fixed in Harmony:
https://cs.chromium.org/chromium/src/ui/views/window/dialog_client_view.cc?l=286

So you can do one of a couple things IMO.

(1) Just wait for Harmony and abandon this change.
(2) Rip out ShouldDefaultButtonBeBlue() entirely and basically turn on the
Harmony behavior now, since we already have blue buttons in a lot of places (so
it's not actually that big of a behavior shift).

The change as-is walks a middle-ground where it converts message boxes to have
blue buttons, but not everything.  I'm not a big fan of that compared to either
option above.

[afakhry, 2017-06-05 23:20:49]

On 2017/06/05 22:28:09, Peter Kasting wrote:
> Apparently this is already fixed in Harmony:
>
https://cs.chromium.org/chromium/src/ui/views/window/dialog_client_view.cc?l=286
> 
> So you can do one of a couple things IMO.
> 
> (1) Just wait for Harmony and abandon this change.
> (2) Rip out ShouldDefaultButtonBeBlue() entirely and basically turn on the
> Harmony behavior now, since we already have blue buttons in a lot of places
(so
> it's not actually that big of a behavior shift).
> 
> The change as-is walks a middle-ground where it converts message boxes to have
> blue buttons, but not everything.  I'm not a big fan of that compared to
either
> option above.

Ah I see. I'm with removing ShouldDefaultButtonBeBlue() entirely. But regarding
turning on Harmony now, are you suggesting that we change 
|MaterialDesignController::include_secondary_ui_| [1] to be true by default, and
change the semantics of the flag "kExtendMdToSecondaryUi" [2] from enable to
disable?

[1]
https://cs.chromium.org/chromium/src/ui/base/material_design/material_design_...

[2]
https://cs.chromium.org/chromium/src/chrome/browser/about_flags.cc?type=cs&q=...

[Peter Kasting, 2017-06-05 23:24:11]

On 2017/06/05 23:20:49, afakhry wrote:
> Ah I see. I'm with removing ShouldDefaultButtonBeBlue() entirely. But
regarding
> turning on Harmony now, are you suggesting that we change 
> |MaterialDesignController::include_secondary_ui_| [1] to be true by default,
and
> change the semantics of the flag "kExtendMdToSecondaryUi" [2] from enable to
> disable?

No, I meant "only at the lone current callsite of ShouldDefaultButtonBeBlue(),
enable the current Harmony behavior [of always treating this as true]".

[afakhry, 2017-06-05 23:34:20]

On 2017/06/05 23:24:11, Peter Kasting wrote:
> On 2017/06/05 23:20:49, afakhry wrote:
> > Ah I see. I'm with removing ShouldDefaultButtonBeBlue() entirely. But
> regarding
> > turning on Harmony now, are you suggesting that we change 
> > |MaterialDesignController::include_secondary_ui_| [1] to be true by default,
> and
> > change the semantics of the flag "kExtendMdToSecondaryUi" [2] from enable to
> > disable?
> 
> No, I meant "only at the lone current callsite of ShouldDefaultButtonBeBlue(),
> enable the current Harmony behavior [of always treating this as true]".

Got it. Done.

[afakhry, 2017-06-05 23:34:28]

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

[commit-bot: I haz the power, 2017-06-05 23:35:07]

Dry run: CQ is trying da patch.

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

[afakhry, 2017-06-05 23:45:04]

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

[afakhry, 2017-06-05 23:45:30]

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

https://codereview.chromium.org/2922263002/diff/20001/ui/views/window/dialog_...
ui/views/window/dialog_client_view.cc:288: : button =
MdTextButton::CreateSecondaryUiButton(this, title);
Oops .. fixed this.

[commit-bot: I haz the power, 2017-06-05 23:45:33]

Dry run: CQ is trying da patch.

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

[Peter Kasting, 2017-06-05 23:46:12]

LGTM

https://codereview.chromium.org/2922263002/diff/40001/ui/views/window/dialog_...
File ui/views/window/dialog_client_view.cc (right):

https://codereview.chromium.org/2922263002/diff/40001/ui/views/window/dialog_...
ui/views/window/dialog_client_view.cc:285: // The default button is always blue
in Harmony.
Nit: Comment is no longer accurate (or necessary)

[afakhry, 2017-06-05 23:48:59]

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

[afakhry, 2017-06-05 23:49:00]

sky@chromium.org: Please review changes in ui/. Thanks!

https://codereview.chromium.org/2922263002/diff/40001/ui/views/window/dialog_...
File ui/views/window/dialog_client_view.cc (right):

https://codereview.chromium.org/2922263002/diff/40001/ui/views/window/dialog_...
ui/views/window/dialog_client_view.cc:285: // The default button is always blue
in Harmony.
On 2017/06/05 23:46:11, Peter Kasting wrote:
> Nit: Comment is no longer accurate (or necessary)

Done.

[sky, 2017-06-06 00:00:49]

LGTM

https://codereview.chromium.org/2922263002/diff/60001/ui/views/window/dialog_...
File ui/views/window/dialog_client_view.cc (left):

https://codereview.chromium.org/2922263002/diff/60001/ui/views/window/dialog_...
ui/views/window/dialog_client_view.cc:286: if (is_default &&
(ui::MaterialDesignController::IsSecondaryUiMaterial() ||
I'm assuming we don't care about
ui::MaterialDesignController::IsSecondaryUiMaterial() anymore.

[afakhry, 2017-06-06 00:20:35]

https://codereview.chromium.org/2922263002/diff/60001/ui/views/window/dialog_...
File ui/views/window/dialog_client_view.cc (left):

https://codereview.chromium.org/2922263002/diff/60001/ui/views/window/dialog_...
ui/views/window/dialog_client_view.cc:286: if (is_default &&
(ui::MaterialDesignController::IsSecondaryUiMaterial() ||
On 2017/06/06 00:00:49, sky wrote:
> I'm assuming we don't care about
> ui::MaterialDesignController::IsSecondaryUiMaterial() anymore.

Right, in this instance we don't, as we want the default button to always be
blue.

[afakhry, 2017-06-06 00:33:31]

The CQ bit was checked by afakhry@chromium.org

[afakhry, 2017-06-06 00:33:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2922263002/#ps60001
(title: "nit")

[commit-bot: I haz the power, 2017-06-06 00:34:04]

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-06-06 01:26:46]

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

[commit-bot: I haz the power, 2017-06-06 01:26:47]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[afakhry, 2017-06-06 15:48:30]

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

[afakhry, 2017-06-06 15:48:31]

+tapted@, for some reason, this change results in the test
DialogClientViewTest.UpdateButtons failing on Mac at this line:
https://cs.chromium.org/chromium/src/ui/views/window/dialog_client_view_unitt...

[ RUN      ] DialogClientViewTest.UpdateButtons
[97299:771:0605/174432.386678:46463972839642:ERROR:native_widget_mac.mm(281)]
Not implemented reached in virtual void
views::NativeWidgetMac::SetWindowIcons(const gfx::ImageSkia &, const
gfx::ImageSkia &)
../../ui/views/window/dialog_client_view_unittest.cc:201: Failure
      Expected: GetUpdatedClientBounds().height()
      Which is: 59
To be equal to: height_with_buttons
      Which is: 60

The height of the client bounds seems to decrease by 1 when we only have a
single cancel button. I can't repro this locally because I don't have a Mac, but
do you see anything that sticks out in this change?

[Peter Kasting, 2017-06-06 18:33:01]

On 2017/06/06 15:48:31, afakhry wrote:
> The height of the client bounds seems to decrease by 1 when we only have a
> single cancel button. I can't repro this locally because I don't have a Mac,
but
> do you see anything that sticks out in this change?

That suggests that the "blue button" itself has a 1 px shorter preferred height
than the normal button, at least on Mac.  That's probably not intentional.

[tapted, 2017-06-07 01:18:49]

On 2017/06/06 18:33:01, Peter Kasting wrote:
> On 2017/06/06 15:48:31, afakhry wrote:
> > The height of the client bounds seems to decrease by 1 when we only have a
> > single cancel button. I can't repro this locally because I don't have a Mac,
> but
> > do you see anything that sticks out in this change?
> 
> That suggests that the "blue button" itself has a 1 px shorter preferred
height
> than the normal button, at least on Mac.  That's probably not intentional.

I think it's shadows. views::BlueButton unconditionally applies shadows in
BlueButton::ResetColorsFromNativeTheme(), but LabelButton uses
PlatformStyle::ApplyLabelButtonTextStyle(..).

So on mac, if there is not at least one BlueButton (`Cancel` should never be a
default button on Mac), then the dialog is smaller. We've been mostly working
with "BlueButton should never appear on Mac, just prominent MdTexButton", so
this shouldn't matter in the long run, but maybe web-payments want one and it
would be odd for shadows to be on the blue buttons, but not regular buttons.

But, currently, it seems like this is already the case for Linux? The
PlatformStyle::ApplyLabelButtonTextStyle for desktop linux is a no-op. It won't
add shadows. But BlueButton will. Probably
BlueButton::ResetColorsFromNativeTheme() should use
PlatformStyle::ApplyLabelButtonTextStyle() somehow.

(it could also be bold fonts... but I think that's less likely. E.g., on Mac,
`Cancel` should never be a default button. But an `OK` button can be. For
regular / non-MD buttons, default buttons get a Bold font. A Bold font may be
taller than the equivalent non-bold font. So while there is at least one button
with a Bold font, the dialog height may be taller versus the case where no
buttons have a bold font.)

[Peter Kasting, 2017-06-07 01:30:43]

On 2017/06/07 01:18:49, tapted wrote:
> On 2017/06/06 18:33:01, Peter Kasting wrote:
> > On 2017/06/06 15:48:31, afakhry wrote:
> > > The height of the client bounds seems to decrease by 1 when we only have a
> > > single cancel button. I can't repro this locally because I don't have a
Mac,
> > but
> > > do you see anything that sticks out in this change?
> > 
> > That suggests that the "blue button" itself has a 1 px shorter preferred
> height
> > than the normal button, at least on Mac.  That's probably not intentional.
> 
> I think it's shadows. views::BlueButton unconditionally applies shadows in
> BlueButton::ResetColorsFromNativeTheme(), but LabelButton uses
> PlatformStyle::ApplyLabelButtonTextStyle(..).

That makes sense.  (Also says my analysis was backwards -- BlueButton is taller,
not shorter.)

> So on mac, if there is not at least one BlueButton (`Cancel` should never be a
> default button on Mac), then the dialog is smaller. We've been mostly working
> with "BlueButton should never appear on Mac, just prominent MdTexButton", so
> this shouldn't matter in the long run, but maybe web-payments want one and it
> would be odd for shadows to be on the blue buttons, but not regular buttons.

(1) What do you mean by "BlueButton should never appear on Mac, just prominent
MdTexButton"?  Are you saying CreateSecondaryUiBlueButton() should never be
called on Mac (even in Harmony)?  Was the existing code (for Harmony) in
DialogClientView::UpdateDialogButton() wrong?

(2) How did web payments get in the picture?

(3) I do think it's a little odd to put shadows on blue buttons unconditionally
but not other buttons unconditionally.  I don't know what the right behavior is,
but that inconsistency seems wrong.

> But, currently, it seems like this is already the case for Linux? The
> PlatformStyle::ApplyLabelButtonTextStyle for desktop linux is a no-op. It
won't
> add shadows. But BlueButton will. Probably
> BlueButton::ResetColorsFromNativeTheme() should use
> PlatformStyle::ApplyLabelButtonTextStyle() somehow.

Yeah.  I feel like we should fix this, but we shouldn't do it in this CL.  We
need to figure out what the desired behavior is.

In the meantime, we should probably fix/disable our tests to not flag this, land
it, and file a bug for the shadow behavior.

That all assumes the shadow is indeed the issue.  afakhry, are you able to
verify tapted's hypothesis?

[tapted, 2017-06-07 01:39:51]

On 2017/06/07 01:30:43, Peter Kasting wrote:
> On 2017/06/07 01:18:49, tapted wrote:
> > On 2017/06/06 18:33:01, Peter Kasting wrote:
> > > On 2017/06/06 15:48:31, afakhry wrote:
> > > > The height of the client bounds seems to decrease by 1 when we only have
a
> > > > single cancel button. I can't repro this locally because I don't have a
> Mac,
> > > but
> > > > do you see anything that sticks out in this change?
> > > 
> > > That suggests that the "blue button" itself has a 1 px shorter preferred
> > height
> > > than the normal button, at least on Mac.  That's probably not intentional.
> > 
> > I think it's shadows. views::BlueButton unconditionally applies shadows in
> > BlueButton::ResetColorsFromNativeTheme(), but LabelButton uses
> > PlatformStyle::ApplyLabelButtonTextStyle(..).
> 
> That makes sense.  (Also says my analysis was backwards -- BlueButton is
taller,
> not shorter.)
> 
> > So on mac, if there is not at least one BlueButton (`Cancel` should never be
a
> > default button on Mac), then the dialog is smaller. We've been mostly
working
> > with "BlueButton should never appear on Mac, just prominent MdTexButton", so
> > this shouldn't matter in the long run, but maybe web-payments want one and
it
> > would be odd for shadows to be on the blue buttons, but not regular buttons.
> 
> (1) What do you mean by "BlueButton should never appear on Mac, just prominent
> MdTexButton"?  Are you saying CreateSecondaryUiBlueButton() should never be
> called on Mac (even in Harmony)?  Was the existing code (for Harmony) in
> DialogClientView::UpdateDialogButton() wrong?
> 
> (2) How did web payments get in the picture?

CreateSecondaryUiBlueButton() can be called on Mac, but it checks
IsSecondaryUiMaterial() which, for tooklit-views on Mac, should* always be true.
views::BlueButton is not created when IsSecondaryUiMaterial() returns true.

*should = web payments may launch a views dialog on Mac before we launch
Harmony. So, IsSecondaryUiMaterial() may be false in that case.

[Peter Kasting, 2017-06-07 01:44:49]

On 2017/06/07 01:39:51, tapted wrote:
> On 2017/06/07 01:30:43, Peter Kasting wrote:
> > On 2017/06/07 01:18:49, tapted wrote:
> > > On 2017/06/06 18:33:01, Peter Kasting wrote:
> > > > On 2017/06/06 15:48:31, afakhry wrote:
> > > > > The height of the client bounds seems to decrease by 1 when we only
have
> a
> > > > > single cancel button. I can't repro this locally because I don't have
a
> > Mac,
> > > > but
> > > > > do you see anything that sticks out in this change?
> > > > 
> > > > That suggests that the "blue button" itself has a 1 px shorter preferred
> > > height
> > > > than the normal button, at least on Mac.  That's probably not
intentional.
> > > 
> > > I think it's shadows. views::BlueButton unconditionally applies shadows in
> > > BlueButton::ResetColorsFromNativeTheme(), but LabelButton uses
> > > PlatformStyle::ApplyLabelButtonTextStyle(..).
> > 
> > That makes sense.  (Also says my analysis was backwards -- BlueButton is
> taller,
> > not shorter.)
> > 
> > > So on mac, if there is not at least one BlueButton (`Cancel` should never
be
> a
> > > default button on Mac), then the dialog is smaller. We've been mostly
> working
> > > with "BlueButton should never appear on Mac, just prominent MdTexButton",
so
> > > this shouldn't matter in the long run, but maybe web-payments want one and
> it
> > > would be odd for shadows to be on the blue buttons, but not regular
buttons.
> > 
> > (1) What do you mean by "BlueButton should never appear on Mac, just
prominent
> > MdTexButton"?  Are you saying CreateSecondaryUiBlueButton() should never be
> > called on Mac (even in Harmony)?  Was the existing code (for Harmony) in
> > DialogClientView::UpdateDialogButton() wrong?
> > 
> > (2) How did web payments get in the picture?
> 
> CreateSecondaryUiBlueButton() can be called on Mac, but it checks
> IsSecondaryUiMaterial() which, for tooklit-views on Mac, should* always be
true.
> views::BlueButton is not created when IsSecondaryUiMaterial() returns true.
> 
> *should = web payments may launch a views dialog on Mac before we launch
> Harmony. So, IsSecondaryUiMaterial() may be false in that case.

Ah.  I get it, on both parts.

I wonder if we should just hack CreateSecondaryUiBlueButton() to always use
MdTextButton #if OS_MACOSX.  That would mean that web payments would launch a
Harmony-style button early, instead of launching BlueButton early.  That seems
better.  WDYT?

[tapted, 2017-06-07 01:55:53]

On 2017/06/07 01:44:49, Peter Kasting wrote:
> On 2017/06/07 01:39:51, tapted wrote:
> > On 2017/06/07 01:30:43, Peter Kasting wrote:
> > > On 2017/06/07 01:18:49, tapted wrote:
> > > > On 2017/06/06 18:33:01, Peter Kasting wrote:
> > > > > On 2017/06/06 15:48:31, afakhry wrote:
> > > > > > The height of the client bounds seems to decrease by 1 when we only
> have
> > a
> > > > > > single cancel button. I can't repro this locally because I don't
have
> a
> > > Mac,
> > > > > but
> > > > > > do you see anything that sticks out in this change?
> > > > > 
> > > > > That suggests that the "blue button" itself has a 1 px shorter
preferred
> > > > height
> > > > > than the normal button, at least on Mac.  That's probably not
> intentional.
> > > > 
> > > > I think it's shadows. views::BlueButton unconditionally applies shadows
in
> > > > BlueButton::ResetColorsFromNativeTheme(), but LabelButton uses
> > > > PlatformStyle::ApplyLabelButtonTextStyle(..).
> > > 
> > > That makes sense.  (Also says my analysis was backwards -- BlueButton is
> > taller,
> > > not shorter.)
> > > 
> > > > So on mac, if there is not at least one BlueButton (`Cancel` should
never
> be
> > a
> > > > default button on Mac), then the dialog is smaller. We've been mostly
> > working
> > > > with "BlueButton should never appear on Mac, just prominent
MdTexButton",
> so
> > > > this shouldn't matter in the long run, but maybe web-payments want one
and
> > it
> > > > would be odd for shadows to be on the blue buttons, but not regular
> buttons.
> > > 
> > > (1) What do you mean by "BlueButton should never appear on Mac, just
> prominent
> > > MdTexButton"?  Are you saying CreateSecondaryUiBlueButton() should never
be
> > > called on Mac (even in Harmony)?  Was the existing code (for Harmony) in
> > > DialogClientView::UpdateDialogButton() wrong?
> > > 
> > > (2) How did web payments get in the picture?
> > 
> > CreateSecondaryUiBlueButton() can be called on Mac, but it checks
> > IsSecondaryUiMaterial() which, for tooklit-views on Mac, should* always be
> true.
> > views::BlueButton is not created when IsSecondaryUiMaterial() returns true.
> > 
> > *should = web payments may launch a views dialog on Mac before we launch
> > Harmony. So, IsSecondaryUiMaterial() may be false in that case.
> 
> Ah.  I get it, on both parts.
> 
> I wonder if we should just hack CreateSecondaryUiBlueButton() to always use
> MdTextButton #if OS_MACOSX.  That would mean that web payments would launch a
> Harmony-style button early, instead of launching BlueButton early.  That seems
> better.  WDYT?

We'd probably want to tweak CreateSecondaryUiButton() as well, so the buttons
are consistent. But I think it would work.

There's already precedent for MdTextButton outside of Harmony (and once you have
an MdTextButton its behaviour shouldn't change with/without harmony), so I don't
think this would expose too many obscure codepaths.

(another option is decoupling the logic on Mac that shows toolkit-views dialogs
-- i.e. the MD ones -- from the logic that configures toolkit-views to use the
MD codepaths.. but I like that less at this stage. Likeness subject to change if
other things come up.)

[Peter Kasting, 2017-06-07 01:59:16]

On 2017/06/07 01:55:53, tapted wrote:
> On 2017/06/07 01:44:49, Peter Kasting wrote:
> > On 2017/06/07 01:39:51, tapted wrote:
> > > On 2017/06/07 01:30:43, Peter Kasting wrote:
> > > > On 2017/06/07 01:18:49, tapted wrote:
> > > > > On 2017/06/06 18:33:01, Peter Kasting wrote:
> > > > > > On 2017/06/06 15:48:31, afakhry wrote:
> > > > > > > The height of the client bounds seems to decrease by 1 when we
only
> > have
> > > a
> > > > > > > single cancel button. I can't repro this locally because I don't
> have
> > a
> > > > Mac,
> > > > > > but
> > > > > > > do you see anything that sticks out in this change?
> > > > > > 
> > > > > > That suggests that the "blue button" itself has a 1 px shorter
> preferred
> > > > > height
> > > > > > than the normal button, at least on Mac.  That's probably not
> > intentional.
> > > > > 
> > > > > I think it's shadows. views::BlueButton unconditionally applies
shadows
> in
> > > > > BlueButton::ResetColorsFromNativeTheme(), but LabelButton uses
> > > > > PlatformStyle::ApplyLabelButtonTextStyle(..).
> > > > 
> > > > That makes sense.  (Also says my analysis was backwards -- BlueButton is
> > > taller,
> > > > not shorter.)
> > > > 
> > > > > So on mac, if there is not at least one BlueButton (`Cancel` should
> never
> > be
> > > a
> > > > > default button on Mac), then the dialog is smaller. We've been mostly
> > > working
> > > > > with "BlueButton should never appear on Mac, just prominent
> MdTexButton",
> > so
> > > > > this shouldn't matter in the long run, but maybe web-payments want one
> and
> > > it
> > > > > would be odd for shadows to be on the blue buttons, but not regular
> > buttons.
> > > > 
> > > > (1) What do you mean by "BlueButton should never appear on Mac, just
> > prominent
> > > > MdTexButton"?  Are you saying CreateSecondaryUiBlueButton() should never
> be
> > > > called on Mac (even in Harmony)?  Was the existing code (for Harmony) in
> > > > DialogClientView::UpdateDialogButton() wrong?
> > > > 
> > > > (2) How did web payments get in the picture?
> > > 
> > > CreateSecondaryUiBlueButton() can be called on Mac, but it checks
> > > IsSecondaryUiMaterial() which, for tooklit-views on Mac, should* always be
> > true.
> > > views::BlueButton is not created when IsSecondaryUiMaterial() returns
true.
> > > 
> > > *should = web payments may launch a views dialog on Mac before we launch
> > > Harmony. So, IsSecondaryUiMaterial() may be false in that case.
> > 
> > Ah.  I get it, on both parts.
> > 
> > I wonder if we should just hack CreateSecondaryUiBlueButton() to always use
> > MdTextButton #if OS_MACOSX.  That would mean that web payments would launch
a
> > Harmony-style button early, instead of launching BlueButton early.  That
seems
> > better.  WDYT?
> 
> We'd probably want to tweak CreateSecondaryUiButton() as well, so the buttons
> are consistent. But I think it would work.

Ah, yeah.

> There's already precedent for MdTextButton outside of Harmony (and once you
have
> an MdTextButton its behaviour shouldn't change with/without harmony), so I
don't
> think this would expose too many obscure codepaths.

Would it also avoid the test failure here (because the problem is only in
BlueButton)?  If so, then while everything we said about BlueButton is still
true, maybe it's not worth trying to fix, because it's moot as of Harmony
getting turned on by default.

[tapted, 2017-06-07 02:45:33]

On 2017/06/07 01:59:16, Peter Kasting wrote:
> On 2017/06/07 01:55:53, tapted wrote:
> > On 2017/06/07 01:44:49, Peter Kasting wrote:
> > > On 2017/06/07 01:39:51, tapted wrote:
> > > > On 2017/06/07 01:30:43, Peter Kasting wrote:
> > > > > On 2017/06/07 01:18:49, tapted wrote:
> > > > > > On 2017/06/06 18:33:01, Peter Kasting wrote:
> > > > > > > On 2017/06/06 15:48:31, afakhry wrote:
> > > > > > > > The height of the client bounds seems to decrease by 1 when we
> only
> > > have
> > > > a
> > > > > > > > single cancel button. I can't repro this locally because I don't
> > have
> > > a
> > > > > Mac,
> > > > > > > but
> > > > > > > > do you see anything that sticks out in this change?
> > > > > > > 
> > > > > > > That suggests that the "blue button" itself has a 1 px shorter
> > preferred
> > > > > > height
> > > > > > > than the normal button, at least on Mac.  That's probably not
> > > intentional.
> > > > > > 
> > > > > > I think it's shadows. views::BlueButton unconditionally applies
> shadows
> > in
> > > > > > BlueButton::ResetColorsFromNativeTheme(), but LabelButton uses
> > > > > > PlatformStyle::ApplyLabelButtonTextStyle(..).
> > > > > 
> > > > > That makes sense.  (Also says my analysis was backwards -- BlueButton
is
> > > > taller,
> > > > > not shorter.)
> > > > > 
> > > > > > So on mac, if there is not at least one BlueButton (`Cancel` should
> > never
> > > be
> > > > a
> > > > > > default button on Mac), then the dialog is smaller. We've been
mostly
> > > > working
> > > > > > with "BlueButton should never appear on Mac, just prominent
> > MdTexButton",
> > > so
> > > > > > this shouldn't matter in the long run, but maybe web-payments want
one
> > and
> > > > it
> > > > > > would be odd for shadows to be on the blue buttons, but not regular
> > > buttons.
> > > > > 
> > > > > (1) What do you mean by "BlueButton should never appear on Mac, just
> > > prominent
> > > > > MdTexButton"?  Are you saying CreateSecondaryUiBlueButton() should
never
> > be
> > > > > called on Mac (even in Harmony)?  Was the existing code (for Harmony)
in
> > > > > DialogClientView::UpdateDialogButton() wrong?
> > > > > 
> > > > > (2) How did web payments get in the picture?
> > > > 
> > > > CreateSecondaryUiBlueButton() can be called on Mac, but it checks
> > > > IsSecondaryUiMaterial() which, for tooklit-views on Mac, should* always
be
> > > true.
> > > > views::BlueButton is not created when IsSecondaryUiMaterial() returns
> true.
> > > > 
> > > > *should = web payments may launch a views dialog on Mac before we launch
> > > > Harmony. So, IsSecondaryUiMaterial() may be false in that case.
> > > 
> > > Ah.  I get it, on both parts.
> > > 
> > > I wonder if we should just hack CreateSecondaryUiBlueButton() to always
use
> > > MdTextButton #if OS_MACOSX.  That would mean that web payments would
launch
> a
> > > Harmony-style button early, instead of launching BlueButton early.  That
> seems
> > > better.  WDYT?
> > 
> > We'd probably want to tweak CreateSecondaryUiButton() as well, so the
buttons
> > are consistent. But I think it would work.
> 
> Ah, yeah.
> 
> > There's already precedent for MdTextButton outside of Harmony (and once you
> have
> > an MdTextButton its behaviour shouldn't change with/without harmony), so I
> don't
> > think this would expose too many obscure codepaths.
> 
> Would it also avoid the test failure here (because the problem is only in
> BlueButton)?  If so, then while everything we said about BlueButton is still
> true, maybe it's not worth trying to fix, because it's moot as of Harmony
> getting turned on by default.

Yes - I think adding some #if defined(OS_MACOSX) to md_text_button.cc should get
you past the failing tests. 

Although, if my reasoning is sound, it could leave dialogs on Desktop Linux with
inconsistent text shadows if they have a mix of blue and not-blue buttons. (And
I think this CL puts blue buttons on more dialogs?). But I've lost track of the
things we do to buttons on Desktop Linux - maybe they are fine.

[Peter Kasting, 2017-06-07 02:49:32]

On 2017/06/07 02:45:33, tapted wrote:
> Although, if my reasoning is sound, it could leave dialogs on Desktop Linux
with
> inconsistent text shadows if they have a mix of blue and not-blue buttons.

Right; and to be explicit, that's already true today.

> (And
> I think this CL puts blue buttons on more dialogs?).

Correct.  So we'd be making this problem more widespread.

My suggestion was "since this only persists until Harmony launches, it may not
be worth the effort to try and fix it".  The inconsistent shadow is annoying,
but it seems pretty minor to live with for a couple releases.

I think we're good to move forward with this plan.

[afakhry, 2017-06-08 16:03:43]

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

[commit-bot: I haz the power, 2017-06-08 16:04:06]

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-06-08 17:18:42]

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

[commit-bot: I haz the power, 2017-06-08 17:18:43]

Dry run: This issue passed the CQ dry run.

[afakhry, 2017-06-08 17:28:25]

Hi tapted and pkasting, Thank you both for your suggestions.

The suggested solution seems to have fixed the failing test.

Can you please take a final look? Thanks!

[tapted, 2017-06-09 01:43:48]

https://codereview.chromium.org/2922263002/diff/120001/ui/views/controls/butt...
File ui/views/controls/button/md_text_button.cc (right):

https://codereview.chromium.org/2922263002/diff/120001/ui/views/controls/butt...
ui/views/controls/button/md_text_button.cc:36: #endif  // !defined(OS_MACOSX)
I try to avoid #ifdefs intertwining with branching. I suggest a helper:

bool IsMaterialDefault() {
#if defined(OS_MACOSX)
  return true;
#else
  return ui::MaterialDesignController::IsSecondaryUiMaterial();
#endif
}

https://codereview.chromium.org/2922263002/diff/120001/ui/views/controls/butt...
ui/views/controls/button/md_text_button.cc:52:
ui::MaterialDesignController::IsSecondaryUiMaterial());
and here pass IsMaterialDefault()

https://codereview.chromium.org/2922263002/diff/120001/ui/views/controls/butt...
ui/views/controls/button/md_text_button.cc:60: if
(ui::MaterialDesignController::IsSecondaryUiMaterial()) {
also here. Then remove all the #if.

[Peter Kasting, 2017-06-09 02:21:47]

LGTM with Trent's comments (naming tweak suggested below).

https://codereview.chromium.org/2922263002/diff/120001/ui/views/controls/butt...
File ui/views/controls/button/md_text_button.cc (right):

https://codereview.chromium.org/2922263002/diff/120001/ui/views/controls/butt...
ui/views/controls/button/md_text_button.cc:36: #endif  // !defined(OS_MACOSX)
On 2017/06/09 01:43:48, tapted wrote:
> I try to avoid #ifdefs intertwining with branching. I suggest a helper:
> 
> bool IsMaterialDefault() {
> #if defined(OS_MACOSX)
>   return true;
> #else
>   return ui::MaterialDesignController::IsSecondaryUiMaterial();
> #endif
> }

This, but I would call it UseMaterialSecondaryButtons().

[afakhry, 2017-06-09 17:32:11]

https://codereview.chromium.org/2922263002/diff/120001/ui/views/controls/butt...
File ui/views/controls/button/md_text_button.cc (right):

https://codereview.chromium.org/2922263002/diff/120001/ui/views/controls/butt...
ui/views/controls/button/md_text_button.cc:36: #endif  // !defined(OS_MACOSX)
On 2017/06/09 01:43:48, tapted wrote:
> I try to avoid #ifdefs intertwining with branching. I suggest a helper:
> 
> bool IsMaterialDefault() {
> #if defined(OS_MACOSX)
>   return true;
> #else
>   return ui::MaterialDesignController::IsSecondaryUiMaterial();
> #endif
> }

Done.

https://codereview.chromium.org/2922263002/diff/120001/ui/views/controls/butt...
ui/views/controls/button/md_text_button.cc:36: #endif  // !defined(OS_MACOSX)
On 2017/06/09 02:21:47, Peter Kasting (busy Wed-Thu) wrote:
> On 2017/06/09 01:43:48, tapted wrote:
> > I try to avoid #ifdefs intertwining with branching. I suggest a helper:
> > 
> > bool IsMaterialDefault() {
> > #if defined(OS_MACOSX)
> >   return true;
> > #else
> >   return ui::MaterialDesignController::IsSecondaryUiMaterial();
> > #endif
> > }
> 
> This, but I would call it UseMaterialSecondaryButtons().

Done.

https://codereview.chromium.org/2922263002/diff/120001/ui/views/controls/butt...
ui/views/controls/button/md_text_button.cc:52:
ui::MaterialDesignController::IsSecondaryUiMaterial());
On 2017/06/09 01:43:48, tapted wrote:
> and here pass IsMaterialDefault()

Done.

https://codereview.chromium.org/2922263002/diff/120001/ui/views/controls/butt...
ui/views/controls/button/md_text_button.cc:60: if
(ui::MaterialDesignController::IsSecondaryUiMaterial()) {
On 2017/06/09 01:43:48, tapted wrote:
> also here. Then remove all the #if.

Done.

[afakhry, 2017-06-09 17:33:21]

The CQ bit was checked by afakhry@chromium.org

[afakhry, 2017-06-09 17:33:21]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2922263002/#ps140001
(title: "tapted's and pkasting's comments")

[commit-bot: I haz the power, 2017-06-09 17:33: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-06-09 18:48:07]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1497029601045470,
"parent_rev": "4f8c28c6e0df46d5ee783b4e1f48f7dba96e5b1e", "commit_rev":
"bfb873220a103ccee2cd0480782a7a238c55c95d"}

[commit-bot: I haz the power, 2017-06-09 18:48:22]

Description was changed from

==========
Fix style of profile error dialog.

Profile error dialog should show a blue button to match a material
design look.

BUG=729749
==========

to

==========
Fix style of profile error dialog.

Profile error dialog should show a blue button to match a material
design look.

BUG=729749

Review-Url: https://codereview.chromium.org/2922263002
Cr-Commit-Position: refs/heads/master@{#478352}
Committed:
https://chromium.googlesource.com/chromium/src/+/bfb873220a103ccee2cd0480782a...
==========

[commit-bot: I haz the power, 2017-06-09 18:48:23]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/bfb873220a103ccee2cd0480782a...