Bug 61818

Created attachment 95505 [details] Patch

Comment on attachment 95505 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95505&action=review This is a great change. It really cleans up a lot of little annoying expressions. Please consider the various comments before landing. > Source/WebCore/rendering/RenderLayer.cpp:920 > + transform.translate(delta.x(), delta.y()); It's too bad that translate doesn't understand IntPoint or IntSize... > Source/WebCore/rendering/RenderLayer.cpp:1118 > + location += IntSize((int)absPos.x(), (int)absPos.y()); I wonder if creating some kind of toSize adaptor would be clearer here? > Source/WebCore/rendering/RenderLayer.cpp:2565 > + transform.translateRight(delta.x(), delta.y()); Again, maybe transform.translateRight should have an IntPoint overload... > Source/WebCore/rendering/RenderLayer.cpp:2915 > + transformState->translate(offset.x(), offset.y(), HitTestingTransformState::AccumulateTransform); Ditto translate functions and IntPoint. > Source/WebCore/rendering/RenderLayerBacking.cpp:381 > + IntSize rendererOffset(parentClipRect.location().x() - delta.x(), parentClipRect.location().y() - delta.y()); Isn't the parentClipRect.location() returning an IntPoint as well? Maybe this could be: IntSize renderOffset = toSize(parentClipRect.location() - delta); ?

Comment on attachment 95505 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95505&action=review >> Source/WebCore/rendering/RenderLayer.cpp:1118 >> + location += IntSize((int)absPos.x(), (int)absPos.y()); > > I wonder if creating some kind of toSize adaptor would be clearer here? c++ style casts please. There is also a round down function iirc. >> Source/WebCore/rendering/RenderLayer.cpp:2565 >> + transform.translateRight(delta.x(), delta.y()); > > Again, maybe transform.translateRight should have an IntPoint overload... Nah. translateRight takes a dx, dy, that's just how matrix math works. :) It's possible we could give it a FloatSize overload, but that still might be iffy. >> Source/WebCore/rendering/RenderLayerBacking.cpp:381 >> + IntSize rendererOffset(parentClipRect.location().x() - delta.x(), parentClipRect.location().y() - delta.y()); > > Isn't the parentClipRect.location() returning an IntPoint as well? Maybe this could be: > > IntSize renderOffset = toSize(parentClipRect.location() - delta); > > ? You don't even need toSize for that. :) point - point = size. > Source/WebCore/rendering/RenderLayerBacking.cpp:429 > + IntRect layerBounds = IntRect(delta, IntSize(borderBox.width(), borderBox.height())); This should just be IntRect(delta, borderBox.size()), no?

Created attachment 95640 [details] Patch

Comment on attachment 95640 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95640&action=review LGTM. I think you should consider doing convertToLayerCoords(rect) in this patch or the next, as allt hese callsites will suddenly become simpler. :) > Source/WebCore/rendering/RenderLayerCompositor.cpp:1089 > + curLayer->convertToLayerCoords(layer, delta); > IntRect childRect(rect); > - childRect.move(-x, -y); > + childRect.move(-delta.x(), -delta.y()); I suspect we just need a rect version of convertToLayerCoords eventually. :) Actually, given all the places you did the same dance in this patch, it might make sense to just add convertToLayerCoords(layer, rect) which does exactly this. :)

(In reply to comment #5) > I suspect we just need a rect version of convertToLayerCoords eventually. :) Actually, given all the places you did the same dance in this patch, it might make sense to just add convertToLayerCoords(layer, rect) which does exactly this. :) That's a good point, I'll see about making that change as a part of this patch.

You are welcome to just land this and iterate further. Completely your call.

Created attachment 95647 [details] Patch for landing

Created attachment 95688 [details] Patch

Comment on attachment 95688 [details] Patch Doh, wrong bug.

The commit-queue encountered the following flaky tests while processing attachment 95647 [details]: http/tests/websocket/tests/sub-protocol.html bug 61910 (author: abarth@webkit.org) The commit-queue is continuing to process your patch.

Comment on attachment 95647 [details] Patch for landing Clearing flags on attachment: 95647 Committed r87880: <http://trac.webkit.org/changeset/87880>

All reviewed patches have been landed. Closing bug.