Summary: | Fix usage of LayoutUnits and pixel snapping in RenderLayer | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Emil A Eklund <eae> | ||||||||||
Component: | Layout and Rendering | Assignee: | Emil A Eklund <eae> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | eric, jamesr, leviw, simon.fraser, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 60318 | ||||||||||||
Attachments: |
|
Description
Emil A Eklund
2012-03-28 12:02:48 PDT
Created attachment 134379 [details]
Patch
Eric, any chance you could take a look at this one too while you are at it? 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? (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. Created attachment 134866 [details]
Patch
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. (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. Created attachment 135185 [details]
Patch for landing
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 Created attachment 135209 [details]
Patch for landing
Comment on attachment 135209 [details] Patch for landing Clearing flags on attachment: 135209 Committed r112977: <http://trac.webkit.org/changeset/112977> All reviewed patches have been landed. Closing bug. |