Anchor the infobar arrow to the LocationIconView correctly.

Anchor the infobar arrow to the LocationIconView correctly.

based on palmer's codereview.chromium.org/1576353003/

Only fixes pre-MD, doesn't attempt to get MD version correct.

BUG=570204
Committed: https://crrev.com/4cc0ce015c0544bb23b76ad3fa4efd42e1bdea8f
Cr-Commit-Position: refs/heads/master@{#371366}

[Evan Stade, 2016-01-22 23:58:14]

Description was changed from

==========
Anchor the infobar arrow to the LocationIconView correctly.

based on palmer's codereview.chromium.org/1576353003/

BUG=570204
==========

to

==========
Anchor the infobar arrow to the LocationIconView correctly.

based on palmer's codereview.chromium.org/1576353003/

Only fixes pre-MD, doesn't attempt to get MD version correct.

BUG=570204
==========

[Evan Stade, 2016-01-22 23:58:37]

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

[Evan Stade, 2016-01-22 23:58:38]

This version seems to work in terms of arrow location and tooltip.

https://codereview.chromium.org/1627543002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right):

https://codereview.chromium.org/1627543002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:50: const
views::ImageView* GetImageView() const { return image_; }
I didn't want to publicly expose a non-const pointer to image_

https://codereview.chromium.org/1627543002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (left):

https://codereview.chromium.org/1627543002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:449: const int
icon_padding = GetLayoutConstant(ICON_LABEL_VIEW_TRAILING_PADDING);
I've no idea why this calculation made sense as this padding is the horizontal
spacing (I suspect it only worked by coincidence).

[palmer, 2016-01-23 00:03:50]

Tested locally and LGTM.

[Peter Kasting, 2016-01-23 00:13:41]

LGTM with one main comment

https://codereview.chromium.org/1627543002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (left):

https://codereview.chromium.org/1627543002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:449: const int
icon_padding = GetLayoutConstant(ICON_LABEL_VIEW_TRAILING_PADDING);
On 2016/01/22 23:58:37, Evan Stade wrote:
> I've no idea why this calculation made sense as this padding is the horizontal
> spacing (I suspect it only worked by coincidence).

In pre-MD, the ICON_LABEL_VIEW_TRAILING_PADDING was calculated to match the
padding in the image, so that we'd end up with equal padding on all sides of
everything.  So reading this value gave you the height of the vertical padding
below the image.

In MD this is definitely not the case, but assuming the padding is exactly 2 px
(as the new code does by using -1 directly) is probably also unsafe.

So I think there are a couple options:
(1) Just remove the -1 and point the arrow right at the image bottom.  Assume
this won't look too bad in any mode (which may not be a good assumption).
(2) Add some sort of layout constant for the image's built-in bottom padding. 
Set this to 2 on pre-MD and <whatever> on MD.  Then use the same code that's
already here, but substitute in your constant name for
ICON_LABEL_VIEW_TRAILING_PADDING.

I like option (2) better.

https://codereview.chromium.org/1627543002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_icon_view.h (right):

https://codereview.chromium.org/1627543002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_icon_view.h:69: // True if |this|
currently displays a tooltip.
Nit: How about "True if hovering this view should display a tooltip."  (The
existing text sounds more like this is true when the tooltip is visible
onscreen.)

[Evan Stade, 2016-01-23 00:37:26]

https://codereview.chromium.org/1627543002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (left):

https://codereview.chromium.org/1627543002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:449: const int
icon_padding = GetLayoutConstant(ICON_LABEL_VIEW_TRAILING_PADDING);
On 2016/01/23 00:13:41, Peter Kasting wrote:
> On 2016/01/22 23:58:37, Evan Stade wrote:
> > I've no idea why this calculation made sense as this padding is the
horizontal
> > spacing (I suspect it only worked by coincidence).
> 
> In pre-MD, the ICON_LABEL_VIEW_TRAILING_PADDING was calculated to match the
> padding in the image, so that we'd end up with equal padding on all sides of
> everything.  So reading this value gave you the height of the vertical padding
> below the image.
> 
> In MD this is definitely not the case, but assuming the padding is exactly 2
px
> (as the new code does by using -1 directly) is probably also unsafe.
> 
> So I think there are a couple options:
> (1) Just remove the -1 and point the arrow right at the image bottom.  Assume
> this won't look too bad in any mode (which may not be a good assumption).
> (2) Add some sort of layout constant for the image's built-in bottom padding. 
> Set this to 2 on pre-MD and <whatever> on MD.  Then use the same code that's
> already here, but substitute in your constant name for
> ICON_LABEL_VIEW_TRAILING_PADDING.
> 
> I like option (2) better.

This CL is attempting to fix a regression and is destined for a merge. Since
this works for pre-MD, can your option (2) be a TODO?

Also, wouldn't we still put the constant here (as this patch does) since it's
only referenced here? The only improvement being we may need a different one for
MD and pre MD.

[Peter Kasting, 2016-01-23 01:25:29]

https://codereview.chromium.org/1627543002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (left):

https://codereview.chromium.org/1627543002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:449: const int
icon_padding = GetLayoutConstant(ICON_LABEL_VIEW_TRAILING_PADDING);
On 2016/01/23 00:37:26, Evan Stade wrote:
> On 2016/01/23 00:13:41, Peter Kasting wrote:
> > On 2016/01/22 23:58:37, Evan Stade wrote:
> > > I've no idea why this calculation made sense as this padding is the
> horizontal
> > > spacing (I suspect it only worked by coincidence).
> > 
> > In pre-MD, the ICON_LABEL_VIEW_TRAILING_PADDING was calculated to match the
> > padding in the image, so that we'd end up with equal padding on all sides of
> > everything.  So reading this value gave you the height of the vertical
padding
> > below the image.
> > 
> > In MD this is definitely not the case, but assuming the padding is exactly 2
> px
> > (as the new code does by using -1 directly) is probably also unsafe.
> > 
> > So I think there are a couple options:
> > (1) Just remove the -1 and point the arrow right at the image bottom. 
Assume
> > this won't look too bad in any mode (which may not be a good assumption).
> > (2) Add some sort of layout constant for the image's built-in bottom
padding. 
> > Set this to 2 on pre-MD and <whatever> on MD.  Then use the same code that's
> > already here, but substitute in your constant name for
> > ICON_LABEL_VIEW_TRAILING_PADDING.
> > 
> > I like option (2) better.
> 
> This CL is attempting to fix a regression and is destined for a merge. Since
> this works for pre-MD, can your option (2) be a TODO?

If you want to merge, then don't touch the calculation here at all.  It produces
the correct result for non-MD already, so there's no benefit to replacing it
with a hardcoded -1.

> Also, wouldn't we still put the constant here (as this patch does) since it's
> only referenced here? The only improvement being we may need a different one
for
> MD and pre MD.

When we have constants that differ for MD and non-MD I've been putting them in
layout_constants.cc even if they're only used in one location.

And given that the obvious constant would be "padding inside the image" (not
"net offset computed by adding 1 to the padding inside the image"), we'd still
have code that looks exactly like the existing code here, it would just have a
different constant name.

[Evan Stade, 2016-01-25 23:13:33]

reverted the icon bottom calculation for now

https://codereview.chromium.org/1627543002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_icon_view.h (right):

https://codereview.chromium.org/1627543002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_icon_view.h:69: // True if |this|
currently displays a tooltip.
On 2016/01/23 00:13:41, Peter Kasting wrote:
> Nit: How about "True if hovering this view should display a tooltip."  (The
> existing text sounds more like this is true when the tooltip is visible
> onscreen.)

Done.

[Evan Stade, 2016-01-25 23:13:38]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-01-25 23:13:38]

The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1627543002/#ps40001
(title: "pkasting feedback")

[commit-bot: I haz the power, 2016-01-25 23:16:58]

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

[commit-bot: I haz the power, 2016-01-26 00:14:24]

Description was changed from

==========
Anchor the infobar arrow to the LocationIconView correctly.

based on palmer's codereview.chromium.org/1576353003/

Only fixes pre-MD, doesn't attempt to get MD version correct.

BUG=570204
==========

to

==========
Anchor the infobar arrow to the LocationIconView correctly.

based on palmer's codereview.chromium.org/1576353003/

Only fixes pre-MD, doesn't attempt to get MD version correct.

BUG=570204
==========

[commit-bot: I haz the power, 2016-01-26 00:14:25]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-26 00:18:29]

Description was changed from

==========
Anchor the infobar arrow to the LocationIconView correctly.

based on palmer's codereview.chromium.org/1576353003/

Only fixes pre-MD, doesn't attempt to get MD version correct.

BUG=570204
==========

to

==========
Anchor the infobar arrow to the LocationIconView correctly.

based on palmer's codereview.chromium.org/1576353003/

Only fixes pre-MD, doesn't attempt to get MD version correct.

BUG=570204

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

[commit-bot: I haz the power, 2016-01-26 00:18:30]

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