Use high resolution markers when device scale factor > 1.5f

Use high resolution markers when device scale factor > 1.5f

It's an upstream regression introduced in Blink r151453 when non-compositing
code path was removed but the setHighResMarkers() was left out.

BUG=261123
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154533

[Nico, 2013-07-17 15:08:45]

+trchen, who authored r151453.

This should be covered by
editing/spelling/(inline-spelling|grammar)-markers-hidpi.html , why did that
test not catch this regression?

[trchen, 2013-07-17 21:04:58]

On 2013/07/17 15:08:45, Nico wrote:
> +trchen, who authored r151453.
> 
> This should be covered by
> editing/spelling/(inline-spelling|grammar)-markers-hidpi.html , why did that
> test not catch this regression?

I apologize for another regression caused by my refactoring work. :(

The layout test didn't catch this because the tests don't trigger compositing
mode. In software mode things are painted through PageWidgetDelegate::paint,
which does have gc.UseHighResMarkers(). In compositing mode, after my patch,
things are painted through GraphicsLayerClient::paintContents(). Nothing in
WebKit:: is involved.

This patch LGTM. It should also help composited contents to use high resolution
markers. Better if we can add a layout test that verifies high-res markers on
composited contents so we can avoid future regression.

[Nico, 2013-07-17 21:05:40]

On Wed, Jul 17, 2013 at 2:04 PM, <trchen@chromium.org> wrote:

> On 2013/07/17 15:08:45, Nico wrote:
>
>> +trchen, who authored r151453.
>>
>
>  This should be covered by
>> editing/spelling/(inline-**spelling|grammar)-markers-**hidpi.html , why
>> did that
>> test not catch this regression?
>>
>
> I apologize for another regression caused by my refactoring work. :(
>
> The layout test didn't catch this because the tests don't trigger
> compositing
> mode. In software mode things are painted through
> PageWidgetDelegate::paint,
> which does have gc.UseHighResMarkers(). In compositing mode, after my
> patch,
> things are painted through GraphicsLayerClient::**paintContents().
> Nothing in
> WebKit:: is involved.
>
> This patch LGTM. It should also help composited contents to use high
> resolution
> markers. Better if we can add a layout test that verifies high-res markers
> on
> composited contents so we can avoid future regression.
>

Can you suggest how to do that?


>
>
https://codereview.chromium.**org/19579003/<https://codereview.chromium.org/1...
>

[trchen, 2013-07-17 21:17:55]

On 2013/07/17 21:05:40, Nico wrote:
> On Wed, Jul 17, 2013 at 2:04 PM, <mailto:trchen@chromium.org> wrote:
> > This patch LGTM. It should also help composited contents to use high
> > resolution
> > markers. Better if we can add a layout test that verifies high-res markers
> > on
> > composited contents so we can avoid future regression.
> >
> 
> Can you suggest how to do that?

You can add style="-webkit-transform: translateZ(0);" to the test elements to
force compositing. For example:

*** inline-spelling-markers-hidpi.html  2013-04-04 18:38:33.000000000 -0700
--- inline-spelling-markers-hidpi-composited.html       2013-07-17
14:12:08.000000000 -0700
***************
*** 7,12 ****
--- 7,13 ----
      border: 1px solid black;
      white-space: nowrap;
      overflow: hidden;
+     -webkit-transform: translateZ(0);
  }
  
  .forcertl {

[Nico, 2013-07-17 21:22:18]

jiangj, can you add a composited version of the marker test?


On Wed, Jul 17, 2013 at 2:17 PM, <trchen@chromium.org> wrote:

> On 2013/07/17 21:05:40, Nico wrote:
>
>  On Wed, Jul 17, 2013 at 2:04 PM, <mailto:trchen@chromium.org> wrote:
>> > This patch LGTM. It should also help composited contents to use high
>> > resolution
>> > markers. Better if we can add a layout test that verifies high-res
>> markers
>> > on
>> > composited contents so we can avoid future regression.
>> >
>>
>
>  Can you suggest how to do that?
>>
>
> You can add style="-webkit-transform: translateZ(0);" to the test elements
> to
> force compositing. For example:
>
> *** inline-spelling-markers-hidpi.**html  2013-04-04 18:38:33.000000000
> -0700
> --- inline-spelling-markers-hidpi-**composited.html       2013-07-17
> 14:12:08.000000000 -0700
> ***************
> *** 7,12 ****
> --- 7,13 ----
>       border: 1px solid black;
>       white-space: nowrap;
>       overflow: hidden;
> +     -webkit-transform: translateZ(0);
>   }
>
>   .forcertl {
>
>
>
https://codereview.chromium.**org/19579003/<https://codereview.chromium.org/1...
>

[Jiang Jiang, 2013-07-18 05:00:24]

Sure, I will do that.

[Jiang Jiang, 2013-07-18 11:33:44]

How do I create the expected.png files for each platform? Just copy them from
the non-composited ones?

[Nico, 2013-07-18 15:44:41]

On 2013/07/18 11:33:44, Jiang wrote:
> How do I create the expected.png files for each platform? Just copy them from
> the non-composited ones?

If the expected.png for your system looks good, you'd add

Bug(jiang) path/to/test.html [ NeedsRebaseline ]

to the TestExpectations file. Don't check in any expected.png files.

There's work underway on a bot that will send you a CL with baselines if you do
this, but I'm not sure if that's up and running yet. If you don't get a CL with
baselines after 2 days of landing, you need to run `Tools/Scripts/webkit-patch
rebaseline` which will pull expectations from the bots.

[Jiang Jiang, 2013-07-18 16:10:02]

On 2013/07/18 15:44:41, Nico wrote:
> On 2013/07/18 11:33:44, Jiang wrote:
> > How do I create the expected.png files for each platform? Just copy them
from
> > the non-composited ones?
> 
> If the expected.png for your system looks good, you'd add
> 
> Bug(jiang) path/to/test.html [ NeedsRebaseline ]
> 
> to the TestExpectations file. Don't check in any expected.png files.
> 
> There's work underway on a bot that will send you a CL with baselines if you
do
> this, but I'm not sure if that's up and running yet. If you don't get a CL
with
> baselines after 2 days of landing, you need to run `Tools/Scripts/webkit-patch
> rebaseline` which will pull expectations from the bots.

OK, uploaded a new patchset with the test, is this look OK?

[Nico, 2013-07-18 17:23:03]

lgtm

https://codereview.chromium.org/19579003/diff/10001/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/19579003/diff/10001/LayoutTests/TestExpectati...
LayoutTests/TestExpectations:1755: crbug.com/261123
editing/spelling/inline-spelling-markers-hidpi-composited.html [ NeedsRebaseline
]
(fyi: if you add your line a few lines further up, you're less likely to get a
merge conflict)

[commit-bot: I haz the power, 2013-07-18 20:01:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jiangj@opera.com/19579003/10001

[commit-bot: I haz the power, 2013-07-18 22:00:17]

Change committed as 154533