Bug 60082 – findNextLineBreak splits InlineIterator into 3 pieces

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

[Ryosuke Niwa]

There's a FIXME in findNextLineBreak that says:

     // FIXME: It is error-prone to split the position object out like this.
     // Teach this code to work with objects instead of this split tuple.

We should fix this.

[Ryosuke Niwa]

Created attachment 92167 [details]
cleanup

[Eric Seidel (no email)]

Comment on attachment 92167 [details]
cleanup

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

I think this makes the code uglier, but more obviously shows what was wrong with the old code.  Do you plan to have a follow-up to clean up more of this?

> Source/WebCore/rendering/InlineIterator.h:241
> +inline void InlineIterator::fastIncrementInTextNode()

Seems the normal increment() call wants to call this?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1818
> +    while (current.m_obj) {

Do we not have an object() function to call instead?  Seems new code shouldn't be grabbing m_ members.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1819
> +        RenderObject* next = bidiNext(this, current.m_obj);

I think it's OK to store o as a local to reduce the code churn here.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1998
> +                lBreak.moveToStartOf(current.m_obj);

Wow. I now have some clue what this line does!  Maybe geting rid of "o" in this change is the right thing to do!

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2138
> -                        lBreak.moveTo(o, pos, nextBreakable);
> +                        lBreak.moveTo(current.m_obj, current.m_pos, current.m_nextBreakablePosition);

Sigh.  This is nearly the same as lBreak = current, but not quite (due to root).

[Ryosuke Niwa]

Comment on attachment 92167 [details]
cleanup

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

>> Source/WebCore/rendering/InlineIterator.h:241
>> +inline void InlineIterator::fastIncrementInTextNode()
> 
> Seems the normal increment() call wants to call this?

Sure.

>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1818

> 
> Do we not have an object() function to call instead?  Seems new code shouldn't be grabbing m_ members.

No :(

>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1998

> 
> Wow. I now have some clue what this line does!  Maybe geting rid of "o" in this change is the right thing to do!

I believe so.

[Ryosuke Niwa]

Committed r85695: <http://trac.webkit.org/changeset/85695>