Bug 60046 - Bundle trailingSpaceObject and trailingPositionedBoxes in findNextLineBreak as a class
: Bundle trailingSpaceObject and trailingPositionedBoxes in findNextLineBreak a...
Status: RESOLVED FIXED
: WebKit
Layout and Rendering
: 528+ (Nightly build)
: All All
: P2 Enhancement
Assigned To:
:
:
:
: 57779
  Show dependency treegraph
 
Reported: 2011-05-03 11:36 PST by
Modified: 2011-05-03 13:33 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-05-03 11:36:37 PST
trailingSpaceObject and trailingPositionedBoxes in findNextLineBreak are always used together.  We should make a class that extracts these two variables.
------- Comment #1 From 2011-05-03 11:46:07 PST -------
Created an attachment (id=92097) [details]
cleanup
------- Comment #2 From 2011-05-03 11:51:39 PST -------
(From update of attachment 92097 [details])
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 From 2011-05-03 11:56:39 PST -------
(From update of attachment 92097 [details])
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 From 2011-05-03 12:00:32 PST -------
(From update of attachment 92097 [details])
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 From 2011-05-03 12:16:37 PST -------
Created an attachment (id=92101) [details]
patch for lading
------- Comment #6 From 2011-05-03 12:17:38 PST -------
(From update of attachment 92101 [details])
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 From 2011-05-03 12:18:20 PST -------
(From update of attachment 92101 [details])
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 From 2011-05-03 12:49:00 PST -------
(From update of attachment 92101 [details])
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 From 2011-05-03 13:33:30 PST -------
Committed r85649: <http://trac.webkit.org/changeset/85649>