[chromium] LayerChromium::setNeedsDisplay does not apply contents scale correctly

[Sami Kyostila]

LayerChromium::setNeedsDisplay() internally uses contentBounds() to calculate that the layer region needing to be repainted. The problem is that setNeedsDisplayRect() expects CSS coordinates without the contents scale applied, while contentBounds() includes the contents scale factor.

The fix is to use bounds() to get the unscaled layer bounds. This leads to a second problem, however, that TiledLayerChromium::setNeedsDisplayRect() expects to receive coordinates with the contents scale applied. It uses the given rectangle to invalidate the underlying tiles, which leads to incorrect results if the rectangle is not scaled. This problem is remedied by scaling the rectangle passed into TiledLayerChromium::invalidate().

[Sami Kyostila]

Ack, that last sentence should read "scaling the rectangle before it is passed into TiledLayerChromium::invalidate()."

[Sami Kyostila]

Created attachment 124799 [details]
Patch

[Eric Seidel (no email)]

James is your best reviewer I think.

I'll make sure to grant you EditBugs/CanConfirm too.

[James Robinson]

Comment on attachment 124799 [details]
Patch

What happens for a non-full-layer invalidation of a scaled layer?  I.e. if the layer's contents are 200x200 but it's displayed at 100x100, and WebKit sends us an invalidation rect 50x50@0x0, I think that should map to an invalidation of 50x50@0x0 on the tiles, not 25x25@0x0.  Right?

[Min Qin]

*** Bug 78760 has been marked as a duplicate of this bug. ***

[Sami Kyostila]

(In reply to comment #4)
> (From update of attachment 124799 [details])
> What happens for a non-full-layer invalidation of a scaled layer?  I.e. if the layer's contents are 200x200 but it's displayed at 100x100, and WebKit sends us an invalidation rect 50x50@0x0, I think that should map to an invalidation of 50x50@0x0 on the tiles, not 25x25@0x0.  Right?

This is a bit involved because page scale is handled differently for the root layer vs. child layers.

For the root layer, invalidations (setNeedsDisplayRect) are in scaled coordinates, because GraphicsLayer::setAppliesPageScale() is enabled, and as a result LayerChromium::contentsScale() is always 1. This means that setNeedsDisplayRect(50x50@0x0) from WebKit corresponds to 100x100@0x0 in CSS pixels, but is 50x50@0x0 in tile pixels. So assuming 100x100 pixel tiles, we would paint the upper left quadrant of the single tile backing the layer.

For child layers, WebKit's invalidation rects are given in unscaled coordinates. So in your example a setNeedsDisplayRect(50x50@0x0) from WebKit actually means the 50x50@0x0 CSS rectangle inside the layer, or 25x25@0x0 in tile pixels. Therefore we would paint 25x25@0x0 in the single backing tile.

If you think this is madness then I'd tend to agree. I hope to make things a little more consistent once the dust settles.

Note that my patch does not change behavior for the root layer since contents scale is not used there.

[Sami Kyostila]

Created attachment 127492 [details]
Interactive page scaling

Here's a patch for debugging which allows you to change the page scale interactively with 'w' and 's'.

[James Robinson]

Comment on attachment 124799 [details]
Patch

OK, thanks for the more detailed explanation.

[WebKit Review Bot]

Comment on attachment 124799 [details]
Patch

Clearing flags on attachment: 124799

Committed r108025: <http://trac.webkit.org/changeset/108025>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.