Bug 60046 – Bundle trailingSpaceObject and trailingPositionedBoxes in findNextLineBreak as a class

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

[Ryosuke Niwa]

trailingSpaceObject and trailingPositionedBoxes in findNextLineBreak are always used together.  We should make a class that extracts these two variables.

[Ryosuke Niwa]

Created attachment 92097 [details]
cleanup

[Eric Seidel (no email)]

Comment on attachment 92097 [details]
cleanup

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

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1670
> +    void addMidpoints(LineMidpointState&, const InlineIterator& lBreak);

I would add a newline befor eprivate:

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1824
> +                // FIXME: should we call trainingSpace.clear() ?

trailingObjects. you mean.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2172
> +                    trailingObjects.setTrailingWhitespace(static_cast<RenderText*>(o));

do we have a safer toRenderText function to use instead of a direct cast?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2297
> +    trailingObjects.addMidpoints(lineMidpointState, lBreak);

Maybe addMidpoints needs a better name.  Are we adding midpoints to the TrailingObjects?  Or are we adding midpoints based on the trailing objects?  Should this be on MidPointSTate or on the TrailingObjects class?

[Ryosuke Niwa]

Comment on attachment 92097 [details]
cleanup

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

>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1670
>> +    void addMidpoints(LineMidpointState&, const InlineIterator& lBreak);
> 
> I would add a newline befor eprivate:

Will do.

>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2172
>> +                    trailingObjects.setTrailingWhitespace(static_cast<RenderText*>(o));
> 
> do we have a safer toRenderText function to use instead of a direct cast?

Yes but we know that o->isText() is true because we're inside if (o->isText()).  Look around the line 1860; that's where this giant if clause starts.

>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2297
>> +    trailingObjects.addMidpoints(lineMidpointState, lBreak);
> 
> Maybe addMidpoints needs a better name.  Are we adding midpoints to the TrailingObjects?  Or are we adding midpoints based on the trailing objects?  Should this be on MidPointSTate or on the TrailingObjects class?

Does updateMidpoints sound better?

[Eric Seidel (no email)]

Comment on attachment 92097 [details]
cleanup

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

>>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2297
>>> +    trailingObjects.addMidpoints(lineMidpointState, lBreak);
>> 
>> Maybe addMidpoints needs a better name.  Are we adding midpoints to the TrailingObjects?  Or are we adding midpoints based on the trailing objects?  Should this be on MidPointSTate or on the TrailingObjects class?
> 
> Does updateMidpoints sound better?

Try updateMidpointsForTrailingBoxes.

[Ryosuke Niwa]

Created attachment 92101 [details]
patch for lading

[Ryosuke Niwa]

Comment on attachment 92101 [details]
patch for lading

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

> Source/WebCore/ChangeLog:15
> +        (WebCore::TrailingObjects::addMidpoints):

Oops. This should be updateMidpointsForTrailingBoxes instead.

[Eric Seidel (no email)]

Comment on attachment 92101 [details]
patch for lading

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

OK.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2297
> +    trailingObjects.updateMidpointsForTrailingBoxes(lineMidpointState, lBreak);

lbreak needs a better name at some point. :)

[Ryosuke Niwa]

Comment on attachment 92101 [details]
patch for lading

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

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:-1747
> -                previousLineBrokeCleanly = true;

Oops, I shouldn't be removing this line :(

[Ryosuke Niwa]

Committed r85649: <http://trac.webkit.org/changeset/85649>