Bug 57851 - Bundle lineLeftOffset and lineRightOffset as a class
Summary: Bundle lineLeftOffset and lineRightOffset as a class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on: 57943
Blocks: 57779
  Show dependency treegraph
 
Reported: 2011-04-05 08:17 PDT by Ryosuke Niwa
Modified: 2011-04-06 11:34 PDT (History)
8 users (show)

See Also:


Attachments
cleanup (17.01 KB, patch)
2011-04-05 08:36 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
cleanup (17.05 KB, patch)
2011-04-05 08:39 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
cleanup (8.53 KB, patch)
2011-04-06 07:28 PDT, Ryosuke Niwa
no flags 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-04-05 08:17:02 PDT
lineLeftOffset and lineRightOffset are always used as a pair so we should bundle them up as a class.
Comment 1 Ryosuke Niwa 2011-04-05 08:36:54 PDT
Created attachment 88241 [details]
cleanup
Comment 2 Ryosuke Niwa 2011-04-05 08:39:08 PDT
Created attachment 88242 [details]
cleanup
Comment 3 Eric Seidel (no email) 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.
Comment 4 Ryosuke Niwa 2011-04-06 07:28:56 PDT
Created attachment 88413 [details]
cleanup
Comment 5 Dimitri Glazkov (Google) 2011-04-06 08:52:51 PDT
Comment on attachment 88413 [details]
cleanup

pretty.
Comment 6 Ryosuke Niwa 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.
Comment 7 WebKit Commit Bot 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>
Comment 8 WebKit Commit Bot 2011-04-06 11:34:33 PDT
All reviewed patches have been landed.  Closing bug.