[webkit-reviews] review requested: [Bug 86882] [chromium] Make sure that render surfaces are not pixel doubled with a device scale factor of 2 : [Attachment 146920] 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 146920: Patch
https://bugs.webkit.org/attachment.cgi?id=146920&action=review

------- Additional Comments from vollick at chromium.org
(In reply to comment #17)
> (From update of attachment 146638 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=146638&action=review
>
> I like this patch a *lot* better.
>
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1517
> > +	 IntRect scaledScissorRect = scissorRect;
> > +	 scaledScissorRect.scale(settings().deviceScaleFactor);
>
> Is deviceScaleFactor always an integer? If no, how does this scissor math
work out when it's not?
Good question. It may eventually be a non-integer, and may cause rounding
issues. I've converted the rects being scaled to FloatRects, but these values
are certainly squashed back into integers, currently. We will need to make the
scissor rect use float rects, but this feels like a separate change. I will
open a bug for this.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:125
> > +	 contentSize.scale(layerRenderer->settings().deviceScaleFactor);
> > +	 ASSERT(m_contentSize == contentSize);
>
> This is kind of awkward.  Maybe you should just compute m_contentSize in
setContentRect.
The reason for the awkwardness is that we need the settings from the LRC to get
the scale factor, and we done have it in setContentRect.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.h:151
> > +	 // FIXME: This should be renamed since it is no longer in physical
pixels.
>
> This seems like a small change.  Can you just do this in this patch?
Actually, it started to get pretty large and was making it tough to find the
'real' change in the patch.
Shawn mentioned some other names that should be clarified. Maybe it would be
useful to have a patch to clean up all the names and in the description of the
patch gave a little glossary of the terms we use?