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

------- Additional Comments from vollick at chromium.org
(In reply to comment #20)
> (In reply to comment #18)
> > Created an attachment (id=146920) [details] [details]
> > Patch
> >
> > (In reply to comment #17)
> > > (From update of attachment 146638 [details] [details] [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.
>
> Yeah, you can't have float scissor rects in gl.  I guess
CClayerTreeHostCommon's subtreeShouldRenderToSeparateSurface should
theoretically catch this case, but in looking at code I don't think
isScaleOrTranslation is the right check for determining if a scissor is pixel
aligned.  Can you put a link to the bug here?
There are some glitches with non-integer device scale factors. Most have been
ironed out, but there's still at least one subtle bug to fix.

It worries me a bit that with this approach -- scaling in LRC rather than in
the scene graph -- we expose ourselves to many subtle rounding bugs. Even if we
get the current bugs fixed, I think it would be easy to introduce a new one.
There was no such risk with the old approach, as gross as it looked;
non-integer scale factors worked correctly.

I have made a dependent 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.
>
> Oh, I see.  I think the weird part to me is that the two prepare functions
aren't symmetrical. It's not obvious that the caller needs to call
prepareContentsTexture before hasValidBackgroundTexture can return true, since
the former sets m_contentSize used by the latter.
Yuck, good point. The functions are now symmetrical.
>
> > > > 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?
>
> If it's larger than it sounds, then punting is fine.	Can you file a
dependent bug to fix this particular naming issue and put a link here?	I don't
know that everything has to be renamed at once.

Great, I've made a bug for this (89212).