Fix the absence of padding on some UI buttons.

Fix the absence of padding on some UI buttons.

It turns out that despite what I thought some buttons are being created
as MdTextButtons even when harmony is off, for example in the infobar. So
this patch reverts some changes from crrev.com/2660553005 to ensure
behavior pre-harmony remains unchanged.

BUG=694129
Review-Url: https://codereview.chromium.org/2702403010
Cr-Commit-Position: refs/heads/master@{#452937}
Committed: https://chromium.googlesource.com/chromium/src/+/2c407ccc88aae08641fcb667277df2b08a2f251a

[Bret, 2017-02-23 22:08:40]

Description was changed from

==========
Fix the absence of padding on some UI buttons.

It turns out that despite what I thought some buttons are being created
as MdTextButtons even when harmony is off, for example the in the
infobar. So this patch reverts some changes from crrev.com/2660553005 so
that behavior pre-harmony remains unchanged.

BUG=694129
==========

to

==========
Fix the absence of padding on some UI buttons.

It turns out that despite what I thought some buttons are being created
as MdTextButtons even when harmony is off, for example in the infobar. So
this patch reverts some changes from crrev.com/2660553005 to ensure
behavior pre-harmony remains unchanged.

BUG=694129
==========

[Bret, 2017-02-23 22:08:40]

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

[Bret, 2017-02-23 22:11:02]

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

[Bret, 2017-02-23 22:11:45]

pkasting: general review
sky: OWNERS

[sky, 2017-02-24 00:36:31]

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

[sky, 2017-02-24 00:39:36]

Allen is working on redoing this code here:
https://codereview.chromium.org/2696263002/ . Any chance you could wait for that
to land? Also, see my comments in that patch. I would like to avoid adding new
constants to 	ui/views/layout/layout_constants.h.

[Bret, 2017-02-24 00:47:05]

On 2017/02/24 00:39:36, sky wrote:
> Allen is working on redoing this code here:
> https://codereview.chromium.org/2696263002/ . Any chance you could wait for
that
> to land? Also, see my comments in that patch. I would like to avoid adding new
> constants to 	ui/views/layout/layout_constants.h.

Sure, I can wait for that patch. The only reason I'm adding to
layout_constants.h is because the constants need to be shared between
layout_delegate.cc and views_delegate.cc. The plumbing is pretty annoying right
now so it'd be great to make it less so.

[Peter Kasting, 2017-02-24 02:00:50]

LGTM.  I would go ahead and land immediately, as this is a visible regression of
the shipping product, and Allen's patch may take some time before we're all
ready for it to land.

https://codereview.chromium.org/2702403010/diff/1/chrome/browser/ui/views/har...
File chrome/browser/ui/views/harmony/harmony_layout_delegate.cc (right):

https://codereview.chromium.org/2702403010/diff/1/chrome/browser/ui/views/har...
chrome/browser/ui/views/harmony/harmony_layout_delegate.cc:29: return
GetMetric(Metric::BUTTON_MINIMUM_WIDTH);
Nit: Honestly I'd find it a bit clearer to do

  case Metric::BUTTON_MINIMUM_WIDTH:
  case Metric::DIALOG_BUTTON_MINIMUM_WIDTH:
    ...

Though this kind of implies that we should do similar things below:

  case Metric::BUTTON_HORIZONTAL_PADDING:
  case Metric::DIALOG_BUTTON_MARGIN:
  case Metric::DIALOG_BUTTON_TOP_SPACING:
  ...:
    return kHarmonyLayoutUnit;

...which honestly maybe we should, since that will make it clearer post-Harmony
what enum values we could consider removing entirely.

[Bret, 2017-02-24 02:29:45]

Okay, hopefully you can live with one more constant in layout_constants for now,
sky.

https://codereview.chromium.org/2702403010/diff/1/chrome/browser/ui/views/har...
File chrome/browser/ui/views/harmony/harmony_layout_delegate.cc (right):

https://codereview.chromium.org/2702403010/diff/1/chrome/browser/ui/views/har...
chrome/browser/ui/views/harmony/harmony_layout_delegate.cc:29: return
GetMetric(Metric::BUTTON_MINIMUM_WIDTH);
On 2017/02/24 02:00:50, Peter Kasting wrote:
> Nit: Honestly I'd find it a bit clearer to do
> 
>   case Metric::BUTTON_MINIMUM_WIDTH:
>   case Metric::DIALOG_BUTTON_MINIMUM_WIDTH:
>     ...
> 
> Though this kind of implies that we should do similar things below:
> 
>   case Metric::BUTTON_HORIZONTAL_PADDING:
>   case Metric::DIALOG_BUTTON_MARGIN:
>   case Metric::DIALOG_BUTTON_TOP_SPACING:
>   ...:
>     return kHarmonyLayoutUnit;
> 
> ...which honestly maybe we should, since that will make it clearer
post-Harmony
> what enum values we could consider removing entirely.

Done, just for BUTTON_MINIMUM_WIDTH. I'll let Allen finish refactoring before I
move all the constants around.

[sky, 2017-02-24 04:15:43]

https://codereview.chromium.org/2702403010/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/harmony/layout_delegate.cc (right):

https://codereview.chromium.org/2702403010/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/harmony/layout_delegate.cc:28: return
views::kMinimumButtonWidth;
Rather than exposing the constant in views could this call back to a function
exposed on ChromeViewsDelegate to get the value from ViewsDelegate? Said
function could be private and LayoutDelegate a friend.

[Peter Kasting, 2017-02-24 06:32:31]

https://codereview.chromium.org/2702403010/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/harmony/layout_delegate.cc (right):

https://codereview.chromium.org/2702403010/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/harmony/layout_delegate.cc:28: return
views::kMinimumButtonWidth;
On 2017/02/24 04:15:43, sky wrote:
> Rather than exposing the constant in views could this call back to a function
> exposed on ChromeViewsDelegate to get the value from ViewsDelegate? Said
> function could be private and LayoutDelegate a friend.

I don't think we should do that in this patch set, because I think that will be
obsolete in Allen's patch set.  This should be thought of as a stopgap, and his
the patchset that fixes all this at once.

[sky, 2017-02-24 18:22:44]

Ok, fair enough, LGTM

[Bret, 2017-02-24 20:13:37]

The CQ bit was checked by bsep@chromium.org

[Bret, 2017-02-24 20:13:38]

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/2702403010/#ps20001
(title: "change switch")

[commit-bot: I haz the power, 2017-02-24 20:14:24]

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-02-24 21:48:59]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487967217849580,
"parent_rev": "e8cd67ca7b95d3dbe5b88888dd2391662150aa6e", "commit_rev":
"2c407ccc88aae08641fcb667277df2b08a2f251a"}

[commit-bot: I haz the power, 2017-02-24 21:49:34]

Description was changed from

==========
Fix the absence of padding on some UI buttons.

It turns out that despite what I thought some buttons are being created
as MdTextButtons even when harmony is off, for example in the infobar. So
this patch reverts some changes from crrev.com/2660553005 to ensure
behavior pre-harmony remains unchanged.

BUG=694129
==========

to

==========
Fix the absence of padding on some UI buttons.

It turns out that despite what I thought some buttons are being created
as MdTextButtons even when harmony is off, for example in the infobar. So
this patch reverts some changes from crrev.com/2660553005 to ensure
behavior pre-harmony remains unchanged.

BUG=694129

Review-Url: https://codereview.chromium.org/2702403010
Cr-Commit-Position: refs/heads/master@{#452937}
Committed:
https://chromium.googlesource.com/chromium/src/+/2c407ccc88aae08641fcb667277d...
==========

[commit-bot: I haz the power, 2017-02-24 21:49:35]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/2c407ccc88aae08641fcb667277d...