Bug 123208 – [CSS Grid Layout] Add support for order inside grid items

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Sergio Villar Senin]

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

[Sergio Villar Senin]

Created attachment 215041 [details]
Patch

[Antti Koivisto]

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.

[Sergio Villar Senin]

(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 ?

[Antti Koivisto]

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

[Sergio Villar Senin]

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

[Sergio Villar Senin]

(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

[Antti Koivisto]

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

[Sergio Villar Senin]

Created attachment 215296 [details]
Uploading again for review as I used std::move as suggested by Darin in http://wkb.ug/118620

[Sergio Villar Senin]

Created attachment 215297 [details]
Patch

Uploading again for review as I used std::move as suggested by Darin in http://wkb.ug/118620

[WebKit Commit Bot]

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.

[Sergio Villar Senin]

(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

[Antti Koivisto]

Comment on attachment 215297 [details]
Patch

r=me

[Antti Koivisto]

> False positive, the script is not prepared for the T&& syntax

Would be nice to fix that.

[Darin Adler]

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.

[Sergio Villar Senin]

(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

[Sergio Villar Senin]

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

[Darin Adler]

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

[Sergio Villar Senin]

Committed r158115: <http://trac.webkit.org/changeset/158115>