Chromium Code Reviews (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out

Issue 1529083006: [css-grid] Initial support for implicit grid before explicit grid (Closed)

5 years ago by Manuel Rego
4 years, 11 months ago
chromium-reviews, jfernandez,,, zoltan1, svillar,,, Manuel Rego, leviw+renderwatch, jchaffraix+rendering, blink-reviews, eae+blinkwatch
Base URL:
Target Ref:


[css-grid] Initial support for implicit grid before explicit grid Change GridSpan to store int instead of size_t. This allows us to resolve positions before the explicit grid with negative values. This patch adds a new type of GridSpan called "Untranslated". This type is only used in populateExplicitGridAndOrderIterator(). Where we store the smallest negative position in both axis. Then the GridSpans are translated into positive values, using the offset calculated before. This is done in placeItemsOnGrid() and from that moment the rest of the code uses "Definite" GridSpans, which returns only positive positions (size_t instead of int). This allows us to don't have to modify the rest of the code, as it keeps using GridSpans as before. Let's use an example to explain how it works. Imagine that we've a 2 columns grid and 2 items placed like: * Item A: grid-column: -5; * Item B: grid-column: 1; Initially we'll use "Unstranslated" GridSpans with the following values: * Item A: GridSpan(-2, -1) * Item B: GridSpan(0, 1) Then we'll translate them using the smallest position as offset (-2) so we've "Definite" GridSpans: * Item A: GridSpan(0, 1) * Item B: GridSpan(2, 3) Updated results in current tests and added specific test for this. BUG=444011 TEST=fast/css-grid-layout/implicit-tracks-before-explicit.html Committed: Cr-Commit-Position: refs/heads/master@{#367511}

Patch Set 1 #

Patch Set 2 : Try to fix Windows build #

Total comments: 24

Patch Set 3 : Apply suggested changes #

Total comments: 2


Total messages: 17 (5 generated)
Manuel Rego
5 years ago (2015-12-17 12:58:45 UTC) #2
The mechanics look good but I have many doubts about the proposed API (although perhaps ...
5 years ago (2015-12-18 13:08:30 UTC) #3
jfernandez File third_party/WebKit/LayoutTests/fast/css-grid-layout/implicit-tracks-before-explicit.html (right): third_party/WebKit/LayoutTests/fast/css-grid-layout/implicit-tracks-before-explicit.html:57: <div class="item autoRowAutoColumn" data-offset-x="0" data-offset-y="0" data-expected-width="100" data-expected-height="50">XX</div> Where is ...
5 years ago (2015-12-18 14:33:22 UTC) #5
Manuel Rego
Thanks for the reviews! I've renamed the GridSpan types, I hope it's clearer now. ...
5 years ago (2015-12-18 22:42:30 UTC) #6
The naming is indeed much better but I think we still need another iteration. ...
4 years, 11 months ago (2016-01-04 09:10:06 UTC) #7
Manuel Rego
Thanks for the review. File third_party/WebKit/Source/core/style/GridCoordinate.h (right): third_party/WebKit/Source/core/style/GridCoordinate.h:62: On 2016/01/04 09:10:06, svillar wrote: ...
4 years, 11 months ago (2016-01-04 10:00:11 UTC) #8
OK, lgtm for layout/, you'll need other owners for style/ and parser/ stuff
4 years, 11 months ago (2016-01-04 11:01:05 UTC) #9
Manuel Rego
Thanks for the new review! @rune and/or @timloh could you please take a look?
4 years, 11 months ago (2016-01-04 11:35:58 UTC) #11
Timothy Loh
On 2016/01/04 11:35:58, Manuel Rego wrote: > Thanks for the new review! > > @rune ...
4 years, 11 months ago (2016-01-05 02:31:47 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at View timeline at
4 years, 11 months ago (2016-01-05 10:31:18 UTC) #14
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years, 11 months ago (2016-01-05 12:55:52 UTC) #15
commit-bot: I haz the power
4 years, 11 months ago (2016-01-05 12:56:46 UTC) #17
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
Cr-Commit-Position: refs/heads/master@{#367511}

Powered by Google App Engine
This is Rietveld 408576698