WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
123208
[CSS Grid Layout] Add support for order inside grid items
https://bugs.webkit.org/show_bug.cgi?id=123208
Summary
[CSS Grid Layout] Add support for order inside grid items
Sergio Villar Senin
Reported
2013-10-23 08:08:25 PDT
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
Attachments
Patch
(32.93 KB, patch)
2013-10-24 03:09 PDT
,
Sergio Villar Senin
no flags
Details
Formatted Diff
Diff
Uploading again for review as I used std::move as suggested by Darin in http://wkb.ug/118620
(33.52 KB, patch)
2013-10-28 05:16 PDT
,
Sergio Villar Senin
no flags
Details
Formatted Diff
Diff
Patch
(33.52 KB, patch)
2013-10-28 05:17 PDT
,
Sergio Villar Senin
koivisto
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Sergio Villar Senin
Comment 1
2013-10-24 03:09:23 PDT
Created
attachment 215041
[details]
Patch
Antti Koivisto
Comment 2
2013-10-24 04:14:15 PDT
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
Comment 3
2013-10-24 04:48:13 PDT
(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
Comment 4
2013-10-24 05:59:16 PDT
(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
Comment 5
2013-10-24 08:19:59 PDT
(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
Comment 6
2013-10-24 08:41:42 PDT
(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
Comment 7
2013-10-24 09:24:46 PDT
(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
Comment 8
2013-10-28 05:16:30 PDT
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
Comment 9
2013-10-28 05:17:31 PDT
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
Comment 10
2013-10-28 05:19:21 PDT
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
Comment 11
2013-10-28 08:52:47 PDT
(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 12
2013-10-28 09:18:57 PDT
Comment on
attachment 215297
[details]
Patch r=me
Antti Koivisto
Comment 13
2013-10-28 09:20:26 PDT
> False positive, the script is not prepared for the T&& syntax
Would be nice to fix that.
Darin Adler
Comment 14
2013-10-28 09:29:09 PDT
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
Comment 15
2013-10-28 09:56:35 PDT
(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
Comment 16
2013-10-28 09:57:46 PDT
(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
Comment 17
2013-10-28 10:02:07 PDT
(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
Comment 18
2013-10-28 10:34:16 PDT
Committed
r158115
: <
http://trac.webkit.org/changeset/158115
>
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