[webkit-reviews] review granted: [Bug 58362] Bundle w and tmpW in findNextLineBreak together as a class : [Attachment 89268] cleanup

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 58362: Bundle w and tmpW in findNextLineBreak together as a class
https://bugs.webkit.org/show_bug.cgi?id=58362

Attachment 89268: cleanup
https://bugs.webkit.org/attachment.cgi?id=89268&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=89268&action=review

This could also be done as a followup, but availableWidth should move into
LineWidth as well.  Then you could ask questions like "remainingWidth()" to get
availableWidth - currentWidth etc., and lots of these comparisons would get
cleaner.

r=me

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1639
> +inline LineWidth::LineWidth()
> +    : m_uncommittedWidth(0)
> +    , m_committedWidth(0)
> +{
> +}

I don't much like this style when the class is so tiny.  I'd just put this
right inside the class declaration.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1645
> +inline void LineWidth::commit()
> +{
> +    m_committedWidth += m_uncommittedWidth;
> +    m_uncommittedWidth = 0;
> +}

Same here.  Just put it up where you first declared the function.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1660
> +    float availableWidth = lineOffsets.width();
> +    LineWidth width;

Any reason not to just merge LineWidth with LineOffsets? They're close enough
to one another that I'd just make them a single struct. I guess that could be
done in a future patch though.