WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
58362
Bundle w and tmpW in findNextLineBreak together as a class
https://bugs.webkit.org/show_bug.cgi?id=58362
Summary
Bundle w and tmpW in findNextLineBreak together as a class
Ryosuke Niwa
Reported
2011-04-12 12:45:11 PDT
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.
Attachments
cleanup
(19.66 KB, patch)
2011-04-12 14:05 PDT
,
Ryosuke Niwa
hyatt
: review+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1
2011-04-12 14:05:22 PDT
Created
attachment 89268
[details]
cleanup
Ryosuke Niwa
Comment 2
2011-04-12 14:07:43 PDT
After this patch, I'll move availableWidth into LineWidth and will merge LineWidth and LineOffsets after that.
Dave Hyatt
Comment 3
2011-04-12 14:10:44 PDT
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
Comment 4
2011-04-12 14:13:47 PDT
(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
Comment 5
2011-04-12 14:33:03 PDT
Committed
r83640
: <
http://trac.webkit.org/changeset/83640
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug