Bug 61818

Summary: Switch RenderLayer::convertToLayerCoords to use IntPoint
Product: WebKit Reporter: Emil A Eklund <eae>
Component: Layout and RenderingAssignee: 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 Flags
Patch
none
Patch
none
Patch for landing
none
Patch none

Description Emil A Eklund 2011-05-31 16:19:15 PDT
int x, int y to IntPoint conversion.
Comment 1 Emil A Eklund 2011-05-31 16:23:11 PDT
Created attachment 95505 [details]
Patch
Comment 2 Brent Fulgham 2011-05-31 21:32:21 PDT
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 3 Eric Seidel (no email) 2011-05-31 23:32:57 PDT
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?
Comment 4 Emil A Eklund 2011-06-01 12:19:56 PDT
Created attachment 95640 [details]
Patch
Comment 5 Eric Seidel (no email) 2011-06-01 12:25:08 PDT
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. :)
Comment 6 Emil A Eklund 2011-06-01 12:31:56 PDT
(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.
Comment 7 Eric Seidel (no email) 2011-06-01 12:51:51 PDT
You are welcome to just land this and iterate further.  Completely your call.
Comment 8 Emil A Eklund 2011-06-01 13:30:25 PDT
Created attachment 95647 [details]
Patch for landing
Comment 9 Emil A Eklund 2011-06-01 17:01:08 PDT
Created attachment 95688 [details]
Patch
Comment 10 Emil A Eklund 2011-06-01 17:02:04 PDT
Comment on attachment 95688 [details]
Patch

Doh, wrong bug.
Comment 11 WebKit Commit Bot 2011-06-01 23:39:59 PDT
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 12 WebKit Commit Bot 2011-06-01 23:41:31 PDT
Comment on attachment 95647 [details]
Patch for landing

Clearing flags on attachment: 95647

Committed r87880: <http://trac.webkit.org/changeset/87880>
Comment 13 WebKit Commit Bot 2011-06-01 23:41:38 PDT
All reviewed patches have been landed.  Closing bug.