Bug 82498 - Fix usage of LayoutUnits and pixel snapping in RenderLayer
Summary: Fix usage of LayoutUnits and pixel snapping in RenderLayer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Emil A Eklund
URL:
Keywords:
Depends on:
Blocks: 60318
  Show dependency treegraph
 
Reported: 2012-03-28 12:02 PDT by Emil A Eklund
Modified: 2012-04-02 19:45 PDT (History)
5 users (show)

See Also:


Attachments
Patch (10.47 KB, patch)
2012-03-28 13:10 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (10.43 KB, patch)
2012-03-30 13:08 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch for landing (10.73 KB, patch)
2012-04-02 14:26 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch for landing (10.54 KB, patch)
2012-04-02 15:47 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Emil A Eklund 2012-03-28 12:02:48 PDT
Fix usage of subpixel types and snapping/rounding in RenderLayer in preparation for turning on subpixel layout.
Comment 1 Emil A Eklund 2012-03-28 13:10:26 PDT
Created attachment 134379 [details]
Patch
Comment 2 Emil A Eklund 2012-03-28 17:01:50 PDT
Eric, any chance you could take a look at this one too while you are at it?
Comment 3 Julien Chaffraix 2012-03-29 15:39:58 PDT
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?
Comment 4 Emil A Eklund 2012-03-30 12:22:13 PDT
(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.
Comment 5 Emil A Eklund 2012-03-30 13:08:06 PDT
Created attachment 134866 [details]
Patch
Comment 6 Julien Chaffraix 2012-04-02 13:31:48 PDT
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.
Comment 7 Emil A Eklund 2012-04-02 13:51:12 PDT
(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.
Comment 8 Emil A Eklund 2012-04-02 14:26:49 PDT
Created attachment 135185 [details]
Patch for landing
Comment 9 WebKit Review Bot 2012-04-02 15:09:51 PDT
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
Comment 10 Emil A Eklund 2012-04-02 15:47:58 PDT
Created attachment 135209 [details]
Patch for landing
Comment 11 WebKit Review Bot 2012-04-02 19:45:00 PDT
Comment on attachment 135209 [details]
Patch for landing

Clearing flags on attachment: 135209

Committed r112977: <http://trac.webkit.org/changeset/112977>
Comment 12 WebKit Review Bot 2012-04-02 19:45:09 PDT
All reviewed patches have been landed.  Closing bug.