Bug 58362 – Bundle w and tmpW in findNextLineBreak together as a class

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Ryosuke Niwa]

This is a cleanup. In this patch, I'm going to rename w, tmpW, and width as committedWidth, uncommittedWidth, and availableWith respectively per Dave's suggestion.  Furthermore, I'm going to create a new LineWidth class out of committedWidth and uncommittedWidth.

[Ryosuke Niwa]

Created attachment 89268 [details]
cleanup

[Ryosuke Niwa]

After this patch, I'll move availableWidth into LineWidth and will merge LineWidth and LineOffsets after that.

[Dave Hyatt]

Comment on attachment 89268 [details]
cleanup

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.

[Ryosuke Niwa]

(In reply to comment #3)
> (From update of attachment 89268 [details])
> 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.

Yup. My plan exactly!

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

Okay, will do.

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

That's a part of my plan as well :)

[Ryosuke Niwa]

Committed r83640: <http://trac.webkit.org/changeset/83640>