Bug 57851

Summary: Bundle lineLeftOffset and lineRightOffset as a class
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Enhancement CC: commit-queue, dglazkov, eric, hyatt, jamesr, leviw, mitz, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 57943    
Bug Blocks: 57779    
Attachments:
Description Flags
cleanup
none
cleanup
none
cleanup none

Ryosuke Niwa
Reported 2011-04-05 08:17:02 PDT
lineLeftOffset and lineRightOffset are always used as a pair so we should bundle them up as a class.
Attachments
cleanup (17.01 KB, patch)
2011-04-05 08:36 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
cleanup (17.05 KB, patch)
2011-04-05 08:39 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
cleanup (8.53 KB, patch)
2011-04-06 07:28 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1 2011-04-05 08:36:54 PDT
Created attachment 88241 [details] cleanup
Ryosuke Niwa
Comment 2 2011-04-05 08:39:08 PDT
Created attachment 88242 [details] cleanup
Eric Seidel (no email)
Comment 3 2011-04-06 05:37:21 PDT
Comment on attachment 88242 [details] cleanup View in context: https://bugs.webkit.org/attachment.cgi?id=88242&action=review > Source/WebCore/rendering/RenderBlockLineLayout.cpp:80 > + extraWidth += borderPaddingMarginStart(toRenderBoxModelObject(parent)); I might have even added a RenderBoxModelObject* parentAsBoxModelObject local. I wonder it that's always true for SVG Text. I think we still use a RenderBoxModel object to hol the SVG text. I think we have to since we haven't split LineBoxTree out into its own class. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:598 > +static inline bool isWhitespace(UChar character, bool shouldPreserveNewline) Might want a more specific name relating to what spec defines this whitespace, etc. isCSS3WhiteSpace for example. isCSSSkipableWhitespace, or wahtever. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2279 > +bool RenderBlock::positionNewFloatOnLine(FloatingObject* newFloat, FloatingObject* lastFloatFromPreviousLine, bool firstLine, LineOffsets& lineOffsets) I would have preferred that you moved this code in a separate patch. Makes it difficult to read this diff. I'm slightly lost in all the changes you're making.
Ryosuke Niwa
Comment 4 2011-04-06 07:28:56 PDT
Created attachment 88413 [details] cleanup
Dimitri Glazkov (Google)
Comment 5 2011-04-06 08:52:51 PDT
Comment on attachment 88413 [details] cleanup pretty.
Ryosuke Niwa
Comment 6 2011-04-06 10:02:26 PDT
(In reply to comment #5) > (From update of attachment 88413 [details]) > pretty. Yay! Thanks for the review. Hopefully, I can add more high-level methods in later patches.
WebKit Commit Bot
Comment 7 2011-04-06 11:34:27 PDT
Comment on attachment 88413 [details] cleanup Clearing flags on attachment: 88413 Committed r83076: <http://trac.webkit.org/changeset/83076>
WebKit Commit Bot
Comment 8 2011-04-06 11:34:33 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.