[Mac][Material Design] Center elements within the toolbar.

[Mac][Material Design] Center elements within the toolbar.

This cl fixes a regression in https://codereview.chromium.org/187033300
that leaves the toolbar's elements no longer vertically centered within
the toolbar.

R=avi@chromium.org
BUG=602393
Committed: https://crrev.com/4a9a39e0a0e2bd60e1ebd81b0dcac4a3525f64e3
Cr-Commit-Position: refs/heads/master@{#386519}

[shrike, 2016-04-11 21:25:51]

PTAL

[Avi (use Gerrit), 2016-04-11 21:32:06]

https://codereview.chromium.org/1876293002/diff/1/chrome/browser/ui/cocoa/too...
File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right):

https://codereview.chromium.org/1876293002/diff/1/chrome/browser/ui/cocoa/too...
chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:362:
locationBarFrame.origin.y = NSMaxY(toolbarBounds) - 2 -
Is this "2" related to a constant somehow? An inset, an inset divided by 2?

[shrike, 2016-04-11 21:36:03]

On 2016/04/11 21:32:06, Avi wrote:
>
https://codereview.chromium.org/1876293002/diff/1/chrome/browser/ui/cocoa/too...
> File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right):
> 
>
https://codereview.chromium.org/1876293002/diff/1/chrome/browser/ui/cocoa/too...
> chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:362:
> locationBarFrame.origin.y = NSMaxY(toolbarBounds) - 2 -
> Is this "2" related to a constant somehow? An inset, an inset divided by 2?

I don't like magic numbers either, but it's not logically related to any of the
padding or inset constants I have created: the locationbar is simply 2pts down
from the top of the toolbar. I could call it kMaterialDesignElementPadding / 2
but technically the spec is not dictating this distance that way. Or I could
create a new constant just for this one case?

[Avi (use Gerrit), 2016-04-11 21:40:03]

On 2016/04/11 21:36:03, shrike wrote:
> On 2016/04/11 21:32:06, Avi wrote:
> >
>
https://codereview.chromium.org/1876293002/diff/1/chrome/browser/ui/cocoa/too...
> > File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right):
> > 
> >
>
https://codereview.chromium.org/1876293002/diff/1/chrome/browser/ui/cocoa/too...
> > chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:362:
> > locationBarFrame.origin.y = NSMaxY(toolbarBounds) - 2 -
> > Is this "2" related to a constant somehow? An inset, an inset divided by 2?
> 
> I don't like magic numbers either, but it's not logically related to any of
the
> padding or inset constants I have created: the locationbar is simply 2pts down
> from the top of the toolbar. I could call it kMaterialDesignElementPadding / 2
> but technically the spec is not dictating this distance that way. Or I could
> create a new constant just for this one case?

Can you make this a one-off constant? It would help make it clear what the
number is.

[shrike, 2016-04-11 21:44:04]

On 2016/04/11 21:40:03, Avi wrote:
> On 2016/04/11 21:36:03, shrike wrote:
> > On 2016/04/11 21:32:06, Avi wrote:
> > >
> >
>
https://codereview.chromium.org/1876293002/diff/1/chrome/browser/ui/cocoa/too...
> > > File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1876293002/diff/1/chrome/browser/ui/cocoa/too...
> > > chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:362:
> > > locationBarFrame.origin.y = NSMaxY(toolbarBounds) - 2 -
> > > Is this "2" related to a constant somehow? An inset, an inset divided by
2?
> > 
> > I don't like magic numbers either, but it's not logically related to any of
> the
> > padding or inset constants I have created: the locationbar is simply 2pts
down
> > from the top of the toolbar. I could call it kMaterialDesignElementPadding /
2
> > but technically the spec is not dictating this distance that way. Or I could
> > create a new constant just for this one case?
> 
> Can you make this a one-off constant? It would help make it clear what the
> number is.

PTAL

[Avi (use Gerrit), 2016-04-11 21:46:06]

lgtm

Thanks!

[shrike, 2016-04-11 21:46:34]

The CQ bit was checked by shrike@chromium.org

[commit-bot: I haz the power, 2016-04-11 21:47:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1876293002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1876293002/20001

[commit-bot: I haz the power, 2016-04-11 23:47:20]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-04-11 23:48:13]

Description was changed from

==========
[Mac][Material Design] Center elements within the toolbar.

This cl fixes a regression in https://codereview.chromium.org/187033300
that leaves the toolbar's elements no longer vertically centered within
the toolbar.

R=avi@chromium.org
BUG=602393
==========

to

==========
[Mac][Material Design] Center elements within the toolbar.

This cl fixes a regression in https://codereview.chromium.org/187033300
that leaves the toolbar's elements no longer vertically centered within
the toolbar.

R=avi@chromium.org
BUG=602393

Committed: https://crrev.com/4a9a39e0a0e2bd60e1ebd81b0dcac4a3525f64e3
Cr-Commit-Position: refs/heads/master@{#386519}
==========

[commit-bot: I haz the power, 2016-04-11 23:48:15]

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