Bug 61414 – Replace RenderLayer::x/y/width/height with location/size

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Emil A Eklund]

Replace the individual x/w/with/height methods with location/size using IntSize/IntPoint.

[Emil A Eklund]

Created attachment 94737 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 94737 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94737&action=review

> Source/WebCore/rendering/RenderLayer.cpp:655
>          inlineBoundingBoxOffset = IntSize(lineBox.x(), lineBox.y());

I assume lineBox has a location() function by now.

> Source/WebCore/rendering/RenderLayer.cpp:2007
> +    IntPoint bottomRight = toPoint(size());

Kinda a funny assumption that location() is always 0, 0.  Seems this should be asking for m_frameRect.bottomRight() or whatever location() + size() is.  Or maybe we should be ASSERTing that location() is 0, 0?  Or maybe its that the internal and external size of the layer are considered the same, and that the offset this returns is in local coordinates?  Confused.

> Source/WebCore/rendering/RenderLayer.cpp:3454
>      convertToLayerCoords(rootLayer, x, y);
> -    layerBounds = IntRect(x, y, width(), height());
> +    layerBounds = IntRect(IntPoint(x, y), size());

Seems odd that we wouldn't just map the whole rect.

> Source/WebCore/rendering/RenderTreeAsText.cpp:583
> +                    writeLayers(ts, l, l, IntRect(l->location(), l->size()), indent + 1, behavior);

Why not just ask for l->rect()?

> Source/WebCore/rendering/RenderTreeAsText.cpp:778
> +        writeLayers(ts, l, l, IntRect(l->location(), l->size()), 0, behavior);

l->rect() here?

[Emil A Eklund]

Created attachment 94875 [details]
Patch

[Emil A Eklund]

(In reply to comment #2)
> I assume lineBox has a location() function by now.

Ah, of course. Thanks!

> 
> > Source/WebCore/rendering/RenderLayer.cpp:2007
> > +    IntPoint bottomRight = toPoint(size());
> 
> Kinda a funny assumption that location() is always 0, 0.  Seems this should be asking for m_frameRect.bottomRight() or whatever location() + size() is.  Or maybe we should be ASSERTing that location() is 0, 0?  Or maybe its that the internal and external size of the layer are considered the same, and that the offset this returns is in local coordinates?  Confused.

Yeah, this is a bit weird. The way I understand it the internal and external coordinates are both considered to be the same here. 

> 
> > Source/WebCore/rendering/RenderLayer.cpp:3454
> >      convertToLayerCoords(rootLayer, x, y);
> > -    layerBounds = IntRect(x, y, width(), height());
> > +    layerBounds = IntRect(IntPoint(x, y), size());
> 
> Seems odd that we wouldn't just map the whole rect.

Odd indeed, I'd rather not make any behavior changes in this patch though.

> 
> > Source/WebCore/rendering/RenderTreeAsText.cpp:583
> > +                    writeLayers(ts, l, l, IntRect(l->location(), l->size()), indent + 1, behavior);
> 
> Why not just ask for l->rect()?

I didn't have a rect method before, now I do :)

[Eric Seidel (no email)]

Comment on attachment 94875 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94875&action=review

I don't need to see this again, but you should look at fixing some of the below mentioned nits before landing (with teh cq or otherwise).

> Source/WebCore/dom/MouseRelatedEvent.cpp:209
> +            m_layerX -= location.x();
> +            m_layerY -= location.y();

An obvious next candidate? :)

> Source/WebCore/rendering/RenderInline.cpp:1083
> +        IntRect boxRect(IntPoint(), containerBox->layer()->size());
>          rect = intersection(repaintRect, boxRect);

What is similar about this and the previous function?  Should IntRect rect(IntPoint(), containingBlock->layer()->size());  rect.intercect(repaintRect, boxRect); be some helper fucntion?

> Source/WebCore/rendering/RenderLayer.cpp:655
> +        inlineBoundingBoxOffset = toSize(lineBox.location());

I take it we youse inlineBoundingBoxOffset again later, or should this just get merged into the localPoint += line below?

> Source/WebCore/rendering/RenderLayer.cpp:2007
> +    IntPoint bottomRight = toPoint(size());

I feel like this needs a FIXME to explain why we assume the location is 0, 0.

> Source/WebCore/rendering/RenderLayer.cpp:3453
>      convertToLayerCoords(rootLayer, x, y);

An obvious next target.

> Source/WebCore/rendering/RenderLayer.h:220
> +    IntRect rect() const { return IntRect(location(), size()); }

Seems we should just move m_topLeft and m_layerSize into a rect soon. :)  Unless the location and size don't actually make sense paired as a rect? (except for DRT dumping).

> Source/WebCore/rendering/RenderView.cpp:331
> +    quads.append(FloatRect(IntPoint(), m_layer->size()));

I think you meant FloatPoint() here.

[Emil A Eklund]

Created attachment 95058 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 95058 [details]
Patch for landing

Clearing flags on attachment: 95058

Committed r87467: <http://trac.webkit.org/changeset/87467>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.