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

Issue 1500433003: [css-grid] Get rid of GridResolvedPosition (Closed)

Created:
3 years, 7 months ago by Manuel Rego
Modified:
3 years, 7 months ago
Reviewers:
cbiesinger, svillar, rune
CC:
chromium-reviews, jfernandez, szager+layoutwatch_chromium.org, blink-reviews-style_chromium.org, zoltan1, svillar, blink-reviews-css, pdr+renderingwatchlist_chromium.org, 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] Get rid of GridResolvedPosition GridResolvedPosition was a small class just wrapping a size_t. In the future it should actually wrap an integer (as we want to support implicit tracks before the explicit grid). The class itself is not providing any benefit, so we can get rid of it and store directly 2 size_t in GridSpan. This will make simpler future changes related to this task. We keep the class just as a utility for the methods that deal with the positions resolution. But it should be renamed in a follow-up patch. No new tests, no change of behavior. BUG=444011 Committed: https://crrev.com/3ad965028770aec5042ea3587843aec66027e1da Cr-Commit-Position: refs/heads/master@{#363384}

Patch Set 1 #

Total comments: 1

Patch Set 2 : New version fixing compilation issues #

Total comments: 9

Patch Set 3 : Adding ASSERTs #

Patch Set 4 : Change operators overloaded in GridSpanIterator #

Unified diffs Side-by-side diffs Delta from patch set Stats (+126 lines, -180 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 +4 lines, -4 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 1 22 chunks +51 lines, -52 lines 0 comments Download
M third_party/WebKit/Source/core/paint/GridPainter.cpp View 1 chunk +3 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/style/GridCoordinate.h View 1 2 3 3 chunks +22 lines, -12 lines 0 comments Download
M third_party/WebKit/Source/core/style/GridResolvedPosition.h View 1 2 chunks +3 lines, -67 lines 0 comments Download
M third_party/WebKit/Source/core/style/GridResolvedPosition.cpp View 1 2 11 chunks +39 lines, -38 lines 0 comments Download

Messages

Total messages: 32 (12 generated)
Manuel Rego
Another refactoring related to the grid placement code.
3 years, 7 months ago (2015-12-03 08:59:42 UTC) #2
rune
I think it was better with the overflow checks wrapped inside a method. Couldn't you ...
3 years, 7 months ago (2015-12-03 09:36:49 UTC) #3
Manuel Rego
On 2015/12/03 09:36:49, rune wrote: > I think it was better with the overflow checks ...
3 years, 7 months ago (2015-12-03 12:48:40 UTC) #4
rune
I've proposed some ASSERTs. Otherwise lgtm. https://codereview.chromium.org/1500433003/diff/20001/third_party/WebKit/Source/core/style/GridCoordinate.h File third_party/WebKit/Source/core/style/GridCoordinate.h (left): https://codereview.chromium.org/1500433003/diff/20001/third_party/WebKit/Source/core/style/GridCoordinate.h#oldcode70 third_party/WebKit/Source/core/style/GridCoordinate.h:70: ASSERT(isDefinite()); ASSERT(m_resolvedFinalPosition >= ...
3 years, 7 months ago (2015-12-03 14:08:38 UTC) #5
cbiesinger
https://codereview.chromium.org/1500433003/diff/20001/third_party/WebKit/Source/core/style/GridCoordinate.h File third_party/WebKit/Source/core/style/GridCoordinate.h (right): https://codereview.chromium.org/1500433003/diff/20001/third_party/WebKit/Source/core/style/GridCoordinate.h#newcode86 third_party/WebKit/Source/core/style/GridCoordinate.h:86: struct GridSpanIterator { How does this work as an ...
3 years, 7 months ago (2015-12-03 17:19:17 UTC) #6
Manuel Rego
Thanks for the reviews! https://codereview.chromium.org/1500433003/diff/20001/third_party/WebKit/Source/core/style/GridCoordinate.h File third_party/WebKit/Source/core/style/GridCoordinate.h (left): https://codereview.chromium.org/1500433003/diff/20001/third_party/WebKit/Source/core/style/GridCoordinate.h#oldcode70 third_party/WebKit/Source/core/style/GridCoordinate.h:70: ASSERT(isDefinite()); On 2015/12/03 14:08:38, rune ...
3 years, 7 months ago (2015-12-04 07:28:06 UTC) #7
Manuel Rego
https://codereview.chromium.org/1500433003/diff/20001/third_party/WebKit/Source/core/style/GridCoordinate.h File third_party/WebKit/Source/core/style/GridCoordinate.h (right): https://codereview.chromium.org/1500433003/diff/20001/third_party/WebKit/Source/core/style/GridCoordinate.h#newcode86 third_party/WebKit/Source/core/style/GridCoordinate.h:86: struct GridSpanIterator { On 2015/12/03 17:19:17, cbiesinger wrote: > ...
3 years, 7 months ago (2015-12-04 07:30:21 UTC) #8
cbiesinger
https://codereview.chromium.org/1500433003/diff/20001/third_party/WebKit/Source/core/style/GridCoordinate.h File third_party/WebKit/Source/core/style/GridCoordinate.h (right): https://codereview.chromium.org/1500433003/diff/20001/third_party/WebKit/Source/core/style/GridCoordinate.h#newcode86 third_party/WebKit/Source/core/style/GridCoordinate.h:86: struct GridSpanIterator { On 2015/12/04 07:30:21, Manuel Rego wrote: ...
3 years, 7 months ago (2015-12-04 21:06:29 UTC) #9
cbiesinger
lgtm with that change
3 years, 7 months ago (2015-12-04 21:13:11 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1500433003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1500433003/60001
3 years, 7 months ago (2015-12-04 21:41:01 UTC) #13
commit-bot: I haz the power
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_rel_ng/builds/105598)
3 years, 7 months ago (2015-12-04 23:04:54 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1500433003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1500433003/60001
3 years, 7 months ago (2015-12-05 08:51:20 UTC) #17
commit-bot: I haz the power
Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL) win_chromium_rel_ng on ...
3 years, 7 months ago (2015-12-05 10:52:16 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1500433003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1500433003/60001
3 years, 7 months ago (2015-12-05 13:26:57 UTC) #21
commit-bot: I haz the power
Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL) win_chromium_rel_ng on ...
3 years, 7 months ago (2015-12-05 15:28:39 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1500433003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1500433003/60001
3 years, 7 months ago (2015-12-06 09:48:26 UTC) #25
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/150715)
3 years, 7 months ago (2015-12-06 11:29:48 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1500433003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1500433003/60001
3 years, 7 months ago (2015-12-06 22:03:18 UTC) #29
commit-bot: I haz the power
Committed patchset #4 (id:60001)
3 years, 7 months ago (2015-12-06 22:41:33 UTC) #30
commit-bot: I haz the power
3 years, 7 months ago (2015-12-06 22:42:33 UTC) #32
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/3ad965028770aec5042ea3587843aec66027e1da
Cr-Commit-Position: refs/heads/master@{#363384}

Powered by Google App Engine
This is Rietveld 408576698