Bug 110644 - Add descriptive names for different addMidpoint use cases
Summary: Add descriptive names for different addMidpoint use cases
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Levi Weintraub
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-22 14:11 PST by Levi Weintraub
Modified: 2013-02-22 16:02 PST (History)
7 users (show)

See Also:


Attachments
Patch (13.96 KB, patch)
2013-02-22 14:30 PST, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (14.23 KB, patch)
2013-02-22 14:58 PST, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch for landing (14.60 KB, patch)
2013-02-22 15:43 PST, Levi Weintraub
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Levi Weintraub 2013-02-22 14:11:18 PST
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.
Comment 1 Levi Weintraub 2013-02-22 14:30:59 PST
Created attachment 189830 [details]
Patch
Comment 2 Eric Seidel (no email) 2013-02-22 14:38:12 PST
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?
Comment 3 Eric Seidel (no email) 2013-02-22 14:38:29 PST
Please answer my qeustions before settign cq+. :)
Comment 4 Levi Weintraub 2013-02-22 14:56:28 PST
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.
Comment 5 Levi Weintraub 2013-02-22 14:58:58 PST
Created attachment 189840 [details]
Patch
Comment 6 Ryosuke Niwa 2013-02-22 15:14:00 PST
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 :(
Comment 7 Levi Weintraub 2013-02-22 15:26:46 PST
(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 8 Ryosuke Niwa 2013-02-22 15:41:39 PST
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.
Comment 9 Levi Weintraub 2013-02-22 15:43:40 PST
Created attachment 189851 [details]
Patch for landing
Comment 10 WebKit Review Bot 2013-02-22 16:02:14 PST
Comment on attachment 189851 [details]
Patch for landing

Clearing flags on attachment: 189851

Committed r143812: <http://trac.webkit.org/changeset/143812>
Comment 11 WebKit Review Bot 2013-02-22 16:02:20 PST
All reviewed patches have been landed.  Closing bug.