Use an InsetMetric instead of a constant for vector button padding.

Use an InsetMetric instead of a constant for vector button padding.

R=pkasting@chromium.org
BUG=691895
Review-Url: https://codereview.chromium.org/2783023002
Cr-Commit-Position: refs/heads/master@{#460615}
Committed: https://chromium.googlesource.com/chromium/src/+/c084fc8b9db3d90930f51a2a72f38444d2a07778

[Peter Kasting, 2017-03-29 22:39:42]

LGTM

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

https://codereview.chromium.org/2783023002/diff/1/chrome/browser/ui/views/har...
chrome/browser/ui/views/harmony/harmony_layout_delegate.cc:35: // it out here.
Nit: Maybe clearer:

      constexpr int kVisibleMargin = kHarmonyLayoutUnit / 2;
      // The visible margin is based on the unpadded size, but the actual margin
      // needs to account for the padding.
      return kVisibleMargin - GetMetric(Metric::VECTOR_IMAGE_BUTTON_PADDING);

https://codereview.chromium.org/2783023002/diff/1/chrome/browser/ui/views/har...
chrome/browser/ui/views/harmony/harmony_layout_delegate.cc:63: return 4;
Nit: It seems like if we scaled up the layout unit we'd probably scale this
value up too, so maybe use kHarmonyLayoutUnit / 4?

[Bret, 2017-03-29 23:06:11]

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

https://codereview.chromium.org/2783023002/diff/1/chrome/browser/ui/views/har...
chrome/browser/ui/views/harmony/harmony_layout_delegate.cc:35: // it out here.
On 2017/03/29 22:39:42, Peter Kasting wrote:
> Nit: Maybe clearer:
> 
>       constexpr int kVisibleMargin = kHarmonyLayoutUnit / 2;
>       // The visible margin is based on the unpadded size, but the actual
margin
>       // needs to account for the padding.
>       return kVisibleMargin - GetMetric(Metric::VECTOR_IMAGE_BUTTON_PADDING);

Done.

https://codereview.chromium.org/2783023002/diff/1/chrome/browser/ui/views/har...
chrome/browser/ui/views/harmony/harmony_layout_delegate.cc:63: return 4;
On 2017/03/29 22:39:42, Peter Kasting wrote:
> Nit: It seems like if we scaled up the layout unit we'd probably scale this
> value up too, so maybe use kHarmonyLayoutUnit / 4?

I went back and forth on this. It's not really a layout unit value, since it
results in a button size of 24 dips, it's just that making the button any
smaller is problematic. I think if the layout unit was 24 for example we'd be
happy keeping it as 4, and if we changed the vector asset size we'd want to
fiddle with it. So I'm going to leave it as just "4".

[Bret, 2017-03-29 23:06:58]

The CQ bit was checked by bsep@chromium.org

[Bret, 2017-03-29 23:06:58]

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/2783023002/#ps20001
(title: "comments")

[Bret, 2017-03-29 23:08:03]

The CQ bit was unchecked by bsep@chromium.org

[Bret, 2017-03-29 23:13:13]

The CQ bit was checked by bsep@chromium.org

[Bret, 2017-03-29 23:13:13]

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/2783023002/#ps40001
(title: "revert the stuff that got mixed in from another patch")

[commit-bot: I haz the power, 2017-03-29 23:13:49]

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-03-29 23:24:50]

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

[commit-bot: I haz the power, 2017-03-29 23:24:50]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Bret, 2017-03-29 23:30:17]

Description was changed from

==========
Use an InsetMetric instead of a constant for vector button padding.

R=pkasting@chromium.org
BUG=691895
==========

to

==========
Use an InsetMetric instead of a constant for vector button padding.

R=pkasting@chromium.org
BUG=691895
==========

[Bret, 2017-03-29 23:30:17]

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

[Bret, 2017-03-29 23:30:51]

sky@: need OWNERS for ui/views

[sky, 2017-03-29 23:45:31]

https://codereview.chromium.org/2783023002/diff/40001/ui/views/layout/layout_...
File ui/views/layout/layout_constants.h (right):

https://codereview.chromium.org/2783023002/diff/40001/ui/views/layout/layout_...
ui/views/layout/layout_constants.h:96: constexpr int kVectorButtonExtraTouchSize
= 4;
If you add this constant here how do you prevent people from using it directly?
Don't you want code to use the InsetsMetric::VECTOR_IMAGE_BUTTON_PADDING?

[Peter Kasting, 2017-03-29 23:47:21]

https://codereview.chromium.org/2783023002/diff/40001/ui/views/layout/layout_...
File ui/views/layout/layout_constants.h (right):

https://codereview.chromium.org/2783023002/diff/40001/ui/views/layout/layout_...
ui/views/layout/layout_constants.h:96: constexpr int kVectorButtonExtraTouchSize
= 4;
On 2017/03/29 23:45:31, sky wrote:
> If you add this constant here how do you prevent people from using it
directly?
> Don't you want code to use the InsetsMetric::VECTOR_IMAGE_BUTTON_PADDING?

I think the answer is "Allen's refactoring lands and this file goes away, and
the layout delegate becomes the only way to get at anything".

[Bret, 2017-03-29 23:52:00]

https://codereview.chromium.org/2783023002/diff/40001/ui/views/layout/layout_...
File ui/views/layout/layout_constants.h (right):

https://codereview.chromium.org/2783023002/diff/40001/ui/views/layout/layout_...
ui/views/layout/layout_constants.h:96: constexpr int kVectorButtonExtraTouchSize
= 4;
On 2017/03/29 23:47:21, Peter Kasting wrote:
> On 2017/03/29 23:45:31, sky wrote:
> > If you add this constant here how do you prevent people from using it
> directly?
> > Don't you want code to use the InsetsMetric::VECTOR_IMAGE_BUTTON_PADDING?
> 
> I think the answer is "Allen's refactoring lands and this file goes away, and
> the layout delegate becomes the only way to get at anything".

Right, since we're moving towards deleting this file I'd rather move constants
into it so they go away as part of the refactor.

[sky, 2017-03-30 00:09:43]

Ok, LGTM

[Bret, 2017-03-30 00:18:22]

The CQ bit was checked by bsep@chromium.org

[commit-bot: I haz the power, 2017-03-30 00:19:37]

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-03-30 01:16:17]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490833102075650,
"parent_rev": "0d7709aa52ae16a4376589ff3ba9004929791113", "commit_rev":
"c084fc8b9db3d90930f51a2a72f38444d2a07778"}

[commit-bot: I haz the power, 2017-03-30 01:17:24]

Description was changed from

==========
Use an InsetMetric instead of a constant for vector button padding.

R=pkasting@chromium.org
BUG=691895
==========

to

==========
Use an InsetMetric instead of a constant for vector button padding.

R=pkasting@chromium.org
BUG=691895

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

[commit-bot: I haz the power, 2017-03-30 01:17:25]

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