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+
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
Note You need to log in before you can comment on or make changes to this bug.