Convert x,y and width,height pairs to IntPoint and IntSize for RenderLayer

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Emil A Eklund]

Convert RenderLayer to use IntPoint for x,y pairs and IntSize for offsets and sizes.

See https://bugs.webkit.org/show_bug.cgi?id=44412 for similar change to InlineBox.

[Emil A Eklund]

Created attachment 92647 [details]
WIP Patch

[Eric Seidel (no email)]

Comment on attachment 92647 [details]
WIP Patch

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

You are my hero.

> Source/WebCore/rendering/RenderLayer.cpp:144
> +    , m_topLeft(0, 0)
> +    , m_layerSize(0, 0)
> +    , m_scrollOffset(0, 0)
> +    , m_scrollOverflow(0, 0)
> +    , m_scrollSize(0, 0)

Do we even need these now?  Doesn't IntSize(), IntPoint() do the right thing?

[Eric Seidel (no email)]

Am I supposed to review this WIP?

[Emil A Eklund]

No, I just wanted to get some initial feedback on the approach. I'll upload a new patch when I'm ready. I should have made that clear.

> Do we even need these now?  Doesn't IntSize(), IntPoint() do the right thing?

Good point, we don't as the constructors do the right thing.

[Simon Fraser (smfr)]

Comment on attachment 92647 [details]
WIP Patch

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

> Source/WebCore/platform/graphics/IntPoint.h:93
> +    void move(const IntPoint& s) { move(s.x(), s.y()); } 

This is logically inconsistent - what does it mean to move a point by another point? It seems like the calling code should be using an IntSize.

[Emil A Eklund]

> This is logically inconsistent - what does it mean to move a point by another point? It seems like the calling code should be using an IntSize.

Yeah, I wasn't sure about that change. I'll revert it. 
Using an IntSize is probably not the right option either though (as it's used by RenderLayer::m_topLeft which represents a position).

[Emil A Eklund]

Created attachment 92670 [details]
Patch

Ready for review. I belive I've fixed the issues you pointed out in the previous patch.

[Eric Seidel (no email)]

Comment on attachment 92670 [details]
Patch

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

Looks great to me!  But I think we should leave Simon (or someone else familiar with layers) a day to comment if they care.

> Source/WebCore/rendering/RenderLayer.cpp:722
> +        m_relativeOffset.setWidth(0);
> +        m_relativeOffset.setHeight(0);

You could just do m_relativeOffset = IntSize(), no?  Or maybe intSize() has a clear() method?  I figure this can be expressed in one line regardless. :)

> Source/WebCore/rendering/RenderLayer.h:215
> +    int x() const { return m_topLeft.x(); }
> +    int y() const { return m_topLeft.y(); }

I feel like there was some concern brought up about using "topLeft" in the InlineBox patch I wrote like this... something about how things might not always be "top" or "left".   But that may not apply to layers... I think it was relating to vertical text.

> Source/WebCore/rendering/RenderLayer.h:219
> +        m_topLeft.setX(x);
> +        m_topLeft.setY(y);

I suspect there is a one-line equivalent.  m_topLeft = IntPoint(x, y); would be one way. :)

[Simon Fraser (smfr)]

Comment on attachment 92670 [details]
Patch

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

This is a good start.

> Source/WebCore/rendering/RenderLayer.cpp:718
> +        m_relativeOffset.setWidth(renderer()->relativePositionOffsetX());
> +        m_relativeOffset.setHeight(renderer()->relativePositionOffsetY());

Seems like the renderer should have relativePositionOffset()

> Source/WebCore/rendering/RenderLayer.h:224
> +    IntSize size() const { return m_layerSize; }

This could return const IntSize&

> Source/WebCore/rendering/RenderLayer.h:237
> +    IntSize scrolledContentOffset() const { return IntSize(scrollXOffset() + m_scrollOverflow.width(), scrollYOffset() + m_scrollOverflow.height()); }

Would be nice to do this math entirely in IntSize/IntPoint.

> Source/WebCore/rendering/RenderLayer.h:310
> +    IntSize relativePositionOffset() const { return m_relativeOffset; }

Could return const IntSize&

[Emil A Eklund]

Created attachment 92827 [details]
Patch

Thanks for the review Simon and Eric. I've made the changes you suggested, please take another look.

[Eric Seidel (no email)]

Comment on attachment 92827 [details]
Patch

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

Fantastic!

> Source/WebCore/rendering/RenderLayer.cpp:1335
> +    IntSize newScrollOffset = IntSize(x - m_scrollOrigin.x(), y - m_scrollOrigin.y());

I might have written this as IntPoint(x, y) - m_scrollOrigin, on the assumption that eventually this function will take a point instead of an int pair.

[Emil A Eklund]

Created attachment 93016 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 93016 [details]
Patch for landing

Clearing flags on attachment: 93016

Committed r86196: <http://trac.webkit.org/changeset/86196>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.