Draw the text area resizer with drawing commands

Draw the text area resizer with drawing commands

We were drawing the text area resizer using an image scaled
to either 100% or 200% depending on the device scale factor.
This leads to a resizer that is too small at 190% DPI, for example,
and seems to sometimes draw too small on standard 2x DPI devices.

This patch refactors the code to paint the resizer using lines,
rather than an image. This results in correct behavior for any scale.

R=fmalita@chromium.org, peter@chromium.org
BUG=599225
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

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

[Stephen Chennney, 2017-04-25 20:53:00]

Description was changed from

==========
Draw the text area resizer with drawing commands

We were drawing the text area resizer using an image scaled
to either 100% or 200% depending on the device scale factor.
This leads to a resizer that is too small at 190% DPI, for example,
and seems to sometimes draw too small on standard 2x DPI devices.

This patch refactors the code to paint the resizer using lines,
rather than an image. This results in correct behavior for any scale.

R=fmalita@chromium.org, peter@chromium.org
BUG=599225
==========

to

==========
Draw the text area resizer with drawing commands

We were drawing the text area resizer using an image scaled
to either 100% or 200% depending on the device scale factor.
This leads to a resizer that is too small at 190% DPI, for example,
and seems to sometimes draw too small on standard 2x DPI devices.

This patch refactors the code to paint the resizer using lines,
rather than an image. This results in correct behavior for any scale.

R=fmalita@chromium.org, peter@chromium.org
BUG=599225
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Stephen Chennney, 2017-04-25 20:55:28]

Not super clean because GraphicsContext doesn't support arbitrary oriented line
drawing (despite the existence of GraphicsContext:DrawLine).

peter@, need approval for removing all the content code for the resizer image.

https://codereview.chromium.org/2835203005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/ScrollableAreaPainter.cpp (right):

https://codereview.chromium.org/2835203005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/ScrollableAreaPainter.cpp:99:
resizer_corner_rect.Width() / 2,
While this seems like a roundabout way to do it, it does result in an exact 45
degree line under any circumstances. And hence it matches the old results.

https://codereview.chromium.org/2835203005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/ScrollableAreaPainter.cpp:106:
resizer_corner_rect.Width() * 3 / 4);
Ditto.

[Stephen Chennney, 2017-04-25 20:55:50]

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

[commit-bot: I haz the power, 2017-04-25 20:56:25]

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-04-25 20:59:11]

Actually, there's something wrong here. It doesn't seem to be scaling or zooming
at all.

[commit-bot: I haz the power, 2017-04-25 21:19:13]

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

[commit-bot: I haz the power, 2017-04-25 21:19:14]

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-04-26 15:46:30]

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

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

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-04-26 16:54:37]

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

[commit-bot: I haz the power, 2017-04-26 16:54:38]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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-04-26 17:56:34]

https://codereview.chromium.org/2835203005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/ScrollableAreaPainter.cpp (right):

https://codereview.chromium.org/2835203005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/ScrollableAreaPainter.cpp:84:
context.DrawPath(line_path, paint_flags);
Can we use DrawLine instead?

(DrawPoints would be even better, but I don't think we have SkCanvas::DrawPoints
plumbed in GC)

https://codereview.chromium.org/2835203005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/ScrollableAreaPainter.cpp:109: }
I'm thinking we can merge part of these branches, and only keep the line
endpoints computation separate.  How about something along these lines

  IntPoint dark_points[4];
  IntPoint light_points[4];

  if (<logical left>) {
    <compute logical left pts>
  } else {
    <compute right pts>
  }

  context.setStrokeColor(<dark>);
  context.DrawLine(dark_points[0], dark_points[1]);
  context.DrawLine(dark_points[2], dark_points[3]);

  context.setStrokeColor(<light>);
  context.DrawLine(light_points[0], light_points[1]);
  context.DrawLine(light_points[2], light_points[3]);

[Stephen Chennney, 2017-04-26 18:16:16]

On 2017/04/26 17:56:34, f(malita) wrote:
>
https://codereview.chromium.org/2835203005/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/paint/ScrollableAreaPainter.cpp (right):
> 
>
https://codereview.chromium.org/2835203005/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/paint/ScrollableAreaPainter.cpp:84:
> context.DrawPath(line_path, paint_flags);
> Can we use DrawLine instead?
> 
> (DrawPoints would be even better, but I don't think we have
SkCanvas::DrawPoints
> plumbed in GC)

GraphicsContext::DrawLine is used only for vertical and horizontal border edges
(and one other case that's also vertical or horizontal). The code assumes so, as
I learned while doing the dashes/dots work.

> 
>
https://codereview.chromium.org/2835203005/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/paint/ScrollableAreaPainter.cpp:109: }
> I'm thinking we can merge part of these branches, and only keep the line
> endpoints computation separate.  How about something along these lines
> 
>   IntPoint dark_points[4];
>   IntPoint light_points[4];
> 
>   if (<logical left>) {
>     <compute logical left pts>
>   } else {
>     <compute right pts>
>   }
> 
>   context.setStrokeColor(<dark>);
>   context.DrawLine(dark_points[0], dark_points[1]);
>   context.DrawLine(dark_points[2], dark_points[3]);
> 
>   context.setStrokeColor(<light>);
>   context.DrawLine(light_points[0], light_points[1]);
>   context.DrawLine(light_points[2], light_points[3]);


Right, should have gone that route.

Turns out we do do the right thing with HighDPI, but it must be applied outside
this drawing code. I'll make a test to be sure.

[Stephen Chennney, 2017-04-27 15:21:14]

Description was changed from

==========
Draw the text area resizer with drawing commands

We were drawing the text area resizer using an image scaled
to either 100% or 200% depending on the device scale factor.
This leads to a resizer that is too small at 190% DPI, for example,
and seems to sometimes draw too small on standard 2x DPI devices.

This patch refactors the code to paint the resizer using lines,
rather than an image. This results in correct behavior for any scale.

R=fmalita@chromium.org, peter@chromium.org
BUG=599225
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Draw the text area resizer with drawing commands

We were drawing the text area resizer using an image scaled
to either 100% or 200% depending on the device scale factor.
This leads to a resizer that is too small at 190% DPI, for example,
and seems to sometimes draw too small on standard 2x DPI devices.

This patch refactors the code to paint the resizer using lines,
rather than an image. This results in correct behavior for any scale.

R=fmalita@chromium.org, peter@chromium.org
BUG=599225
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Stephen Chennney, 2017-04-27 15:21:14]

schenney@chromium.org changed reviewers:
+ kinuko@chromium.org

[Stephen Chennney, 2017-04-27 15:22:11]

+kinuko for chrome and blink_platform removal of the image resource

Otherwise have added a test and refactored for easier understanding. Correct
behavior confirmed on Windows with various DPI settings.

[Stephen Chennney, 2017-04-27 15:22:21]

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

[commit-bot: I haz the power, 2017-04-27 15:22:49]

Dry run: CQ is trying da patch.

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

[Peter Beverloo, 2017-04-27 15:32:49]

lgtm

I think we can also remove the following two files now:

third_party/WebKit/public/default_100_percent/blink/textarea_resize_corner.png
third_party/WebKit/public/default_200_percent/blink/textarea_resize_corner.png

Would you also mind removing the reference to textarea_resize_corner.png from:

content/app/resources/README.txt

Thanks for doing this!

[f(malita), 2017-04-27 15:41:34]

LGTM

[commit-bot: I haz the power, 2017-04-27 15:48:56]

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

[commit-bot: I haz the power, 2017-04-27 15:48:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Stephen Chennney, 2017-04-27 17:48:11]

The CQ bit was checked by schenney@chromium.org

[Stephen Chennney, 2017-04-27 17:48:12]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
fmalita@chromium.org
Link to the patchset: https://codereview.chromium.org/2835203005/#ps60001
(title: "New baselines")

[commit-bot: I haz the power, 2017-04-27 17:48:42]

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-04-27 18:01:19]

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

[commit-bot: I haz the power, 2017-04-27 18:01:20]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Stephen Chennney, 2017-04-27 18:12:28]

schenney@chromium.org changed reviewers:
+ esprehn@chromium.org

[Stephen Chennney, 2017-04-27 18:13:22]

+esprehn, who I think has the power to approve the remaining unapproved changes.

[esprehn, 2017-04-27 18:53:20]

lgtm

[Stephen Chennney, 2017-04-27 19:10:56]

The CQ bit was checked by schenney@chromium.org

[commit-bot: I haz the power, 2017-04-27 19:11:42]

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-04-27 19:34:46]

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

[commit-bot: I haz the power, 2017-04-27 19:34:47]

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...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Stephen Chennney, 2017-04-27 19:48:32]

On 2017/04/27 19:34:47, commit-bot: I haz the power wrote:
> 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...)
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
>   linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
>   linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Still need an lgtm for the README.txt.

What's going on with goma and the linux builds?

[kinuko, 2017-04-28 13:15:48]

lgtm (looks like the bot failure is unrelated)

[Stephen Chennney, 2017-04-28 13:32:13]

The CQ bit was checked by schenney@chromium.org

[commit-bot: I haz the power, 2017-04-28 13:32:36]

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-04-28 14:35:12]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493386333230020,
"parent_rev": "8b9e9fcb9eb5c2408d8488f2f619b51ff24d0470", "commit_rev":
"d4c124aa6f93deabb5415b6463d8ac6c362e0eab"}

[commit-bot: I haz the power, 2017-04-28 14:35:22]

Description was changed from

==========
Draw the text area resizer with drawing commands

We were drawing the text area resizer using an image scaled
to either 100% or 200% depending on the device scale factor.
This leads to a resizer that is too small at 190% DPI, for example,
and seems to sometimes draw too small on standard 2x DPI devices.

This patch refactors the code to paint the resizer using lines,
rather than an image. This results in correct behavior for any scale.

R=fmalita@chromium.org, peter@chromium.org
BUG=599225
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Draw the text area resizer with drawing commands

We were drawing the text area resizer using an image scaled
to either 100% or 200% depending on the device scale factor.
This leads to a resizer that is too small at 190% DPI, for example,
and seems to sometimes draw too small on standard 2x DPI devices.

This patch refactors the code to paint the resizer using lines,
rather than an image. This results in correct behavior for any scale.

R=fmalita@chromium.org, peter@chromium.org
BUG=599225
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

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

[commit-bot: I haz the power, 2017-04-28 14:35:24]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/d4c124aa6f93deabb5415b6463d8...