Bug 60046

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:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

57779

Attachments:

Description	Flags
cleanup	eric: review+
patch for lading	eric: review+, rniwa: commit-queue-

Ryosuke Niwa

Reported 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.

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
View All Add attachment proposed patch, testcase, etc.

Ryosuke Niwa

Comment 1 2011-05-03 11:46:07 PDT

Created attachment 92097 [details] cleanup

Eric Seidel (no email)

Comment 2 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?

Ryosuke Niwa

Comment 3 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?

Eric Seidel (no email)

Comment 4 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.

Ryosuke Niwa

Comment 5 2011-05-03 12:16:37 PDT

Created attachment 92101 [details] patch for lading

Ryosuke Niwa

Comment 6 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.

Eric Seidel (no email)

Comment 7 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. :)

Ryosuke Niwa

Comment 8 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 :(

Ryosuke Niwa

Comment 9 2011-05-03 13:33:30 PDT

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

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