The following code appears almost identical twice in ::paintObject. As per the review comments in bug 156590 (https://bugs.webkit.org/show_bug.cgi?id=156590#c16 and https://bugs.webkit.org/show_bug.cgi?id=156590#c14), it might be nice to factor out this block to avoid duplication. void RenderListBox::paintObject(PaintInfo& paintInfo, const LayoutPoint& paintOffset) { if (style().visibility() != VISIBLE) return; (...) if (paintInfo.phase == PaintPhaseForeground) {{ int index = m_indexOffset; while (index < listItemsSize && index <= m_indexOffset + numVisibleItems()) { paintItemForeground(paintInfo, paintOffset, index); index++; } } (...) case PaintPhaseChildBlockBackground: case PaintPhaseChildBlockBackgrounds: { int index = m_indexOffset; while (index < listItemsSize && index <= m_indexOffset + numVisibleItems()) { paintItemBackground(paintInfo, paintOffset, index); index++; } } (...) }
Created attachment 277591 [details] Patch v1
Attachment 277591 [details] did not pass style-queue: ERROR: Source/WebCore/rendering/RenderListBox.cpp:276: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 277593 [details] Patch v1.1 - fixed style issue. (In reply to comment #2) > Attachment 277591 [details] did not pass style-queue: > > > ERROR: Source/WebCore/rendering/RenderListBox.cpp:276: Weird number of > spaces at line-start. Are you using a 4-space indent? [whitespace/indent] > [3] > Total errors found: 1 in 3 files Fixed.
Comment on attachment 277593 [details] Patch v1.1 - fixed style issue. View in context: https://bugs.webkit.org/attachment.cgi?id=277593&action=review > Source/WebCore/ChangeLog:3 > + Factor out the "paint item" logic in RenderListBox into a helper. I removed the period from the Bugzilla title. Please update this line. > Source/WebCore/ChangeLog:8 > + Patch factors out the duplicated painting logic in RenderListBox::paintObject Very minor: RenderListBox::paintObject => RenderListBox::paintObject() (Notice the addition of parentheses at the end of proposed substitution to make it clear we are talking about a function - though this can likely be inferred from context). > Source/WebCore/ChangeLog:9 > + into a helper method named ::paintItem. Very minor: "method named ::paintItem" => "member function named paintItem" > Source/WebCore/ChangeLog:11 > + This is a preparation for bug 156590. Nit: a => in > Source/WebCore/rendering/RenderListBox.cpp:279 > + ASSERT(paintInfo.phase == PaintPhaseForeground > + || paintInfo.phase == PaintPhaseChildBlockBackground > + || paintInfo.phase == PaintPhaseChildBlockBackgrounds); Is this ASSERT() necessary? I mean, I do not see the need to make this function specific to these paint phases. I also do not see the value in having this ASSERT() because it seems like it would be contrived for a person to call this for something other than these phases. > Source/WebCore/rendering/RenderListBox.cpp:286 > + int listItemsSize = numItems(); > + int index = m_indexOffset; > + while (index < listItemsSize && index <= m_indexOffset + numVisibleItems()) { > + painterFunction(paintInfo, paintOffset, index); > + index++; > + } We could take this opportunity to cache m_indexOffset + numVisibleItems() in a local variable and avoid computing it on each iteration. Maybe something like: int listItemsSize = numItems(); int endIndex = m_indexOffset + numVisibleItems(); for (int i = m_indexOffset; i < listItemsSize && i <= endIndex; ++i) paintFunction(paintInfo, paintOffset, i); > Source/WebCore/rendering/RenderListBox.cpp:296 > + using namespace std::placeholders; > + paintItem(paintInfo, paintOffset, std::bind(&RenderListBox::paintItemForeground, this, _1, _2, _3)); From my understanding we prefer to use C++11 lambda functions instead of std::bind(). I would write this as: paintItem(paintInfo, paintOffset, [this](PaintInfo& paintInfo, const LayoutPoint& paintOffset, int listItemIndex) { paintItemForeground(paintInfo, paintOffset, listItemIndex); }); > Source/WebCore/rendering/RenderListBox.cpp:316 > + using namespace std::placeholders; > + paintItem(paintInfo, paintOffset, std::bind(&RenderListBox::paintItemBackground, this, _1, _2, _3)); Similarly, I would write this as: paintItem(paintInfo, paintOffset, [this](PaintInfo& paintInfo, const LayoutPoint& paintOffset, int listItemIndex) { paintItemBackground(paintInfo, paintOffset, listItemIndex); }); > Source/WebCore/rendering/RenderListBox.h:145 > + typedef std::function<void(PaintInfo& , const LayoutPoint& , int listIndex)> PainterFunction; Although unwritten, we prefer to use C++11 type aliases instead of typedef: using PaintFunction = std::function<void(PaintInfo&, const LayoutPoint&, int listItemIndex)>; Notice that I renamed the function from PainterFunction to PaintFunction so as to be consistent with PaintInfo and only placed a space character after each comma.
(In reply to comment #4) > Comment on attachment 277593 [details] > Patch v1.1 - fixed style issue. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=277593&action=review > > > Source/WebCore/ChangeLog:3 > > + Factor out the "paint item" logic in RenderListBox into a helper. > > I removed the period from the Bugzilla title. Please update this line. Done. > > Source/WebCore/ChangeLog:8 > > + Patch factors out the duplicated painting logic in RenderListBox::paintObject > > Very minor: RenderListBox::paintObject => RenderListBox::paintObject() Done. > > Source/WebCore/ChangeLog:9 > > + into a helper method named ::paintItem. > > Very minor: "method named ::paintItem" => "member function named paintItem" > Done. > > Source/WebCore/ChangeLog:11 > > + This is a preparation for bug 156590. > > Nit: a => in Done. > > Source/WebCore/rendering/RenderListBox.cpp:279 > > + ASSERT(paintInfo.phase == PaintPhaseForeground > > + || paintInfo.phase == PaintPhaseChildBlockBackground > > + || paintInfo.phase == PaintPhaseChildBlockBackgrounds); > > Is this ASSERT() necessary? I mean, I do not see the need to make this > function specific to these paint phases. I also do not see the value in > having this ASSERT() because it seems like it would be contrived for a > person to call this for something other than these phases. It was mostly to self-document it, making it explicit to readers that it will be called with one of this painting phases. Not only catch errors made in the code. In any case, I have removed the ASSERT. > > Source/WebCore/rendering/RenderListBox.cpp:286 > > + int listItemsSize = numItems(); > > + int index = m_indexOffset; > > + while (index < listItemsSize && index <= m_indexOffset + numVisibleItems()) { > > + painterFunction(paintInfo, paintOffset, index); > > + index++; > > + } > > We could take this opportunity to cache m_indexOffset + numVisibleItems() in > a local variable and avoid computing it on each iteration. Maybe something > like: > > int listItemsSize = numItems(); > int endIndex = m_indexOffset + numVisibleItems(); > for (int i = m_indexOffset; i < listItemsSize && i <= endIndex; ++i) > paintFunction(paintInfo, paintOffset, i); Good! Done. > > Source/WebCore/rendering/RenderListBox.cpp:296 > > + using namespace std::placeholders; > > + paintItem(paintInfo, paintOffset, std::bind(&RenderListBox::paintItemForeground, this, _1, _2, _3)); > > From my understanding we prefer to use C++11 lambda functions instead of > std::bind(). I would write this as: > > paintItem(paintInfo, paintOffset, [this](PaintInfo& paintInfo, const > LayoutPoint& paintOffset, int listItemIndex) { > paintItemForeground(paintInfo, paintOffset, listItemIndex); > }); Yes, sir. Done. > > Source/WebCore/rendering/RenderListBox.cpp:316 > > + using namespace std::placeholders; > > + paintItem(paintInfo, paintOffset, std::bind(&RenderListBox::paintItemBackground, this, _1, _2, _3)); > > Similarly, I would write this as: > > paintItem(paintInfo, paintOffset, [this](PaintInfo& paintInfo, const > LayoutPoint& paintOffset, int listItemIndex) { > paintItemBackground(paintInfo, paintOffset, listItemIndex); > }); Done. > > Source/WebCore/rendering/RenderListBox.h:145 > > + typedef std::function<void(PaintInfo& , const LayoutPoint& , int listIndex)> PainterFunction; > > Although unwritten, we prefer to use C++11 type aliases instead of typedef: > > using PaintFunction = std::function<void(PaintInfo&, const LayoutPoint&, int > listItemIndex)>; > > Notice that I renamed the function from PainterFunction to PaintFunction so > as to be consistent with PaintInfo and only placed a space character after > each comma. Done.
Created attachment 277616 [details] Patch v1.2 - for landing (addressed Dan's review).
Comment on attachment 277616 [details] Patch v1.2 - for landing (addressed Dan's review). Clearing flags on attachment: 277616 Committed r200190: <http://trac.webkit.org/changeset/200190>
All reviewed patches have been landed. Closing bug.
Comment on attachment 277616 [details] Patch v1.2 - for landing (addressed Dan's review). View in context: https://bugs.webkit.org/attachment.cgi?id=277616&action=review > Source/WebCore/rendering/RenderListBox.h:146 > + void paintItem(PaintInfo& , const LayoutPoint& , PaintFunction); Stray spaces before the commas here.
(In reply to comment #9) > > Source/WebCore/rendering/RenderListBox.h:146 > > + void paintItem(PaintInfo& , const LayoutPoint& , PaintFunction); > > Stray spaces before the commas here. Removed stray spaces in <http://trac.webkit.org/changeset/200230>.
(In reply to comment #10) > (In reply to comment #9) > > > Source/WebCore/rendering/RenderListBox.h:146 > > > + void paintItem(PaintInfo& , const LayoutPoint& , PaintFunction); > > > > Stray spaces before the commas here. > > Removed stray spaces in <http://trac.webkit.org/changeset/200230>. I think Dan pointed them out, and I missed it. Thanks Dan/Darin.