Change IconLabelBubbleView layout to specify padding beside the separator.

Change IconLabelBubbleView layout to specify padding beside the separator.

Specifying the total extra padding as before was confusing, because the value
inherently depended on the external spacing after the view.  Going this route
means that no matter how the spacing after the view changes, we won't need to
change the padding constant in sync.

This changes the net padding on either side of the separator from 9 DIP to 8
DIP, which matches the mocks.  (Note that until the omnibox text position is
tweaked, in another CL to come, it may not appear to be 8 DIP away.)

BUG=616191
TEST=none

Committed: https://crrev.com/21db7774b3bae9eec33c7e645672e9aa73d13c4d
Cr-Commit-Position: refs/heads/master@{#400165}

[Peter Kasting, 2016-06-15 21:47:04]

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

[Peter Kasting, 2016-06-15 21:47:05]

I think the reason the old value looked right to you was that the omnibox text
positioning itself is wrong (as I noted in the CL description).  After I fixed
that (in another CL), I realized this needed to be changed to match.

[Evan Stade, 2016-06-15 23:12:59]

lgtm

https://codereview.chromium.org/2064323005/diff/1/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right):

https://codereview.chromium.org/2064323005/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:29: // lay out,
so the best we can do is to reserve a whole DIP.
wouldn't the more correct thing to do be reserve 0px for scale factors >2?
Imagine 10x scale factor. You can choose between 1px too few or 9px too many.

[Peter Kasting, 2016-06-16 05:01:31]

Description was changed from

==========
Change IconLabelBubbleView layout to specify padding beside the separator.

Specifying the total extra padding as before was confusing, because the value
inherently depended on the external spacing after the view.  Going this route
means that no matter how the spacing after the view changes, we won't need to
change the padding constant in sync.

This changes the net padding on either side of the separator from 9 DIP to 8
DIP, which matches the mocks.  (Note that until the omnibox text position is
tweaked, in another CL to come, it may not appear to be 8 DIP away.)

I also centered the separator line within its DIP (but still snapped to the
pixel grid), which I think has no effect on scale factors <= 2, but will be more
correct if we go larger than that in the future.

BUG=616191
TEST=none
==========

to

==========
Change IconLabelBubbleView layout to specify padding beside the separator.

Specifying the total extra padding as before was confusing, because the value
inherently depended on the external spacing after the view.  Going this route
means that no matter how the spacing after the view changes, we won't need to
change the padding constant in sync.

This changes the net padding on either side of the separator from 9 DIP to 8
DIP, which matches the mocks.  (Note that until the omnibox text position is
tweaked, in another CL to come, it may not appear to be 8 DIP away.)

BUG=616191
TEST=none
==========

[Peter Kasting, 2016-06-16 05:12:39]

https://codereview.chromium.org/2064323005/diff/1/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right):

https://codereview.chromium.org/2064323005/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:29: // lay out,
so the best we can do is to reserve a whole DIP.
On 2016/06/15 23:12:58, Evan Stade wrote:
> wouldn't the more correct thing to do be reserve 0px for scale factors >2?
> Imagine 10x scale factor. You can choose between 1px too few or 9px too many.

Yeah, that would be better.  Changed, though this did require getting the scale
factor off the widget.

[Evan Stade, 2016-06-16 15:58:25]

I look forward to the bright shiny future when we have 10x pixel density devices

[Peter Kasting, 2016-06-16 16:10:02]

The CQ bit was checked by pkasting@chromium.org

[Peter Kasting, 2016-06-16 16:10:03]

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

[commit-bot: I haz the power, 2016-06-16 16:10:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2064323005/60001

[commit-bot: I haz the power, 2016-06-16 16:16:55]

Description was changed from

==========
Change IconLabelBubbleView layout to specify padding beside the separator.

Specifying the total extra padding as before was confusing, because the value
inherently depended on the external spacing after the view.  Going this route
means that no matter how the spacing after the view changes, we won't need to
change the padding constant in sync.

This changes the net padding on either side of the separator from 9 DIP to 8
DIP, which matches the mocks.  (Note that until the omnibox text position is
tweaked, in another CL to come, it may not appear to be 8 DIP away.)

BUG=616191
TEST=none
==========

to

==========
Change IconLabelBubbleView layout to specify padding beside the separator.

Specifying the total extra padding as before was confusing, because the value
inherently depended on the external spacing after the view.  Going this route
means that no matter how the spacing after the view changes, we won't need to
change the padding constant in sync.

This changes the net padding on either side of the separator from 9 DIP to 8
DIP, which matches the mocks.  (Note that until the omnibox text position is
tweaked, in another CL to come, it may not appear to be 8 DIP away.)

BUG=616191
TEST=none
==========

[commit-bot: I haz the power, 2016-06-16 16:16:57]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-16 16:16:59]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-16 16:18:33]

Description was changed from

==========
Change IconLabelBubbleView layout to specify padding beside the separator.

Specifying the total extra padding as before was confusing, because the value
inherently depended on the external spacing after the view.  Going this route
means that no matter how the spacing after the view changes, we won't need to
change the padding constant in sync.

This changes the net padding on either side of the separator from 9 DIP to 8
DIP, which matches the mocks.  (Note that until the omnibox text position is
tweaked, in another CL to come, it may not appear to be 8 DIP away.)

BUG=616191
TEST=none
==========

to

==========
Change IconLabelBubbleView layout to specify padding beside the separator.

Specifying the total extra padding as before was confusing, because the value
inherently depended on the external spacing after the view.  Going this route
means that no matter how the spacing after the view changes, we won't need to
change the padding constant in sync.

This changes the net padding on either side of the separator from 9 DIP to 8
DIP, which matches the mocks.  (Note that until the omnibox text position is
tweaked, in another CL to come, it may not appear to be 8 DIP away.)

BUG=616191
TEST=none

Committed: https://crrev.com/21db7774b3bae9eec33c7e645672e9aa73d13c4d
Cr-Commit-Position: refs/heads/master@{#400165}
==========

[commit-bot: I haz the power, 2016-06-16 16:18:34]

Patchset 4 (id:??) landed as
https://crrev.com/21db7774b3bae9eec33c7e645672e9aa73d13c4d
Cr-Commit-Position: refs/heads/master@{#400165}