Fix infobar painting issues at fractional scales

chrome/browser/ui/views/infobars/infobar_background.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/infobars/infobar_background.h"
 
 #include "chrome/browser/ui/infobar_container_delegate.h"
 #include "chrome/browser/ui/views/infobars/infobar_view.h"
 #include "components/infobars/core/infobar.h"
 #include "third_party/skia/include/effects/SkGradientShader.h"
@@ skipping 53 matching lines @@
       gfx::Rect(0,
                 view->height() - InfoBarContainerDelegate::kSeparatorLineHeight,
                 view->width(), InfoBarContainerDelegate::kSeparatorLineHeight),
       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;
+
+  // The arrow part of the top separator. (The rest was drawn by the toolbar or
+  // the infobar above this one.)
+  SkPath stroke_path;
+  // The fill for the arrow

[Peter Kasting, 2016/07/25 22:21:21]

Nit: This comment doesn't seem to add much.  I'd probably just nuke it.  After
doing so, the declaration lines can be combined again as they were before.

[Evan Stade, 2016/07/25 23:16:48]

Done.

+  SkPath fill_path;
   SkColor separator_color = SK_ColorBLACK;
+  gfx::ScopedCanvas scoped(canvas);
+  // Undo the scale factor so we can stroke with a width of 1px (not 1dp).
+  const float dsf = canvas->UndoDeviceScaleFactor();
+  // Always scale to a whole number to make sure strokes are sharp.

[Peter Kasting, 2016/07/25 22:21:21]

Nit: Can you extend this comment to explain why floor() is a better choice than
round() (or, I guess, ceil())?  I'm not sure why, myself.

[Bret, 2016/07/25 22:35:31]

Which method to use to snap to pixels is somewhat arbitrary I think. My instinct
would be to use round but if it looks fine I don't think it's important to
change.

[Peter Kasting, 2016/07/25 22:38:27]

To be clear, I'm fine with any of them (whatever works best), but such subtle
differences usually matter and are thus worth explaining, and that goes double
anywhere that doesn't use round(), since that'd probably be the default choice
for most people and thus using something else is another indicator that the
distinction is important.

[Evan Stade, 2016/07/25 23:16:48]

good question, I didn't give it much thought beyond that was how to fix 1.5x. It
didn't fix 1.75x though so it's clearly not always right (round and ceil both
don't work for 1.5x).

I thought about it more and I concluded that based on rounding of other numbers
(view coordinates, toolbar dimensions, etc.) floor would sometimes be right and
sometimes make the fill region 1px too high, such as in the 1.75x case. Trying
to figure out if we should be using ceil instead in cases like that seems very
difficult, so I still think floor is the right call. To compensate for the 1px
of overlap we'll get in some cases, like 1.75x, I made this re-draw the top
separator.

(I also noticed that my original patchset didn't work well for the second
infobar if there are multiple infobars visible, at 1.5x, and this fixes that
situation.)

+  auto scale = [dsf](int dimension) { return std::floor(dimension * dsf); };
+  SkScalar arrow_height = scale(infobar->arrow_height());
   if (delegate) {
     separator_color = delegate->GetInfoBarSeparatorColor();
     int arrow_x;
     if (delegate->DrawInfoBarArrows(&arrow_x) && infobar->arrow_height() > 0) {
-      SkScalar arrow_fill_height = SkIntToScalar(infobar->arrow_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);
+      SkScalar arrow_half_width =
+          scale(infobar->arrow_half_width());
+      stroke_path.moveTo(scale(arrow_x) - scale(infobar->arrow_half_width()),

[Bret, 2016/07/25 22:35:31]

reuse arrow_half_width

[Evan Stade, 2016/07/25 23:23:25]

Done.

+                         arrow_height);
+      stroke_path.rLineTo(arrow_half_width, -arrow_height);
+      // Make the tip of the arrow sharp.
+      stroke_path.rLineTo(-1, 0);

[Peter Kasting, 2016/07/25 22:21:21]

I don't understand how this line works.  Doesn't this convert this:

 /\
/  \

To this?:

 _
 X
/ \

That seems wrong.  Why wouldn't leaving this rLineTo() call out be better?

[Evan Stade, 2016/07/25 23:16:48]

Your ASCII art is accurate, except the way it's rendered on screen looks more
like:

 ██
█  █

vs

 █
█ █

The latter is preferable.

+      stroke_path.rLineTo(arrow_half_width, arrow_height);
 
       fill_path = stroke_path;
       fill_path.close();
     }
   }
-  fill_path.addRect(0, SkIntToScalar(infobar->arrow_height()),
-                    SkIntToScalar(infobar->width()),
-                    SkIntToScalar(infobar->height()));
-
-  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);
-
+  fill_path.addRect(0, arrow_height, scale(infobar->width()),
+                    scale(infobar->height()));
   SkPaint fill;
   fill.setStyle(SkPaint::kFill_Style);
   fill.setColor(top_color_);
-
-  SkCanvas* canvas_skia = canvas->sk_canvas();
-  // The paths provided by the above calculations are in dp. Manually scale for
-  // px.
-  SkMatrix dsf_transform;
-  dsf_transform.setScale(SkFloatToScalar(scale), SkFloatToScalar(scale));
-  fill_path.transform(dsf_transform);
-  // In order to affect exactly the pixels we want, the fill and stroke paths
-  // need to go through the pixel centers instead of along the pixel
-  // edges/corners. Skia considers integral coordinates to be the edges between
-  // pixels, so offset by 0.5 to get to the centers.
-  fill_path.offset(SK_ScalarHalf, SK_ScalarHalf);
-  canvas_skia->drawPath(fill_path, fill);
+  canvas->DrawPath(fill_path, fill);
 
   SkPaint stroke;
   stroke.setStyle(SkPaint::kStroke_Style);
   const int kSeparatorThicknessPx = 1;
   stroke.setStrokeWidth(SkIntToScalar(kSeparatorThicknessPx));

[Bret, 2016/07/25 22:35:31]

I think the default stroke width is 1px already (while you're cleaning up
anyway).

[Evan Stade, 2016/07/25 23:23:25]

the default is 0 (hairline)

   stroke.setColor(separator_color);
-  stroke_path.transform(dsf_transform);
-  stroke_path.offset(SK_ScalarHalf, SK_ScalarHalf);
+  stroke.setAntiAlias(true);
+  canvas->DrawPath(stroke_path, stroke);
 
-  // The arrow part of the top separator. (The rest was drawn by the toolbar or
-  // the infobar above this one.)
-  stroke.setAntiAlias(true);
-  canvas_skia->drawPath(stroke_path, stroke);
   // Bottom separator.
   stroke.setAntiAlias(false);
+  gfx::SizeF view_size_px = gfx::ScaleSize(gfx::SizeF(view->size()), dsf);
   SkScalar y = SkIntToScalar(view_size_px.height() - kSeparatorThicknessPx) +
                SK_ScalarHalf;
   SkScalar w = SkIntToScalar(view_size_px.width());
-  canvas_skia->drawLine(0, y, w, y, stroke);
+  canvas->sk_canvas()->drawLine(0, y, w, y, stroke);
 }