[webkit-reviews] review denied: [Bug 71225] [chromium] composited layers are blurry with a zoom-in page scale factor : [Attachment 114983] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Hin-Chung Lam
<hclam at google.com>'s request for review:
Bug 71225: [chromium] composited layers are blurry with a zoom-in page scale
factor
https://bugs.webkit.org/show_bug.cgi?id=71225

Attachment 114983: Patch
https://bugs.webkit.org/attachment.cgi?id=114983&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=114983&action=review


This seems very close - the contentScale updating seems a bit off but
everything else seems quite nice.  Could you please upload the .pngs for at
least one platform? That's very useful for the gardener or whoever rebaselines
the tests (assuming it isn't you) to have a baseline expectation of what the
test should look like so they can eyeball if it's failing horribly on the other
platforms or not.

> Source/WebCore/platform/graphics/chromium/CanvasLayerTextureUpdater.cpp:63
> +	       int x = floor(contentRect.x() * invScale);
> +	       int y = floor(contentRect.y() * invScale);

doing all this math component-wise is pretty verbose and runs the risk of X/Y
confusion errors that can be a pain to debug. Can you try to find the correct
combination of (Int|Rect)/(Rect|Size) methods to do this?  I think it's
something like:

scaledContentRect = FloatRect(contentRect).scale(invScale).enclosingIntRect();

or something along those lines (don't take my word for it, please test)

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:195
> +    // Call this method first to assign contents scale to LayerChromium so
the painter can apply the scale transform.
> +    updateContentsScale();

this still seems weird - setting the shouldn't change the contents scale. 
contentsScale() appears to be a function of appliesPageScale(),
pageScaleFactor(), and deviceScaleFactor(). Can we recalculate the scale factor
when those things change, and not when setting the transform? we have a
deviceOrPageScaleChanged() function already that seems like a good place to
update this

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:600
> +	   updateContentsScale();

this call also confuses me a bit - why do we need to update the contentsScale
here when updating the transform hierarchy? we aren't changing the pageScale or
deviceScale here

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:687
> +    return 1.0f;

nit: for constants like this in WebKit we typically just say '1'