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.
Created attachment 189830 [details] Patch
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?
Please answer my qeustions before settign cq+. :)
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.
Created attachment 189840 [details] Patch
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 :(
(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.
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.
Created attachment 189851 [details] Patch for landing
Comment on attachment 189851 [details] Patch for landing Clearing flags on attachment: 189851 Committed r143812: <http://trac.webkit.org/changeset/143812>
All reviewed patches have been landed. Closing bug.