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

Issue 1459373002: [css-grid] Refactor GridSpan to avoid pointers (Closed)

Created:
3 years, 6 months ago by Manuel Rego
Modified:
3 years, 5 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] Refactor GridSpan to avoid pointers Add new boolean to know if a GridSpan is definite or indefinite. That way we don't need to use pointers and we can always have two GridSpans in GridCoordinate, if the position is "auto" the GridSpan will be marked as indefinite. This will allow in a follow-up patch to avoid repeated calls to methods that resolve positions. Most operations in GridSpan are restricted to definite GridSpans (access to positions, iterator, etc.). For indefinite GridSpans we only need to know that they're indefinite we shouldn't use the rest of the data. No new tests, no change of behavior. BUG=444011 Committed: https://crrev.com/1ac0ca9794efed848b7546eeb54fdf7d7850c680 Cr-Commit-Position: refs/heads/master@{#361119}

Patch Set 1 #

Total comments: 4

Patch Set 2 : Fixed review comments #

Total comments: 2

Patch Set 3 : Patch for landing #

Unified diffs Side-by-side diffs Delta from patch set Stats (+141 lines, -109 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 chunk +5 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp View 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutGrid.cpp View 20 chunks +50 lines, -50 lines 0 comments Download
M third_party/WebKit/Source/core/paint/GridPainter.cpp View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/style/GridCoordinate.h View 1 2 5 chunks +64 lines, -32 lines 0 comments Download
M third_party/WebKit/Source/core/style/GridResolvedPosition.h View 1 chunk +3 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/style/GridResolvedPosition.cpp View 6 chunks +14 lines, -14 lines 0 comments Download

Messages

Total messages: 12 (4 generated)
Manuel Rego
New refactoring patch following the plan described at http://code.google.com/p/chromium/issues/detail?id=444011#c7
3 years, 6 months ago (2015-11-19 21:43:12 UTC) #2
cbiesinger
lgtm with nits below https://codereview.chromium.org/1459373002/diff/1/third_party/WebKit/Source/core/style/GridCoordinate.h File third_party/WebKit/Source/core/style/GridCoordinate.h (right): https://codereview.chromium.org/1459373002/diff/1/third_party/WebKit/Source/core/style/GridCoordinate.h#newcode119 third_party/WebKit/Source/core/style/GridCoordinate.h:119: ASSERT(isDefinite()); Seems slightly weird that ...
3 years, 6 months ago (2015-11-19 22:22:08 UTC) #3
Manuel Rego
Thanks for the review, now we need a OWNER for the other folders. https://codereview.chromium.org/1459373002/diff/1/third_party/WebKit/Source/core/style/GridCoordinate.h File ...
3 years, 6 months ago (2015-11-20 08:53:45 UTC) #4
rune
lgtm with nits. https://codereview.chromium.org/1459373002/diff/20001/third_party/WebKit/Source/core/style/GridCoordinate.h File third_party/WebKit/Source/core/style/GridCoordinate.h (right): https://codereview.chromium.org/1459373002/diff/20001/third_party/WebKit/Source/core/style/GridCoordinate.h#newcode61 third_party/WebKit/Source/core/style/GridCoordinate.h:61: span.m_definite = false; I would make ...
3 years, 5 months ago (2015-11-23 10:30:35 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1459373002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1459373002/40001
3 years, 5 months ago (2015-11-23 15:08:20 UTC) #9
Manuel Rego
Thanks for the review! https://codereview.chromium.org/1459373002/diff/20001/third_party/WebKit/Source/core/style/GridCoordinate.h File third_party/WebKit/Source/core/style/GridCoordinate.h (right): https://codereview.chromium.org/1459373002/diff/20001/third_party/WebKit/Source/core/style/GridCoordinate.h#newcode61 third_party/WebKit/Source/core/style/GridCoordinate.h:61: span.m_definite = false; On 2015/11/23 ...
3 years, 5 months ago (2015-11-23 15:27:14 UTC) #10
commit-bot: I haz the power
Committed patchset #3 (id:40001)
3 years, 5 months ago (2015-11-23 16:25:42 UTC) #11
commit-bot: I haz the power
3 years, 5 months ago (2015-11-23 16:26:47 UTC) #12
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/1ac0ca9794efed848b7546eeb54fdf7d7850c680
Cr-Commit-Position: refs/heads/master@{#361119}

Powered by Google App Engine
This is Rietveld 408576698