Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(124)

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

Created:
3 years, 7 months ago by Manuel Rego
Modified:
3 years, 6 months ago
CC:
chromium-reviews, jfernandez, szager+layoutwatch_chromium.org, blink-reviews-style_chromium.org, zoltan1, svillar, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, Manuel Rego, leviw+renderwatch, jchaffraix+rendering, blink-reviews, eae+blinkwatch
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[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: https://crrev.com/7433487e4082c928a06ddff51fe5c963a28da3ef 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

Messages

Total messages: 17 (5 generated)
Manuel Rego
3 years, 7 months ago (2015-12-17 12:58:45 UTC) #2
svillar
The mechanics look good but I have many doubts about the proposed API (although perhaps ...
3 years, 7 months ago (2015-12-18 13:08:30 UTC) #3
jfernandez
https://codereview.chromium.org/1529083006/diff/20001/third_party/WebKit/LayoutTests/fast/css-grid-layout/implicit-tracks-before-explicit.html File third_party/WebKit/LayoutTests/fast/css-grid-layout/implicit-tracks-before-explicit.html (right): https://codereview.chromium.org/1529083006/diff/20001/third_party/WebKit/LayoutTests/fast/css-grid-layout/implicit-tracks-before-explicit.html#newcode57 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 ...
3 years, 7 months 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. https://codereview.chromium.org/1529083006/diff/20001/third_party/WebKit/LayoutTests/fast/css-grid-layout/implicit-tracks-before-explicit.html ...
3 years, 7 months ago (2015-12-18 22:42:30 UTC) #6
svillar
The naming is indeed much better but I think we still need another iteration. https://codereview.chromium.org/1529083006/diff/40001/third_party/WebKit/Source/core/style/GridCoordinate.h ...
3 years, 6 months ago (2016-01-04 09:10:06 UTC) #7
Manuel Rego
Thanks for the review. https://codereview.chromium.org/1529083006/diff/40001/third_party/WebKit/Source/core/style/GridCoordinate.h File third_party/WebKit/Source/core/style/GridCoordinate.h (right): https://codereview.chromium.org/1529083006/diff/40001/third_party/WebKit/Source/core/style/GridCoordinate.h#newcode62 third_party/WebKit/Source/core/style/GridCoordinate.h:62: On 2016/01/04 09:10:06, svillar wrote: ...
3 years, 6 months ago (2016-01-04 10:00:11 UTC) #8
svillar
OK, lgtm for layout/, you'll need other owners for style/ and parser/ stuff
3 years, 6 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?
3 years, 6 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 ...
3 years, 6 months ago (2016-01-05 02:31:47 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1529083006/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1529083006/40001
3 years, 6 months ago (2016-01-05 10:31:18 UTC) #14
commit-bot: I haz the power
Committed patchset #3 (id:40001)
3 years, 6 months ago (2016-01-05 12:55:52 UTC) #15
commit-bot: I haz the power
3 years, 6 months ago (2016-01-05 12:56:46 UTC) #17
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/7433487e4082c928a06ddff51fe5c963a28da3ef
Cr-Commit-Position: refs/heads/master@{#367511}

Powered by Google App Engine
This is Rietveld 408576698