| Summary: | [CSS Grid Layout] Implement the auto-placement algorithm without grid growth | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Julien Chaffraix <jchaffraix> | ||||||||
| Component: | Layout and Rendering | Assignee: | Julien Chaffraix <jchaffraix> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | eric, ojan.autocc, ojan, tony, webkit.review.bot, xan.lopez | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 103316 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Julien Chaffraix
2013-02-19 16:07:06 PST
Created attachment 189200 [details]
Proposed patch: Change is big due to testing and scaffolding code. Splittable on demand.
Comment on attachment 189200 [details] Proposed patch: Change is big due to testing and scaffolding code. Splittable on demand. View in context: https://bugs.webkit.org/attachment.cgi?id=189200&action=review This is pretty straightforward. > Source/WebCore/ChangeLog:16 > + are no empty grid area. If we don't find any empty grid area, we just insert in the first Nit: area -> areas (both uses). > Source/WebCore/rendering/RenderGrid.cpp:100 > + PassOwnPtr<GridCoordinate> firstEmptyGridArea() Nit: I think nextEmptyGridArea would be more clear. first sounds like you're resetting the value. > Source/WebCore/rendering/RenderGrid.cpp:102 > + if (!m_grid.size()) Nit: m_grid.isEmpty() > Source/WebCore/rendering/RenderGrid.cpp:534 > + placeDefinedMajorAxisItemsOnGrid(autoGridItems); > + placeAutoMajorAxisItemsOnGrid(autoGridItems); It's a bit unfortunate that we have to loop through autoGridItems twice. Maybe we should have 2 vectors, one for defined and one for auto? > LayoutTests/fast/css-grid-layout/grid-item-addition-auto-placement-update-expected.txt:14 > +FAIL: > +Expected 30 for height, but got 50. Can you add some comments next to the test cases that fail (in the .html file) explaining the failures? These don't have to show up in the expected file. > LayoutTests/fast/css-grid-layout/grid-item-removal-auto-placement-update-expected.txt:5 > +FAIL: > +Expected 170 for width, but got 50. Comments for these failures too. Comment on attachment 189200 [details] Proposed patch: Change is big due to testing and scaffolding code. Splittable on demand. View in context: https://bugs.webkit.org/attachment.cgi?id=189200&action=review >> Source/WebCore/rendering/RenderGrid.cpp:100 >> + PassOwnPtr<GridCoordinate> firstEmptyGridArea() > > Nit: I think nextEmptyGridArea would be more clear. first sounds like you're resetting the value. There is one issue with this renaming: as written, you can't call nextEmptyGridArea in a loop or else you would loop indefinitely. That's because we don't advance the iterator when we find an empty grid area and will always return the first grid area if called repeatedly. I thought firstEmptyGridArea would match better the intent that it shouldn't to be called in a loop. I am fine with fixing the function to be more loop friendly and do the renaming if that's something you prefer. However I don't know of a use for such a function in the current specification (note that the column / row span is completely omitted in the specification which I raised http://lists.w3.org/Archives/Public/www-style/2013Feb/0468.html so there *may* be a use sometimes in the future). >> Source/WebCore/rendering/RenderGrid.cpp:534 >> + placeAutoMajorAxisItemsOnGrid(autoGridItems); > > It's a bit unfortunate that we have to loop through autoGridItems twice. Maybe we should have 2 vectors, one for defined and one for auto? That should be possible to do :) > Source/WebCore/rendering/RenderGrid.cpp:537 > +void RenderGrid::placeDefinedMajorAxisItemsOnGrid(Vector<RenderBox*> autoGridItems) I reread the specification and they use explicitly specified - sometimes just specified - to refer to what I called 'defined'. We are probably better matching them here. >> LayoutTests/fast/css-grid-layout/grid-item-addition-auto-placement-update-expected.txt:14 >> +Expected 30 for height, but got 50. > > Can you add some comments next to the test cases that fail (in the .html file) explaining the failures? These don't have to show up in the expected file. I could but then my next patch is to implement the missing piece: handle growing the grid. This will fix all tests and make them pass (they fail due to our bad fallback to inserting the grid item in (1, 1)). Comment on attachment 189200 [details] Proposed patch: Change is big due to testing and scaffolding code. Splittable on demand. View in context: https://bugs.webkit.org/attachment.cgi?id=189200&action=review >>> Source/WebCore/rendering/RenderGrid.cpp:100 >>> + PassOwnPtr<GridCoordinate> firstEmptyGridArea() >> >> Nit: I think nextEmptyGridArea would be more clear. first sounds like you're resetting the value. > > There is one issue with this renaming: as written, you can't call nextEmptyGridArea in a loop or else you would loop indefinitely. That's because we don't advance the iterator when we find an empty grid area and will always return the first grid area if called repeatedly. > > I thought firstEmptyGridArea would match better the intent that it shouldn't to be called in a loop. I am fine with fixing the function to be more loop friendly and do the renaming if that's something you prefer. However I don't know of a use for such a function in the current specification (note that the column / row span is completely omitted in the specification which I raised http://lists.w3.org/Archives/Public/www-style/2013Feb/0468.html so there *may* be a use sometimes in the future). I see. Maybe call it findNextEmptyGridArea? >>> LayoutTests/fast/css-grid-layout/grid-item-addition-auto-placement-update-expected.txt:14 >>> +Expected 30 for height, but got 50. >> >> Can you add some comments next to the test cases that fail (in the .html file) explaining the failures? These don't have to show up in the expected file. > > I could but then my next patch is to implement the missing piece: handle growing the grid. This will fix all tests and make them pass (they fail due to our bad fallback to inserting the grid item in (1, 1)). Ok, please include a note of this in LayoutTests/ChangeLog. I now see the note in WebCore/ChangeLog but I had forgotten about it by the time I got to reviewing the tests. Comment on attachment 189200 [details] Proposed patch: Change is big due to testing and scaffolding code. Splittable on demand. View in context: https://bugs.webkit.org/attachment.cgi?id=189200&action=review >>>> Source/WebCore/rendering/RenderGrid.cpp:100 >>>> + PassOwnPtr<GridCoordinate> firstEmptyGridArea() >>> >>> Nit: I think nextEmptyGridArea would be more clear. first sounds like you're resetting the value. >> >> There is one issue with this renaming: as written, you can't call nextEmptyGridArea in a loop or else you would loop indefinitely. That's because we don't advance the iterator when we find an empty grid area and will always return the first grid area if called repeatedly. >> >> I thought firstEmptyGridArea would match better the intent that it shouldn't to be called in a loop. I am fine with fixing the function to be more loop friendly and do the renaming if that's something you prefer. However I don't know of a use for such a function in the current specification (note that the column / row span is completely omitted in the specification which I raised http://lists.w3.org/Archives/Public/www-style/2013Feb/0468.html so there *may* be a use sometimes in the future). > > I see. Maybe call it findNextEmptyGridArea? If we are going down that route, I prefer nextEmptyGridArea as it's shorter. I will just fix the function. >>>> LayoutTests/fast/css-grid-layout/grid-item-addition-auto-placement-update-expected.txt:14 >>>> +Expected 30 for height, but got 50. >>> >>> Can you add some comments next to the test cases that fail (in the .html file) explaining the failures? These don't have to show up in the expected file. >> >> I could but then my next patch is to implement the missing piece: handle growing the grid. This will fix all tests and make them pass (they fail due to our bad fallback to inserting the grid item in (1, 1)). > > Ok, please include a note of this in LayoutTests/ChangeLog. I now see the note in WebCore/ChangeLog but I had forgotten about it by the time I got to reviewing the tests. Good catch, I will do that. Created attachment 189400 [details]
Patch for landing
Comment on attachment 189400 [details] Patch for landing Rejecting attachment 189400 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-01', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue Last 500 characters of output: cs -fvisibility-inlines-hidden -Wsign-compare -c ../../Source/WebCore/rendering/RenderGrid.cpp -o obj/Source/WebCore/rendering/webcore_rendering.RenderGrid.o cc1plus: warnings being treated as errors ../../Source/WebCore/rendering/RenderGrid.cpp: In member function 'void WebCore::RenderGrid::placeAutoMajorAxisItemsOnGrid(WTF::Vector<WebCore::RenderBox*, 0ul>)': ../../Source/WebCore/rendering/RenderGrid.cpp:571: error: unused variable 'majorAxisPosition' ninja: build stopped: subcommand failed. Full output: http://queues.webkit.org/results/16658570 Created attachment 189405 [details]
Patch for landing
Comment on attachment 189405 [details] Patch for landing Clearing flags on attachment: 189405 Committed r143535: <http://trac.webkit.org/changeset/143535> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by bug 110523 Tool epic fail! It reopened / blocked this bug on an unrelated rollout. |