[webkit-reviews] review requested: [Bug 86882] [chromium] Make sure that render surfaces are not pixel doubled with a device scale factor of 2 : [Attachment 143627] Patch

[bugzilla-daemon at webkit.org]

vollick at chromium.org has asked	for review:
Bug 86882: [chromium] Make sure that render surfaces are not pixel doubled with
a device scale factor of 2
https://bugs.webkit.org/show_bug.cgi?id=86882

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

------- Additional Comments from vollick at chromium.org
(In reply to comment #2)
> (From update of attachment 143565 [details])
> If we accept how WebCore gives us these scaling factors, the patch seems OK
to me.	But I spent some time thinking about this - I feel really uncomfortable
with the way that deviceScaleFactor and pageScaleFactor are being encoded as a
scaling factor inside of each layer - and then for most layers we don't even
use its own contentsScale(), but rather inherit the contentsScale() from an
ancestor.  Yes, this patch looks like it will work correctly because we have
"global" knowledge about what contentsScale() actually is for the entire
tree... but what that really means is that it shouldn't be a variable that is
unique for every layer in the first place.  Is there ever a case where
contentsScale() is unique for a subtree of the layer tree?   Looking at the
code, contentsScale() is a combination of deviceScaleFactor (totally global per
layer tree) and pageScaleFactor (also totally global per layer tree ??).  In my
opinion, if there's a variable called "contentsScale" that is stored for every
layer, then it should mean that every layer could have a unique scale for its
contents within its bounds, and that would have a very different meaning than
deviceScaleFactor and pageScaleFactor.	People can easily trip over this
confusion, costing more time and bugs.
>
> So, based on that, I wonder if maybe we should go through the trouble of
talking with WebCore guys about passing deviceScaleFactor (and pageScaleFactor
?) to the layer tree host, rather than being given to each layer.
I does seem strange to have contentsScale per layer. What would you think of
passing these values separately as parameters to calcDrawTransformsEtc?

> View in context:
https://bugs.webkit.org/attachment.cgi?id=143565&action=review
>
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:448
> > +	     transformedLayerRect = IntRect(0, 0, contentsScale *
bounds.width(), contentsScale * bounds.height());
>
> So if I'm understanding this correctly:   (1) layer that owns surface needs
to be scaled so that its pixel size when drawing will match the device hi DPI 
(2) this scale is implicitly included in the entire subtree, because this
matrix is also inherited by all children, and (3) the existing code for
computing surface size will include the correct high DPI size because of (1)
and (2).    Is this correct?

I could certainly have this wrong, but It's my understanding that the
transformedLayerRect needed to be in physical pixels so that we generate
textures of the appropriate size.

>
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:457
> > +	     surfaceOriginTransform.scale(1 / contentsScale);
>
> I think a comment is badly needed here.   At a glance, it looks like this
1/contentsScale is just canceling out the use of transformedLayerRect, making
it equivalent to using bounds.width() directly, right?	  Is this really
necessary?  Why couldn't we just keep the original translate3d using
bounds.width/height, and avoid the 1/contentsScale?

I'm a bit shaky on this too, but it looks like surfaceOriginTransform
transforms from content space to its parent's content space. Since we're always
in physical pixels, there was no need to scale by contentsScale, so I
'unscaled' by contentsScale. In any case, you're totally right about needing a
comment here.

>
> > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:1677
> > +	 parent->setIsDrawable(true);
>
> We have the LayerChromiumWithForcedDrawsContent class, that could be used
instead of calling setIsDrawable()
I needed the layers to be ContentLayerChromium's so that setContentsScale would
have an effect. I've added a helper function createDrawableContentLayerChromium
to reduce the duplicated code. Please let me know if there's a better approach.
>
> > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:1685
> > +	 child->setBounds(IntSize(10, 10));
> > +	 child->setPosition(IntPoint(2, 2));
> > +	 child->setAnchorPoint(FloatPoint(0, 0));
>
> and it should be possible to use setLayerPropertiesForTesting() for
initializing each layer.

Good call. Done.