| Summary: | Bundle trailingSpaceObject and trailingPositionedBoxes in findNextLineBreak as a class | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||
| Component: | Layout and Rendering | Assignee: | Ryosuke Niwa <rniwa> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Enhancement | CC: | eric, hyatt, jamesr, leviw, mitz, simon.fraser | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 57779 | ||||||||
| Attachments: |
|
||||||||
|
Description
Ryosuke Niwa
2011-05-03 11:36:37 PDT
Created attachment 92097 [details]
cleanup
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 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 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. Created attachment 92101 [details]
patch for lading
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 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 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 :( Committed r85649: <http://trac.webkit.org/changeset/85649> |