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

Issue 1451883002: [css-grid] Store lines instead of tracks in GridResolvedPosition (Closed)

Created:
3 years, 4 months ago by Manuel Rego
Modified:
3 years, 4 months ago
CC:
chromium-reviews, jfernandez, szager+layoutwatch_chromium.org, blink-reviews-style_chromium.org, zoltan1, svillar, blink-reviews-css, pdr+renderingwatchlist_chromium.org, Manuel Rego, leviw+renderwatch, slimming-paint-reviews_chromium.org, blink-reviews-paint_chromium.org, dglazkov+blink, dshwang, apavlov+blink_chromium.org, jchaffraix+rendering, darktears, blink-reviews, eae+blinkwatch, blink-reviews-layout_chromium.org, rwlbuis
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[css-grid] Store lines instead of tracks in GridResolvedPosition Due to the new feature that allows to create implicit tracks before the explicit ones, we will need to use lines instead of tracks in the code to be able to implement it properly. This is just a first simple patch using lines instead of tracks in GridResolvedPosition. It modifies the code that was using it, as it was considering that the resolvedFinalPosition was a track index and not a line index. So if we've an item positioned like: grid-column: 2 / 5; grid-row: 1 / span 2; Before we were storing this information on the GridSpan: * columns: * resolvedInitialPosition: 1 * resolvedFinalPosition: 3 * rows: * resolvedInitialPosition: 0 * resolvedFinalPosition: 1 And now we're storing: * columns: * resolvedInitialPosition: 1 * resolvedFinalPosition: 4 * rows: * resolvedInitialPosition: 0 * resolvedFinalPosition: 2 No new tests, no change of behavior. BUG=444011 Committed: https://crrev.com/2031b1892af1ca54c5b85660f6f369979e548fdb Cr-Commit-Position: refs/heads/master@{#360361}

Patch Set 1 #

Total comments: 12

Patch Set 2 : Fixed review comments #

Total comments: 2

Patch Set 3 : Patch for landing #

Patch Set 4 : Fix ASSERT on the tests #

Unified diffs Side-by-side diffs Delta from patch set Stats (+77 lines, -92 lines) Patch
M third_party/WebKit/Source/core/css/CSSGridTemplateAreasValue.cpp View 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp View 1 2 2 chunks +5 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutGrid.h View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutGrid.cpp View 1 10 chunks +19 lines, -22 lines 0 comments Download
M third_party/WebKit/Source/core/paint/GridPainter.cpp View 1 1 chunk +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/style/GridCoordinate.h View 1 2 3 8 chunks +22 lines, -25 lines 0 comments Download
M third_party/WebKit/Source/core/style/GridResolvedPosition.h View 3 chunks +6 lines, -22 lines 0 comments Download
M third_party/WebKit/Source/core/style/GridResolvedPosition.cpp View 1 9 chunks +19 lines, -13 lines 0 comments Download

Messages

Total messages: 23 (11 generated)
Manuel Rego
This is the first step to support the "implicit grid before the explicit grid" feature. ...
3 years, 4 months ago (2015-11-16 20:57:41 UTC) #2
svillar
layout/ lgtm. Definitely a step in the right direction. We could simplify some code later ...
3 years, 4 months ago (2015-11-17 11:37:06 UTC) #5
Manuel Rego
Thanks for the review, uploaded new patch fixing the comments. Adding @dsinclair and @timloh as ...
3 years, 4 months ago (2015-11-17 14:30:49 UTC) #7
Timothy Loh
css/ and style/ lgtm I'm not familiar with grid yet, but I'll read over the ...
3 years, 4 months ago (2015-11-18 06:41:37 UTC) #8
Manuel Rego
Thanks for the review. https://codereview.chromium.org/1451883002/diff/20001/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp File third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp (right): https://codereview.chromium.org/1451883002/diff/20001/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp#newcode2969 third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:2969: for (lookAheadCol = currentCol; lookAheadCol ...
3 years, 4 months ago (2015-11-18 08:39:15 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1451883002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1451883002/40001
3 years, 4 months ago (2015-11-18 08:40:00 UTC) #12
commit-bot: I haz the power
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/142381)
3 years, 4 months ago (2015-11-18 10:04:57 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1451883002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1451883002/40001
3 years, 4 months ago (2015-11-18 13:37:24 UTC) #16
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/142435)
3 years, 4 months ago (2015-11-18 14:46:33 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1451883002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1451883002/60001
3 years, 4 months ago (2015-11-18 16:00:45 UTC) #21
commit-bot: I haz the power
Committed patchset #4 (id:60001)
3 years, 4 months ago (2015-11-18 17:58:20 UTC) #22
commit-bot: I haz the power
3 years, 4 months ago (2015-11-18 17:59:32 UTC) #23
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/2031b1892af1ca54c5b85660f6f369979e548fdb
Cr-Commit-Position: refs/heads/master@{#360361}

Powered by Google App Engine
This is Rietveld 408576698