Anchor the infobar arrow to the LocationIconView correctly.

chrome/browser/ui/views/location_bar/location_bar_view.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/location_bar/location_bar_view.h"
 
 #include <algorithm>
 #include <map>
 
 #include "base/i18n/rtl.h"
@@ skipping 427 matching lines @@
 void LocationBarView::SetShowFocusRect(bool show) {
   show_focus_rect_ = show;
   SchedulePaint();
 }
 
 void LocationBarView::SelectAll() {
   omnibox_view_->SelectAll(true);
 }
 
 gfx::Point LocationBarView::GetLocationBarAnchorPoint() const {
-  // The +1 in the next line creates a 1-px gap between icon and arrow tip.
-  const int icon_padding = GetLayoutConstant(ICON_LABEL_VIEW_TRAILING_PADDING);

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

I've no idea why this calculation made sense as this padding is the horizontal
spacing (I suspect it only worked by coincidence).

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

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.

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

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:28]

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.
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.

-  gfx::Point icon_bottom(
-      0, location_icon_view_->GetImageBounds().bottom() - icon_padding + 1);
-  gfx::Point icon_center(location_icon_view_->GetImageBounds().CenterPoint());
-  gfx::Point point(icon_center.x(), icon_bottom.y());
-  ConvertPointToTarget(location_icon_view_, this, &point);
+  const views::ImageView* image = location_icon_view_->GetImageView();
+  // The -1 in the next line creates a 1-px gap between icon and arrow tip.
+  int icon_bottom = image->GetImageBounds().bottom() - 1;
+  gfx::Point icon_center(image->GetImageBounds().CenterPoint());
+  gfx::Point point(icon_center.x(), icon_bottom);
+  ConvertPointToTarget(image, this, &point);
   return point;
 }
 
 int LocationBarView::GetInternalHeight(bool use_preferred_size) {
   int total_height =
       use_preferred_size ? GetPreferredSize().height() : height();
   return std::max(total_height - (VerticalPadding() * 2), 0);
 }
 
 void LocationBarView::GetOmniboxPopupPositioningInfo(
@@ skipping 881 matching lines @@
 
 void LocationBarView::AnimationEnded(const gfx::Animation* animation) {
   AnimationProgressed(animation);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // LocationBarView, private OmniboxEditController implementation:
 
 void LocationBarView::OnChanged() {
   RefreshLocationIcon();
-  location_icon_view_->ShowTooltip(!GetOmniboxView()->IsEditingOrEmpty());
+  location_icon_view_->set_show_tooltip(!GetOmniboxView()->IsEditingOrEmpty());
   Layout();
   SchedulePaint();
 }
 
 void LocationBarView::OnSetFocus() {
   GetFocusManager()->SetFocusedView(this);
 }
 
 const ToolbarModel* LocationBarView::GetToolbarModel() const {
   return delegate_->GetToolbarModel();
@@ skipping 10 matching lines @@
 // LocationBarView, private TemplateURLServiceObserver implementation:
 
 void LocationBarView::OnTemplateURLServiceChanged() {
   template_url_service_->RemoveObserver(this);
   template_url_service_ = nullptr;
   // If the browser is no longer active, let's not show the info bubble, as this
   // would make the browser the active window again.
   if (omnibox_view_ && omnibox_view_->GetWidget()->IsActive())
     ShowFirstRunBubble();
 }