Summary: | Switch RenderLayer::convertToLayerCoords to use IntPoint | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Emil A Eklund <eae> | ||||||||||
Component: | Layout and Rendering | Assignee: | Emil A Eklund <eae> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue, eric, leviw, logingx, simon.fraser | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 60318 | ||||||||||||
Attachments: |
|
Description
Emil A Eklund
2011-05-31 16:19:15 PDT
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. |