Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(254)

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

Created:
5 years ago by Manuel Rego
Modified:
5 years 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.
5 years 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 ...
5 years 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 ...
5 years 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 >= ...
5 years 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 ...
5 years 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 ...
5 years 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: > ...
5 years 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: ...
5 years ago (2015-12-04 21:06:29 UTC) #9
cbiesinger
lgtm with that change
5 years 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
5 years 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)
5 years 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
5 years 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 ...
5 years 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
5 years 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 ...
5 years 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
5 years 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)
5 years 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
5 years ago (2015-12-06 22:03:18 UTC) #29
commit-bot: I haz the power
Committed patchset #4 (id:60001)
5 years ago (2015-12-06 22:41:33 UTC) #30
commit-bot: I haz the power
5 years 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