Bug 60409

Created attachment 92647 [details] WIP Patch

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?

Am I supposed to review this WIP?

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.

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.

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

Created attachment 92670 [details] Patch Ready for review. I belive I've fixed the issues you pointed out in the previous patch.

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

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&

Created attachment 92827 [details] Patch Thanks for the review Simon and Eric. I've made the changes you suggested, please take another look.

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.

Created attachment 93016 [details] Patch for landing

Comment on attachment 93016 [details] Patch for landing Clearing flags on attachment: 93016 Committed r86196: <http://trac.webkit.org/changeset/86196>

All reviewed patches have been landed. Closing bug.