Bug 98692 – Modify LayoutState ASSERTS to support SATURATED_LAYOUT_ARITHMETIC

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

[Emil A Eklund]

We currently overflow/wrap when computing the delta in RenderBlock::setLogicalTopForChild in cases where we have an element width a width or height exceeding maxLayoutUnit. When the delta is later added back in RenderBlock::layoutBlockChild the number wraps again getting us back to the correct value.

With SATURATED_LAYOUT_ARITHMETIC enabled the values no longer wraps, which seems like the correct thing to do however this causes the compare to fail for obvious reasons. By accounting for this we can keep the asserts (which have proven very helpful) even when SATURATED_LAYOUT_ARITHMETIC is turned on.

[Emil A Eklund]

Created attachment 167625 [details]
Patch

[Emil A Eklund]

Eric, would you mind reviewing this?

[Eric Seidel (no email)]

Mitz is the original author of LayoutDelta and LayoutState and is your best reviewer here.

[Emil A Eklund]

Great, thanks!

[Emil A Eklund]

Mitz, any chance you could take a look at this in the next couple of days? Would like to get your feedback on the approach.

Comment on attachment 167625 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        element width a width or height exceeding maxLayoutUnit. When

Typo: “width a width”

[Emil A Eklund]

Thank you!

[Emil A Eklund]

Created attachment 169935 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 169935 [details]
Patch for landing

Clearing flags on attachment: 169935

Committed r132105: <http://trac.webkit.org/changeset/132105>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 169935 [details]
Patch for landing

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

> Source/WebCore/rendering/LayoutState.cpp:39
> +    : m_clipped(false)

Why make this change? m_clipped is set unconditionally below. Just for #if convenience?

> Source/WebCore/rendering/LayoutState.cpp:43
> +#if !ASSERT_DISABLED && ENABLE(SATURATED_LAYOUT_ARITHMETIC)
> +    , m_layoutDeltaXSaturated(false)
> +    , m_layoutDeltaYSaturated(false)
> +#endif

Why initialize these here since they are unconditionally set in the function body below? This makes things more ugly for no real reason.

[Emil A Eklund]

(In reply to comment #11)
> (From update of attachment 169935 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169935&action=review
> 
> > Source/WebCore/rendering/LayoutState.cpp:39
> > +    : m_clipped(false)
> 
> Why make this change? m_clipped is set unconditionally below. Just for #if convenience?

Got a compiler warning about out of order initialization without it.

> 
> > Source/WebCore/rendering/LayoutState.cpp:43
> > +#if !ASSERT_DISABLED && ENABLE(SATURATED_LAYOUT_ARITHMETIC)
> > +    , m_layoutDeltaXSaturated(false)
> > +    , m_layoutDeltaYSaturated(false)
> > +#endif
> 
> Why initialize these here since they are unconditionally set in the function body below? This makes things more ugly for no real reason.

You are right, these are not needed. I'll remove them.

Thanks.

[Darin Adler]

Comment on attachment 169935 [details]
Patch for landing

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

>>> Source/WebCore/rendering/LayoutState.cpp:39
>>> +    : m_clipped(false)
>> 
>> Why make this change? m_clipped is set unconditionally below. Just for #if convenience?
> 
> Got a compiler warning about out of order initialization without it.

I don’t understand. Despite you reporting that, I am pretty sure you can remove this without creating a warning, particularly now that you have removed the initialization of the two new fields.

This constructor should revert to exactly how it was before.

[Emil A Eklund]

(In reply to comment #13)
> I don’t understand. Despite you reporting that, I am pretty sure you can remove this without creating a warning, particularly now that you have removed the initialization of the two new fields.

You are right, without the (unnecessary) initialization it is not needed. Reverted back to how it was before in r132233.

Thanks.