Fix the painting of document markers under zoom and variable DPI

Fix the painting of document markers under zoom and variable DPI

The painting of document markers does not take into account zoom
or DPI settings. We do create different sized bitmaps for high DPI,
but we then scale that out to draw always at the same size.

This patch re-writes the document markers sizing code to scale the
marker with zoom (as the text scales) and to otherwise simplify
the code.

New tests for document marker painting are added. They cover various
zoom factors.

R=fmalita@chromium.org
BUG=686792
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2667363004
Cr-Commit-Position: refs/heads/master@{#448414}
Committed: https://chromium.googlesource.com/chromium/src/+/d3c22e5c574de5ee276228227967029608f4fb3c

[Stephen Chennney, 2017-02-02 16:41:32]

Waiting on new test baselines, but otherwise fixes the issue and cleans up some
poor code.

Don't ask why the tests use selection etc - it's just the series of hoops you
need to jump through to get document markers to paint in a test.

[Stephen Chennney, 2017-02-02 16:41:46]

The CQ bit was checked by schenney@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-02 16:42:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Stephen Chennney, 2017-02-02 16:42:14]

Description was changed from

==========
Fix the painting of document markers under zoom and variable DPI

The painting of document larkers does not take into account zoom
or DPI settings. We do create different sized bitmaps for high DPI,
but we then scale that out to draw always at the same size.

This patch re-writes the document markers sizing code to scale the
marker with zoom (as the text scales) and to otherwise simplify
the code.

New tests for document marker painting are added. They cover various
zoom factors. There was previously no coverage at all for marker
painting.

R=fmalita@chromium.org
BUG=686792
==========

to

==========
Fix the painting of document markers under zoom and variable DPI

The painting of document markers does not take into account zoom
or DPI settings. We do create different sized bitmaps for high DPI,
but we then scale that out to draw always at the same size.

This patch re-writes the document markers sizing code to scale the
marker with zoom (as the text scales) and to otherwise simplify
the code.

New tests for document marker painting are added. They cover various
zoom factors. There was previously no coverage at all for marker
painting.

R=fmalita@chromium.org
BUG=686792
==========

[commit-bot: I haz the power, 2017-02-02 17:45:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-02 17:45:32]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Stephen Chennney, 2017-02-02 20:56:55]

Description was changed from

==========
Fix the painting of document markers under zoom and variable DPI

The painting of document markers does not take into account zoom
or DPI settings. We do create different sized bitmaps for high DPI,
but we then scale that out to draw always at the same size.

This patch re-writes the document markers sizing code to scale the
marker with zoom (as the text scales) and to otherwise simplify
the code.

New tests for document marker painting are added. They cover various
zoom factors. There was previously no coverage at all for marker
painting.

R=fmalita@chromium.org
BUG=686792
==========

to

==========
Fix the painting of document markers under zoom and variable DPI

The painting of document markers does not take into account zoom
or DPI settings. We do create different sized bitmaps for high DPI,
but we then scale that out to draw always at the same size.

This patch re-writes the document markers sizing code to scale the
marker with zoom (as the text scales) and to otherwise simplify
the code.

New tests for document marker painting are added. They cover various
zoom factors.

R=fmalita@chromium.org
BUG=686792
==========

[Stephen Chennney, 2017-02-03 01:02:17]

Description was changed from

==========
Fix the painting of document markers under zoom and variable DPI

The painting of document markers does not take into account zoom
or DPI settings. We do create different sized bitmaps for high DPI,
but we then scale that out to draw always at the same size.

This patch re-writes the document markers sizing code to scale the
marker with zoom (as the text scales) and to otherwise simplify
the code.

New tests for document marker painting are added. They cover various
zoom factors.

R=fmalita@chromium.org
BUG=686792
==========

to

==========
Fix the painting of document markers under zoom and variable DPI

The painting of document markers does not take into account zoom
or DPI settings. We do create different sized bitmaps for high DPI,
but we then scale that out to draw always at the same size.

This patch re-writes the document markers sizing code to scale the
marker with zoom (as the text scales) and to otherwise simplify
the code.

New tests for document marker painting are added. They cover various
zoom factors.

R=fmalita@chromium.org
BUG=686792
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Stephen Chennney, 2017-02-03 01:02:31]

The CQ bit was checked by schenney@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 01:03:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-03 02:04:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-03 02:04:23]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[f(malita), 2017-02-03 15:53:55]

https://codereview.chromium.org/2667363004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/2667363004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp:527:
deviceScaleFactor == 2 ? misspellBitmap2x : misspellBitmap1x;
Looks like we're not taking |zoom| into account when selecting the bitmap, so
we're going to use the low-res version and scale it up at raster time => blurry
results on hidpi?

I presume this is a problem related to --enable-use-zoom-for-dsf, which changed
how DSF is communicated (now a zoom factor instead of m_deviceScaleFactor). 
Would it not suffice to fold it into the deviceScaleFactor calculation above? 
E.g.:

  int deviceScaleFactor = m_deviceScaleFactor * zoom > 1.5f ? 2 : 1;

[Stephen Chennney, 2017-02-03 16:27:45]

https://codereview.chromium.org/2667363004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/2667363004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp:527:
deviceScaleFactor == 2 ? misspellBitmap2x : misspellBitmap1x;
On 2017/02/03 15:53:55, f(malita) wrote:
> Looks like we're not taking |zoom| into account when selecting the bitmap, so
> we're going to use the low-res version and scale it up at raster time =>
blurry
> results on hidpi?
> 
> I presume this is a problem related to --enable-use-zoom-for-dsf, which
changed
> how DSF is communicated (now a zoom factor instead of m_deviceScaleFactor). 
> Would it not suffice to fold it into the deviceScaleFactor calculation above? 
> E.g.:
> 
>   int deviceScaleFactor = m_deviceScaleFactor * zoom > 1.5f ? 2 : 1;

That's a good idea. I'll do that.

I also thought of moving away from the bitmaps, but it seems we historical had
good reasons for doing so and I didn't want to mess with that for now. Of
course, that "good reason" may be Apple's, so I'll do some historical analysis
while I wait for baselines.

[f(malita), 2017-02-03 16:37:42]

On 2017/02/03 16:27:45, Stephen Chennney wrote:

> I also thought of moving away from the bitmaps, but it seems we historical had
> good reasons for doing so and I didn't want to mess with that for now. Of
> course, that "good reason" may be Apple's, so I'll do some historical analysis
> while I wait for baselines.

Indeed, I thought about that too.

The problem with the post-CL impl is that it will scale arbitrarily (CTRL-+), so
the results can be quite pixelated.  Before it was OK resolution-wise, but no
scaling is weird/undesirable per linked bug.

Getting away from bitmaps (another instance of record-time rasterization) seems
the only good fix for both issues.  My only concern is perf-related - you can't
beat a tiny bitmap shader.  But maybe marker rasterization perf is not high on
our priority list, so a path or path segment repeated with a picture shader
would be fine.

I'm fine with landing your current approach for now though.

[Stephen Chennney, 2017-02-04 19:42:24]

The CQ bit was checked by schenney@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-04 19:42:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-04 21:26:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-04 21:26:02]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[f(malita), 2017-02-06 14:39:12]

lgtm

https://codereview.chromium.org/2667363004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/2667363004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp:618: float
dotCount = floor(width / rowPixels);
nit: floorf

[Stephen Chennney, 2017-02-06 15:53:09]

Thanks. Now lets hope the baselines are right.

https://codereview.chromium.org/2667363004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/2667363004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp:618: float
dotCount = floor(width / rowPixels);
On 2017/02/06 14:39:12, f(malita) wrote:
> nit: floorf

Done.

[Stephen Chennney, 2017-02-06 15:53:28]

The CQ bit was checked by schenney@chromium.org

[Stephen Chennney, 2017-02-06 15:53:28]

The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org
Link to the patchset: https://codereview.chromium.org/2667363004/#ps100001
(title: "New baselines and nit fix")

[commit-bot: I haz the power, 2017-02-06 15:53:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-06 17:30:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-06 17:30:22]

Failed to apply patch for
third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp:
While running git apply --index -p1;
  error: patch failed:
third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp:606
  error: third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp: patch
does not apply

Patch:       third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp
Index: third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp
diff --git a/third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp
b/third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp
index
b7f8fa6918fe10df157761bd6d1bfae7ec446c6d..e98a156dbc41305e43f1bd7b00a9d1db0eb8549e
100644
--- a/third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp
+++ b/third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp
@@ -509,12 +509,15 @@ void GraphicsContext::drawLine(const IntPoint& point1,
const IntPoint& point2) {
 
 void GraphicsContext::drawLineForDocumentMarker(const FloatPoint& pt,
                                                 float width,
-                                                DocumentMarkerLineStyle style)
{
+                                                DocumentMarkerLineStyle style,
+                                                float zoom) {
   if (contextDisabled())
     return;
 
   // Use 2x resources for a device scale factor of 1.5 or above.
-  int deviceScaleFactor = m_deviceScaleFactor > 1.5f ? 2 : 1;
+  // This modifes the bitmaps used for the marker, not the overall
+  // scaling of the marker.
+  int deviceScaleFactor = m_deviceScaleFactor * zoom > 1.5f ? 2 : 1;
 
   // Create the pattern we'll use to draw the underline.
   int index = style == DocumentMarkerGrammarLineStyle ? 1 : 0;
@@ -584,7 +587,7 @@ void GraphicsContext::drawLineForDocumentMarker(const
FloatPoint& pt,
     misspellBitmap[index] = new SkBitmap(bitmap);
 #else
     // We use a 2-pixel-high misspelling indicator because that seems to be
-    // what WebKit is designed for, and how much room there is in a typical
+    // what Blink is designed for, and how much room there is in a typical
     // page for it.
     const int rowPixels =
         32 * deviceScaleFactor;  // Must be multiple of 4 for pattern below.
@@ -606,25 +609,24 @@ void GraphicsContext::drawLineForDocumentMarker(const
FloatPoint& pt,
   }
 
 #if OS(MACOSX)
-  SkScalar originX = WebCoreFloatToSkScalar(pt.x()) * deviceScaleFactor;
-  SkScalar originY = WebCoreFloatToSkScalar(pt.y()) * deviceScaleFactor;
-
-  // Make sure to draw only complete dots.
-  int rowPixels = misspellBitmap[index]->width();
-  float widthMod = fmodf(width * deviceScaleFactor, rowPixels);
-  if (rowPixels - widthMod > deviceScaleFactor)
-    width -= widthMod / deviceScaleFactor;
+  // Position already includes zoom and device scale factor.
+  SkScalar originX = WebCoreFloatToSkScalar(pt.x());
+  SkScalar originY = WebCoreFloatToSkScalar(pt.y());
+
+  // Make sure to draw only complete dots, and finish inside the marked text.
+  float rowPixels = misspellBitmap[index]->width() * zoom / deviceScaleFactor;
+  float dotCount = floorf(width / rowPixels);
+  width = dotCount * rowPixels;
 #else
   SkScalar originX = WebCoreFloatToSkScalar(pt.x());
 
   // Offset it vertically by 1 so that there's some space under the text.
   SkScalar originY = WebCoreFloatToSkScalar(pt.y()) + 1;
-  originX *= deviceScaleFactor;
-  originY *= deviceScaleFactor;
 #endif
 
   SkMatrix localMatrix;
-  localMatrix.setTranslate(originX, originY);
+  localMatrix.setScale(zoom / deviceScaleFactor, zoom / deviceScaleFactor);
+  localMatrix.postTranslate(originX, originY);
 
   SkPaint paint;
   paint.setShader(SkShader::MakeBitmapShader(
@@ -632,17 +634,13 @@ void GraphicsContext::drawLineForDocumentMarker(const
FloatPoint& pt,
       SkShader::kRepeat_TileMode, &localMatrix));
 
   SkRect rect;
-  rect.set(originX, originY,
-           originX + WebCoreFloatToSkScalar(width) * deviceScaleFactor,
-           originY + SkIntToScalar(misspellBitmap[index]->height()));
+  rect.set(
+      originX, originY, originX + WebCoreFloatToSkScalar(width),
+      originY +
+          SkIntToScalar(misspellBitmap[index]->height() / deviceScaleFactor) *
+              zoom);
 
-  if (deviceScaleFactor == 2) {
-    save();
-    scale(0.5, 0.5);
-  }
   drawRect(rect, paint);
-  if (deviceScaleFactor == 2)
-    restore();
 }
 
 void GraphicsContext::drawLineForText(const FloatPoint& pt, float width) {

[Stephen Chennney, 2017-02-06 18:51:58]

The CQ bit was checked by schenney@chromium.org

[Stephen Chennney, 2017-02-06 18:51:59]

The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org
Link to the patchset: https://codereview.chromium.org/2667363004/#ps120001
(title: "Rebased")

[commit-bot: I haz the power, 2017-02-06 18:52:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-06 22:25:23]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1486407118778600,
"parent_rev": "ffc3d3143b1f1220beca3b0a942c8d871baa8aa0", "commit_rev":
"d3c22e5c574de5ee276228227967029608f4fb3c"}

[commit-bot: I haz the power, 2017-02-06 22:26:04]

Description was changed from

==========
Fix the painting of document markers under zoom and variable DPI

The painting of document markers does not take into account zoom
or DPI settings. We do create different sized bitmaps for high DPI,
but we then scale that out to draw always at the same size.

This patch re-writes the document markers sizing code to scale the
marker with zoom (as the text scales) and to otherwise simplify
the code.

New tests for document marker painting are added. They cover various
zoom factors.

R=fmalita@chromium.org
BUG=686792
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Fix the painting of document markers under zoom and variable DPI

The painting of document markers does not take into account zoom
or DPI settings. We do create different sized bitmaps for high DPI,
but we then scale that out to draw always at the same size.

This patch re-writes the document markers sizing code to scale the
marker with zoom (as the text scales) and to otherwise simplify
the code.

New tests for document marker painting are added. They cover various
zoom factors.

R=fmalita@chromium.org
BUG=686792
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2667363004
Cr-Commit-Position: refs/heads/master@{#448414}
Committed:
https://chromium.googlesource.com/chromium/src/+/d3c22e5c574de5ee276228227967...
==========

[commit-bot: I haz the power, 2017-02-06 22:26:06]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/d3c22e5c574de5ee276228227967...