Bug 82498 – Fix usage of LayoutUnits and pixel snapping in RenderLayer

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Emil A Eklund]

Fix usage of subpixel types and snapping/rounding in RenderLayer in preparation for turning on subpixel layout.

[Emil A Eklund]

Created attachment 134379 [details]
Patch

[Emil A Eklund]

Eric, any chance you could take a look at this one too while you are at it?

[Julien Chaffraix]

Comment on attachment 134379 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134379&action=review

> Source/WebCore/rendering/RenderLayer.cpp:747
> +        setSize(pixelSnappedIntSize(box->size(), box->location()));

It's kind of weird that the size - that comes from the RenderObject - is pixel snapped. Especially since we will use it back into a LayoutRect in calculateRects (which means 2 conversions). Won't we hit corner cases where the rounding / snapping could make us miss a paint or hit test? (I see that this matches the inline case as implemented now).

> Source/WebCore/rendering/RenderLayer.cpp:2257
> +    RenderBox* box = renderBox();
> +    ASSERT(box);

Nit: just inline that below and do:

return snapSizeToPixel(m_scrollSize.width(), renderBox()->clientLeft());

That would be more readable and the scroll information requires a RenderBox.

> Source/WebCore/rendering/RenderLayer.cpp:2266
> +    RenderBox* box = renderBox();
> +    ASSERT(box);

Ditto.

> Source/WebCore/rendering/RenderLayer.cpp:2600
> +        resizeControlRect = resizerCornerRect(this, box->borderBoxRect());

borderBoxRect returns a LayoutRect and resizerCornerRect takes an IntRect, is there something fishy here?

[Emil A Eklund]

(In reply to comment #3)
> (From update of attachment 134379 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134379&action=review
> 
> > Source/WebCore/rendering/RenderLayer.cpp:747
> > +        setSize(pixelSnappedIntSize(box->size(), box->location()));
> 
> It's kind of weird that the size - that comes from the RenderObject - is pixel snapped. Especially since we will use it back into a LayoutRect in calculateRects (which means 2 conversions). Won't we hit corner cases where the rounding / snapping could make us miss a paint or hit test? (I see that this matches the inline case as implemented now).

It is a bit weird but is needed to ensure that the size of the layer is aligned to pixel bounds. The extra conversions for calcRects are unfortunate I agree but adding extra complexity to avoid the conversions seems like premature optimization.


> 
> > Source/WebCore/rendering/RenderLayer.cpp:2257
> > +    RenderBox* box = renderBox();
> > +    ASSERT(box);
> 
> Nit: just inline that below and do:
> 
> return snapSizeToPixel(m_scrollSize.width(), renderBox()->clientLeft());
> 
> That would be more readable and the scroll information requires a RenderBox.

Good idea, made the change here and below.

> 
> > Source/WebCore/rendering/RenderLayer.cpp:2266
> > +    RenderBox* box = renderBox();
> > +    ASSERT(box);
> 
> Ditto.
> 
> > Source/WebCore/rendering/RenderLayer.cpp:2600
> > +        resizeControlRect = resizerCornerRect(this, box->borderBoxRect());
> 
> borderBoxRect returns a LayoutRect and resizerCornerRect takes an IntRect, is there something fishy here?

We are changing borderBoxRect to return an IntRect in one of the other outstanding patches.

[Emil A Eklund]

Created attachment 134866 [details]
Patch

[Julien Chaffraix]

Comment on attachment 134866 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134866&action=review

> Source/WebCore/rendering/RenderLayer.cpp:747
> +        setSize(pixelSnappedIntSize(box->size(), box->location()));

>> It's kind of weird that the size - that comes from the RenderObject - is pixel snapped. Especially since we will use it back into a LayoutRect in calculateRects (which means 2 conversions). Won't we hit corner cases where the rounding / snapping could make us miss a paint or hit test? (I see that this matches the inline case as implemented now).
> It is a bit weird but is needed to ensure that the size of the layer is aligned to pixel bounds. The extra conversions for calcRects are unfortunate I agree but adding extra complexity to avoid the conversions seems like premature optimization.

I am not 100% this is just an optimization and will not lead to corner cases' issues. Please put a FIXME here that we shouldn't be doing those unneeded snapping.

> Source/WebCore/rendering/RenderLayer.cpp:3143
>          hitTestArea.intersect(pixelSnappedIntRect(frameVisibleRect(renderer())));

I don't think we should pixel snap our frameVisibleRect now as hitTestArea is a LayoutRect.

[Emil A Eklund]

(In reply to comment #6)
> I am not 100% this is just an optimization and will not lead to corner cases' issues. Please put a FIXME here that we shouldn't be doing those unneeded snapping.

Will do, thanks.

> 
> > Source/WebCore/rendering/RenderLayer.cpp:3143
> >          hitTestArea.intersect(pixelSnappedIntRect(frameVisibleRect(renderer())));
> 
> I don't think we should pixel snap our frameVisibleRect now as hitTestArea is a LayoutRect.

Good point, reverted.

[Emil A Eklund]

Created attachment 135185 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 135185 [details]
Patch for landing

Rejecting attachment 135185 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ded at 3022 with fuzz 2 (offset 118 lines).
Hunk #11 FAILED at 2929.
Hunk #12 succeeded at 3069 (offset 120 lines).
Hunk #13 succeeded at 3087 with fuzz 1 (offset 121 lines).
Hunk #14 succeeded at 3269 (offset 130 lines).
Hunk #15 succeeded at 3561 (offset 130 lines).
1 out of 15 hunks FAILED -- saving rejects to file Source/WebCore/rendering/RenderLayer.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12309780

[Emil A Eklund]

Created attachment 135209 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 135209 [details]
Patch for landing

Clearing flags on attachment: 135209

Committed r112977: <http://trac.webkit.org/changeset/112977>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.