[webkit-reviews] review denied: [Bug 61818] Switch RenderLayer::convertToLayerCoords to use IntPoint : [Attachment 95505] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied  review:
Bug 61818: Switch RenderLayer::convertToLayerCoords to use IntPoint
https://bugs.webkit.org/show_bug.cgi?id=61818

Attachment 95505: Patch
https://bugs.webkit.org/attachment.cgi?id=95505&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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?