Bug 60046 - Bundle trailingSpaceObject and trailingPositionedBoxes in findNextLineBreak as a class
: Bundle trailingSpaceObject and trailingPositionedBoxes in findNextLineBreak a...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering
: 528+ (Nightly build)
: All All
: P2 Enhancement
Assigned To: Ryosuke Niwa
:
Depends on:
Blocks: 57779
  Show dependency treegraph
 
Reported: 2011-05-03 11:36 PDT by Ryosuke Niwa
Modified: 2011-05-03 13:33 PDT (History)
6 users (show)

See Also:


Attachments
cleanup (11.32 KB, patch)
2011-05-03 11:46 PDT, Ryosuke Niwa
eric: review+
Details | Formatted Diff | Diff
patch for lading (11.37 KB, patch)
2011-05-03 12:16 PDT, Ryosuke Niwa
eric: review+
rniwa: commit‑queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-05-03 11:36:37 PDT
trailingSpaceObject and trailingPositionedBoxes in findNextLineBreak are always used together.  We should make a class that extracts these two variables.
Comment 1 Ryosuke Niwa 2011-05-03 11:46:07 PDT
Created attachment 92097 [details]
cleanup
Comment 2 Eric Seidel 2011-05-03 11:51:39 PDT
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?
Comment 3 Ryosuke Niwa 2011-05-03 11:56:39 PDT
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?
Comment 4 Eric Seidel 2011-05-03 12:00:32 PDT
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.
Comment 5 Ryosuke Niwa 2011-05-03 12:16:37 PDT
Created attachment 92101 [details]
patch for lading
Comment 6 Ryosuke Niwa 2011-05-03 12:17:38 PDT
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.
Comment 7 Eric Seidel 2011-05-03 12:18:20 PDT
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. :)
Comment 8 Ryosuke Niwa 2011-05-03 12:49:00 PDT
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 :(
Comment 9 Ryosuke Niwa 2011-05-03 13:33:30 PDT
Committed r85649: <http://trac.webkit.org/changeset/85649>