Bug 82498

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.