Switch RenderLayer::convertToLayerCoords to use IntPoint

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Emil A Eklund]

int x, int y to IntPoint conversion.

[Emil A Eklund]

Created attachment 95505 [details]
Patch

[Brent Fulgham]

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);

?

[Eric Seidel (no email)]

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?

[Emil A Eklund]

Created attachment 95640 [details]
Patch

[Eric Seidel (no email)]

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. :)

[Emil A Eklund]

(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.

[Eric Seidel (no email)]

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

[Emil A Eklund]

Created attachment 95647 [details]
Patch for landing

[Emil A Eklund]

Created attachment 95688 [details]
Patch

[Emil A Eklund]

Comment on attachment 95688 [details]
Patch

Doh, wrong bug.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 95647 [details]:

http/tests/websocket/tests/sub-protocol.html bug 61910 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 95647 [details]
Patch for landing

Clearing flags on attachment: 95647

Committed r87880: <http://trac.webkit.org/changeset/87880>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.