Bug 86894 – Simplify RenderOverflow by using Rects

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

[Emil A Eklund]

Simplify the RenderOverflow class and the uses of it by storing the overflow values in rects and using the rects instead of the individual values where it makes sense.

[Emil A Eklund]

Created attachment 142824 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 142824 [details]
Patch

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

Wow.  total insanity.  I suspect there is another round of cleanup to be done here as well with all the "logical" accessors.

> Source/WebCore/page/FrameView.cpp:3283
> +        IntRect dirtyRect(0, static_cast<int>(floorf(oldTop)), root->layoutOverflowRect().maxX(), static_cast<int>(ceilf(oldBottom - oldTop)));

this looks a lot like enclosingIntRect(FloatRect(0, oldTop, maxX, oldBottom - oldTop))?

> Source/WebCore/rendering/InlineFlowBox.h:210
> +    LayoutUnit logicalLeftLayoutOverflow() const { return m_overflow ? (isHorizontal() ? m_overflow->layoutOverflowRect().x() : m_overflow->layoutOverflowRect().y()) : static_cast<LayoutUnit>(logicalLeft()); }

Might make sense to fomat this as multiple lines.

> Source/WebCore/rendering/InlineFlowBox.h:215
> -            return isHorizontal() ? m_overflow->minYLayoutOverflow() : m_overflow->minXLayoutOverflow();
> +            return isHorizontal() ? m_overflow->layoutOverflowRect().y() : m_overflow->layoutOverflowRect().x();

We need to find a cleaner way to do this flipping-on-demand that we do everywhere.  just "return logicalOverflowRect().x()"?

> Source/WebCore/rendering/RenderBox.cpp:3738
> -    m_overflow->resetLayoutOverflow(borderBoxRect());
> +    m_overflow->setLayoutOverflow(borderBoxRect());

I'm not sure I follow?  Did you rename this?

[Emil A Eklund]

(In reply to comment #2)
> (From update of attachment 142824 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142824&action=review
> 
> Wow.  total insanity.  I suspect there is another round of cleanup to be done here as well with all the "logical" accessors.

Yeah, I plan to tackle that next.


> > Source/WebCore/page/FrameView.cpp:3283
> > +        IntRect dirtyRect(0, static_cast<int>(floorf(oldTop)), root->layoutOverflowRect().maxX(), static_cast<int>(ceilf(oldBottom - oldTop)));
> 
> this looks a lot like enclosingIntRect(FloatRect(0, oldTop, maxX, oldBottom - oldTop))?

It does, doesn't it? I'll see if I can change it to use enclosingIntRect. Thanks!

> 
> > Source/WebCore/rendering/InlineFlowBox.h:210
> > +    LayoutUnit logicalLeftLayoutOverflow() const { return m_overflow ? (isHorizontal() ? m_overflow->layoutOverflowRect().x() : m_overflow->layoutOverflowRect().y()) : static_cast<LayoutUnit>(logicalLeft()); }
> 
> Might make sense to fomat this as multiple lines.

Good idea.

> > Source/WebCore/rendering/InlineFlowBox.h:215
> > -            return isHorizontal() ? m_overflow->minYLayoutOverflow() : m_overflow->minXLayoutOverflow();
> > +            return isHorizontal() ? m_overflow->layoutOverflowRect().y() : m_overflow->layoutOverflowRect().x();
> 
> We need to find a cleaner way to do this flipping-on-demand that we do everywhere.  just "return logicalOverflowRect().x()"?

Yeah, I have a couple of ideas around that. Expect more cleanup patches next week :)

> 
> > Source/WebCore/rendering/RenderBox.cpp:3738
> > -    m_overflow->resetLayoutOverflow(borderBoxRect());
> > +    m_overflow->setLayoutOverflow(borderBoxRect());
> 
> I'm not sure I follow?  Did you rename this?

No, I removed the resetLayoutOverflow method as it did exactly the same thing as the set method.

[Emil A Eklund]

Committed r117697: <http://trac.webkit.org/changeset/117697>