Bug 110644 – Add descriptive names for different addMidpoint use cases

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

[Levi Weintraub]

Midpoints are used to delineate ranges where we don't add line boxes for contents (collapsed spaces), but we also use them to split a text paragraph separator into a different line box from text before it. I came up with four helper functions to try and make it clearer what we're doing when we add midpoints during line layout. It also allows us to add an assertion for midpoints to be balanced.

[Levi Weintraub]

Created attachment 189830 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 189830 [details]
Patch

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

You are my hero.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:-2662
> -                    ignoreStart.m_obj = current.m_obj;
> -                    ignoreStart.m_pos = 0;

We're just re-using the iterator here?  Is it OK to remove this?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2940
> -                            if (ignoringSpaces || !currentStyle->collapseWhiteSpace() || !currentCharacterIsSpace || !previousCharacterIsSpace)
> +                            if (ignoringSpaces || !collapseWhiteSpace || !currentCharacterIsSpace || !previousCharacterIsSpace)

I assume these are equivalent?

[Eric Seidel (no email)]

Please answer my qeustions before settign cq+. :)

[Levi Weintraub]

Comment on attachment 189830 [details]
Patch

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

Thanks for the kind words! I'll update as per our conversation.

>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:-2662
>> -                    ignoreStart.m_pos = 0;
> 
> We're just re-using the iterator here?  Is it OK to remove this?

Yessir!

>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2940
>> +                            if (ignoringSpaces || !collapseWhiteSpace || !currentCharacterIsSpace || !previousCharacterIsSpace)
> 
> I assume these are equivalent?

Correct. We explicitly store this value earlier and this is the one place we failed to use the saved value.

[Levi Weintraub]

Created attachment 189840 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 189840 [details]
Patch

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

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:379
> +// When ignoring spaces, this needs to be called for objects that need line boxes such as RenderInlines or
> +// hard line breaks to ensure that they're not ignored.
> +static inline void ensureLineBox(LineMidpointState& lineMidpointState, RenderObject* renderer)

How about renaming the function to something like ensureLineBoxWhileIgnoringSpaces? Though this name seems to suggest as if the function itself is going to ignore spaces :(

[Levi Weintraub]

(In reply to comment #6)
> (From update of attachment 189840 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189840&action=review
> 
> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:379
> > +// When ignoring spaces, this needs to be called for objects that need line boxes such as RenderInlines or
> > +// hard line breaks to ensure that they're not ignored.
> > +static inline void ensureLineBox(LineMidpointState& lineMidpointState, RenderObject* renderer)
> 
> How about renaming the function to something like ensureLineBoxWhileIgnoringSpaces? Though this name seems to suggest as if the function itself is going to ignore spaces :(

Either name is an improvement over what we have now. If you prefer that one I'm happy to change it.

[Ryosuke Niwa]

Comment on attachment 189840 [details]
Patch

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

>>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:379
>>> +static inline void ensureLineBox(LineMidpointState& lineMidpointState, RenderObject* renderer)
>> 
>> How about renaming the function to something like ensureLineBoxWhileIgnoringSpaces? Though this name seems to suggest as if the function itself is going to ignore spaces :(
> 
> Either name is an improvement over what we have now. If you prefer that one I'm happy to change it.

Per IRC discussion, I'd prefer ensureLineBoxInsideIgnoredSpaces.

[Levi Weintraub]

Created attachment 189851 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 189851 [details]
Patch for landing

Clearing flags on attachment: 189851

Committed r143812: <http://trac.webkit.org/changeset/143812>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.