We should merge: Add support for 'order' on grid items The property is used to modify painting order and the order in which we consider grid items for the auto-flow algorithm. The code is build on the existing OrderIterator and matches what flexbox does. Also to avoid adding an extra child walk, refactored the explicit grid initialization so that we do also initialize the OrderIterator's Vector. This refactoring saves a child walk as we now compute both explicit grid's sizes in a single child walk. BUG=257078 TESTS=fast/css-grid-layout/grid-item-order-auto-flow-resolution.html fast/css-grid-layout/grid-item-order-paint-order-expected.html Review URL: https://chromiumcodereview.appspot.com/18516007
Created attachment 215041 [details] Patch
Comment on attachment 215041 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=215041&action=review > Source/WebCore/rendering/OrderIterator.cpp:81 > + do { > + if (!m_currentChild) { > + if (m_orderValuesIterator == m_orderValues.end()) > + return 0; > + if (m_orderValuesIterator) { > + ++m_orderValuesIterator; > + if (m_orderValuesIterator == m_orderValues.end()) > + return 0; > + } else > + m_orderValuesIterator = m_orderValues.begin(); > + > + m_currentChild = m_containerBox->firstChildBox(); > + } else > + m_currentChild = m_currentChild->nextSiblingBox(); > + } while (!m_currentChild || m_currentChild->style()->order() != *m_orderValuesIterator); This would read better if it used continue: if (m_currentChild) { m_currentChild = m_currentChild->nextSiblingBox(); continue; } ... etc > Source/WebCore/rendering/OrderIterator.h:42 > +class OrderIterator { OrderIterator is a rather generic name. Order of what? Maybe something like GridOrderIterator? > Source/WebCore/rendering/OrderIterator.h:53 > + RenderBox* currentChild() const { return m_currentChild; } > + RenderBox* first(); > + RenderBox* next(); > + void reset(); It would be nice if iterators would implement stl style iterator interface. In the future we will be able to use C++11 range for loops. > Source/WebCore/rendering/OrderIterator.h:59 > + Vector<int>::const_iterator m_orderValuesIterator; Please use unsigned instead of Vector<int>::const_iterator and call it m_orderIndex or similar.
(In reply to comment #2) > (From update of attachment 215041 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=215041&action=review > > > Source/WebCore/rendering/OrderIterator.cpp:81 > > + do { > > + if (!m_currentChild) { > > + if (m_orderValuesIterator == m_orderValues.end()) > > + return 0; > > + if (m_orderValuesIterator) { > > + ++m_orderValuesIterator; > > + if (m_orderValuesIterator == m_orderValues.end()) > > + return 0; > > + } else > > + m_orderValuesIterator = m_orderValues.begin(); > > + > > + m_currentChild = m_containerBox->firstChildBox(); > > + } else > > + m_currentChild = m_currentChild->nextSiblingBox(); > > + } while (!m_currentChild || m_currentChild->style()->order() != *m_orderValuesIterator); > > This would read better if it used continue: > > if (m_currentChild) { > m_currentChild = m_currentChild->nextSiblingBox(); > continue; > } > ... etc Will change it > > Source/WebCore/rendering/OrderIterator.h:42 > > +class OrderIterator { > > OrderIterator is a rather generic name. Order of what? Maybe something like GridOrderIterator? > Well that was the name when the class was inside RenderFlexibleBox, GridOrderIterator sounds too specific to grid layout and this is shared with flexbox > > Source/WebCore/rendering/OrderIterator.h:53 > > + RenderBox* currentChild() const { return m_currentChild; } > > + RenderBox* first(); > > + RenderBox* next(); > > + void reset(); > > It would be nice if iterators would implement stl style iterator interface. In the future we will be able to use C++11 range for loops. > Makes sense > > Source/WebCore/rendering/OrderIterator.h:59 > > + Vector<int>::const_iterator m_orderValuesIterator; > > Please use unsigned instead of Vector<int>::const_iterator and call it m_orderIndex or similar. Actually shouldn't it be OrderIterator::const_iterator ?
(In reply to comment #3) > Well that was the name when the class was inside RenderFlexibleBox, GridOrderIterator sounds too specific to grid layout and this is shared with flex box Something else then perhaps. Its real name used to be WebCore::RenderFlexibleBox::OrderIterator which is very specific. Now it is WebCore::OrderIterator which is rather generic. > Actually shouldn't it be OrderIterator::const_iterator ? You can't really define implementation OrderIterator in terms of itself.
(In reply to comment #4) > (In reply to comment #3) > > Well that was the name when the class was inside RenderFlexibleBox, GridOrderIterator sounds too specific to grid layout and this is shared with flex box > > Something else then perhaps. Its real name used to be WebCore::RenderFlexibleBox::OrderIterator which is very specific. Now it is WebCore::OrderIterator which is rather generic. > > > Actually shouldn't it be OrderIterator::const_iterator ? > > You can't really define implementation OrderIterator in terms of itself. Beh, just thought that code was inside the RenderGrid...
(In reply to comment #4) > (In reply to comment #3) > > Well that was the name when the class was inside RenderFlexibleBox, GridOrderIterator sounds too specific to grid layout and this is shared with flex box > > Something else then perhaps. Its real name used to be WebCore::RenderFlexibleBox::OrderIterator which is very specific. Now it is WebCore::OrderIterator which is rather generic. > > > Actually shouldn't it be OrderIterator::const_iterator ? > > You can't really define implementation OrderIterator in terms of itself. I meant OrderValues::const_iterator
(In reply to comment #6) > I meant OrderValues::const_iterator We generally use plain numbers for Vector indexes as it is much more understandable. Vector iterators exist to allow use of STL style algorithms over vectors.
Created attachment 215296 [details] Uploading again for review as I used std::move as suggested by Darin in http://wkb.ug/118620
Created attachment 215297 [details] Patch Uploading again for review as I used std::move as suggested by Darin in http://wkb.ug/118620
Attachment 215297 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/css-grid-layout/grid-item-order-auto-flow-resolution-expected.txt', u'LayoutTests/fast/css-grid-layout/grid-item-order-auto-flow-resolution.html', u'LayoutTests/fast/css-grid-layout/grid-item-order-paint-order-expected.html', u'LayoutTests/fast/css-grid-layout/grid-item-order-paint-order.html', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/rendering/OrderIterator.cpp', u'Source/WebCore/rendering/OrderIterator.h', u'Source/WebCore/rendering/RenderFlexibleBox.cpp', u'Source/WebCore/rendering/RenderFlexibleBox.h', u'Source/WebCore/rendering/RenderGrid.cpp', u'Source/WebCore/rendering/RenderGrid.h']" exit_code: 1 Source/WebCore/rendering/OrderIterator.h:48: Missing spaces around && [whitespace/operators] [3] Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #10) > Attachment 215297 [details] did not pass style-queue: > Source/WebCore/rendering/OrderIterator.h:48: Missing spaces around && [whitespace/operators] [3] > Total errors found: 1 in 16 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. False positive, the script is not prepared for the T&& syntax
Comment on attachment 215297 [details] Patch r=me
> False positive, the script is not prepared for the T&& syntax Would be nice to fix that.
Comment on attachment 215297 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=215297&action=review > Source/WebCore/rendering/OrderIterator.cpp:91 > + m_currentChild = 0; nullptr > Source/WebCore/rendering/OrderIterator.cpp:92 > + m_orderIndex = -1; A little irritating to use a magic number for this. Named constant would be better. > Source/WebCore/rendering/OrderIterator.h:43 > + WTF_MAKE_NONCOPYABLE(OrderIterator); If m_containerBox was a reference, we would not need to use WTF_MAKE_NONCOPYABLE, because a reference is automatically noncopyable. > Source/WebCore/rendering/OrderIterator.h:45 > + OrderIterator(const RenderBox*); Should be a reference, not a pointer. > Source/WebCore/rendering/OrderIterator.h:53 > + void reset(); This should be private. It’s only used inside the class. > Source/WebCore/rendering/OrderIterator.h:56 > + const RenderBox* m_containerBox; Should be a reference, not a pointer.
(In reply to comment #13) > > False positive, the script is not prepared for the T&& syntax > > Would be nice to fix that. I already reported it in bug 123406
(In reply to comment #14) > (From update of attachment 215297 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=215297&action=review > > > Source/WebCore/rendering/OrderIterator.cpp:91 > > + m_currentChild = 0; > > nullptr > > > Source/WebCore/rendering/OrderIterator.cpp:92 > > + m_orderIndex = -1; > > A little irritating to use a magic number for this. Named constant would be better. > > > Source/WebCore/rendering/OrderIterator.h:43 > > + WTF_MAKE_NONCOPYABLE(OrderIterator); > > If m_containerBox was a reference, we would not need to use WTF_MAKE_NONCOPYABLE, because a reference is automatically noncopyable. > > > Source/WebCore/rendering/OrderIterator.h:45 > > + OrderIterator(const RenderBox*); > > Should be a reference, not a pointer. > > > Source/WebCore/rendering/OrderIterator.h:53 > > + void reset(); > > This should be private. It’s only used inside the class. > > > Source/WebCore/rendering/OrderIterator.h:56 > > + const RenderBox* m_containerBox; > > Should be a reference, not a pointer. Thanks for the review. Note that I just moved most of the code to a new file in order to share it, that's why I tried to minimize the changes to the original code, but I'm ok with improving it.
(In reply to comment #16) > Note that I just moved most of the code to a new file in order to share it, that's why I tried to minimize the changes to the original code, but I'm ok with improving it. Yup, I noticed it was just a move. Not urgent to make those changes.
Committed r158115: <http://trac.webkit.org/changeset/158115>