Draw infobar arrow border with width of 1px on any scale factor.

chrome/browser/ui/views/infobars/infobar_background.cc

Index: chrome/browser/ui/views/infobars/infobar_background.cc
diff --git a/chrome/browser/ui/views/infobars/infobar_background.cc b/chrome/browser/ui/views/infobars/infobar_background.cc
index 7e9fe105f810af8ee28893f3064b0fc34bf69ece..d5cd7b13588732d1855c9256439f02e5b86b35b5 100644
--- a/chrome/browser/ui/views/infobars/infobar_background.cc
+++ b/chrome/browser/ui/views/infobars/infobar_background.cc
@@ -8,14 +8,15 @@
 #include "chrome/browser/ui/views/infobars/infobar_view.h"
 #include "components/infobars/core/infobar.h"
 #include "third_party/skia/include/effects/SkGradientShader.h"
+#include "ui/base/material_design/material_design_controller.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/color_utils.h"
+#include "ui/gfx/scoped_canvas.h"
 #include "ui/views/view.h"
 
 InfoBarBackground::InfoBarBackground(
     infobars::InfoBarDelegate::Type infobar_type)
-    : separator_color_(SK_ColorBLACK),
-      top_color_(infobars::InfoBar::GetTopColor(infobar_type)),
+    : top_color_(infobars::InfoBar::GetTopColor(infobar_type)),
       bottom_color_(infobars::InfoBar::GetBottomColor(infobar_type)) {
   SetNativeControlColor(
       color_utils::AlphaBlend(top_color_, bottom_color_, 128));
@@ -25,10 +26,21 @@ InfoBarBackground::~InfoBarBackground() {
 }
 
 void InfoBarBackground::Paint(gfx::Canvas* canvas, views::View* view) const {
+  if (ui::MaterialDesignController::IsModeMaterial()) {
+    PaintMd(canvas, view);
+    return;
+  }
+
+  InfoBarView* infobar = static_cast<InfoBarView*>(view);

[Peter Kasting, 2016/03/30 00:20:26]

Nit: Google style guide suggests declaring these "as close to the first use as
possible".

[Evan Stade, 2016/04/02 02:21:41]

Done.

+  const infobars::InfoBarContainer::Delegate* delegate =
+      infobar->container_delegate();
+  SkColor separator_color =
+      delegate ? delegate->GetInfoBarSeparatorColor() : SK_ColorBLACK;

[Peter Kasting, 2016/03/30 00:20:26]

When will the delegate be null during painting?  I can't think why that could
be.  (2 places)

[Evan Stade, 2016/04/02 02:21:41]

I don't know why it would be null. Removed.

+
   SkPoint gradient_points[2];
   gradient_points[0].iset(0, 0);
   gradient_points[1].iset(0, view->height());
-  SkColor gradient_colors[2] = { top_color_, bottom_color_ };
+  SkColor gradient_colors[2] = {top_color_, bottom_color_};
   SkPaint paint;
   paint.setStrokeWidth(
       SkIntToScalar(InfoBarContainerDelegate::kSeparatorLineHeight));
@@ -37,13 +49,11 @@ void InfoBarBackground::Paint(gfx::Canvas* canvas, views::View* view) const {
   paint.setShader(SkGradientShader::MakeLinear(
       gradient_points, gradient_colors, NULL, 2, SkShader::kClamp_TileMode));
 
-  InfoBarView* infobar = static_cast<InfoBarView*>(view);
   SkCanvas* canvas_skia = canvas->sk_canvas();
   canvas_skia->drawPath(infobar->fill_path(), paint);
 
   paint.setShader(NULL);
-  paint.setColor(SkColorSetA(separator_color_,
-                             SkColorGetA(gradient_colors[0])));
+  paint.setColor(SkColorSetA(separator_color, SkColorGetA(gradient_colors[0])));
   paint.setStyle(SkPaint::kStroke_Style);
   // Anti-alias the path so it doesn't look goofy when the edges are not at 45
   // degree angles, but don't anti-alias anything else, especially not the fill,
@@ -57,5 +67,72 @@ void InfoBarBackground::Paint(gfx::Canvas* canvas, views::View* view) const {
       gfx::Rect(0,
                 view->height() - InfoBarContainerDelegate::kSeparatorLineHeight,
                 view->width(), InfoBarContainerDelegate::kSeparatorLineHeight),
-      separator_color_);
+      separator_color);
+}
+
+void InfoBarBackground::PaintMd(gfx::Canvas* canvas, views::View* view) const {
+  InfoBarView* infobar = static_cast<InfoBarView*>(view);
+  const infobars::InfoBarContainer::Delegate* delegate =
+      infobar->container_delegate();
+  SkPath stroke_path, fill_path;
+  SkColor separator_color = SK_ColorBLACK;
+  if (delegate) {
+    separator_color = delegate->GetInfoBarSeparatorColor();
+    int arrow_x;
+    SkScalar arrow_fill_height =
+        SkIntToScalar(std::max(infobar->arrow_height(), 0));

[Peter Kasting, 2016/03/30 00:20:26]

Can arrow_height() actually return a negative value?

[Evan Stade, 2016/04/02 02:21:41]

I looked into this and can't find anywhere that sets it to a negative value.
Fixed.

+    if (delegate->DrawInfoBarArrows(&arrow_x) && arrow_fill_height) {
+      SkScalar arrow_fill_half_width =
+          SkIntToScalar(infobar->arrow_half_width());
+      stroke_path.moveTo(SkIntToScalar(arrow_x) - arrow_fill_half_width,
+                         SkIntToScalar(infobar->arrow_height()));
+      stroke_path.rLineTo(arrow_fill_half_width, -arrow_fill_height);
+      stroke_path.rLineTo(arrow_fill_half_width, arrow_fill_height);
+
+      fill_path = stroke_path;
+      fill_path.close();
+    }
+  }
+  fill_path.addRect(
+      0.0, SkIntToScalar(infobar->arrow_height()),

[Peter Kasting, 2016/03/30 00:20:26]

Nit: Unless it fails to compile, I'd use 0 instead of 0.0 here, since
technically 0.0 is a double and might require truncation, while 0 should be
promotable to either float or double.

[Evan Stade, 2016/04/02 02:21:41]

Done.

+      SkIntToScalar(infobar->width()),
+      SkIntToScalar(infobar->height() -
+                    InfoBarContainerDelegate::kSeparatorLineHeight));

[Peter Kasting, 2016/03/30 00:20:26]

Two issues:

(1) In general, I don't think we want to use kSeparatorLineHeight in MD.  That
value is in dp, and we now want to force 1 px separators.  I think we should
hardcode 1 px here, as we've done in e.g. the tabstrip, toolbar top and bottom,
etc. and mark kSeparatorLineHeight as to-be-removed when non-MD is gone.

(2) Even if we wanted to keep this, it's being used inconsistently in this
function: you're using it unscaled (so dp) here, and scaled (so px) below. 
You'd have to adjust, e.g. by scaling before adding this rect.  However, maybe
we can just remove this subtraction entirely, fill to the bottom of the rect,
and let the separator paint atop the fill region, thus avoiding the issue.

[Evan Stade, 2016/04/02 02:21:41]

ok, good point. Removed usage of this constant in MD.

+
+  gfx::ScopedCanvas scoped(canvas);
+  // Undo the scale factor so we can stroke with a width of 1px (not 1dp).
+  const float scale = canvas->UndoDeviceScaleFactor();
+  // View bounds are in dp. Manually scale for px.
+  gfx::SizeF view_size_px = gfx::ScaleSize(gfx::SizeF(view->size()), scale);
+
+  SkPaint fill;
+  fill.setStyle(SkPaint::kFill_Style);
+  fill.setColor(top_color_);
+
+  SkCanvas* canvas_skia = canvas->sk_canvas();
+  // The paths provided by |infobar| are in dp. Manually scale for px.
+  SkMatrix dsf_transform;
+  dsf_transform.setScale(SkFloatToScalar(scale), SkFloatToScalar(scale));
+  fill_path.transform(dsf_transform);
+  fill_path.offset(SK_ScalarHalf, SK_ScalarHalf);

[Peter Kasting, 2016/03/30 00:20:26]

Nit: This deserves an explanation

[Evan Stade, 2016/04/02 02:21:41]

Done.

+  canvas_skia->drawPath(fill_path, fill);
+
+  SkPaint stroke;
+  stroke.setStyle(SkPaint::kStroke_Style);
+  stroke.setStrokeWidth(
+      SkIntToScalar(InfoBarContainerDelegate::kSeparatorLineHeight));
+  stroke.setColor(separator_color);
+  stroke_path.transform(dsf_transform);
+  stroke_path.offset(SK_ScalarHalf, SK_ScalarHalf);
+
+  // Top separator (with arrow shape).

[Peter Kasting, 2016/03/30 00:20:26]

Doesn't this only draw the arrow shape, i.e. there's no actual "top separator"
being drawn here, and instead we rely on either the toolbar bottom or the
previous infobar bottom to be the "separator"?

[Evan Stade, 2016/04/02 02:21:41]

reworded

+  stroke.setAntiAlias(true);
+  canvas_skia->drawPath(stroke_path, stroke);
+  // Bottom separator.
+  stroke.setAntiAlias(false);

[Peter Kasting, 2016/03/30 00:20:26]

Nit: I think this whole bottom section can just become a call to
BrowserView::Paint1pxHorizontalLine(canvas, separator_color, view->bounds(),
true);, as long as you let the scale scoper go out of scope first.

[Evan Stade, 2016/04/02 02:21:41]

I don't think this is a huge win. We go from 4 lines to 2 lines (still have to
call canvas->Restore(), or worse, indent a large block of code above for a new
scope). It seems a little roundabout to me to un-unscale then unscale again, and
the call to Paint1PxHorizontalLine seems a bit harder to understand (the "true"
at the end is particularly mysterious until you go look up what it is).

+  SkScalar y = SkIntToScalar(view_size_px.height() -
+                             InfoBarContainerDelegate::kSeparatorLineHeight) +
+               SK_ScalarHalf;
+  SkScalar w = SkIntToScalar(view_size_px.width());
+  canvas_skia->drawLine(0, y, w, y, stroke);
 }