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

Issue 1424913009: [css-grid] Simplify the interface of GridResolvedPosition (Closed)

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

Description

[css-grid] Simplify the interface of GridResolvedPosition The interface of GridResolvedPosition is full of static methods that are used only internally, we should not expose them. Apart from that the resolveXXX() methods return pointers because we wanted to have a wait to notify the caller that the resolution did not succeed. Instead of doing that, resolveGridPositionsFromStyle() do always return a valid GridSpan from now on, meaning that the caller has to ensure that the resolution does not require running the auto-placement algorithm. A new class called GridUnresolvedSpan was added for that purpose.

Patch Set 1 #

Total comments: 8

Patch Set 2 : Patch for landing #

Patch Set 3 : Patch for landing v2 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+273 lines, -226 lines) Patch
M third_party/WebKit/Source/core/layout/LayoutGrid.cpp View 1 6 chunks +59 lines, -48 lines 0 comments Download
M third_party/WebKit/Source/core/style/GridCoordinate.h View 1 chunk +0 lines, -59 lines 0 comments Download
M third_party/WebKit/Source/core/style/GridPosition.h View 1 chunk +7 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/style/GridResolvedPosition.h View 1 2 2 chunks +27 lines, -41 lines 0 comments Download
M third_party/WebKit/Source/core/style/GridResolvedPosition.cpp View 1 2 8 chunks +180 lines, -78 lines 2 comments Download

Messages

Total messages: 18 (6 generated)
svillar
Sending for review
3 years, 6 months ago (2015-11-09 11:31:22 UTC) #2
Manuel Rego
Non-owner LGTM. Nice refactoring, just some small comments inline. https://codereview.chromium.org/1424913009/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/1424913009/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp#newcode1450 third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1450: ...
3 years, 6 months ago (2015-11-10 09:08:03 UTC) #3
svillar
Thanks for the review! https://codereview.chromium.org/1424913009/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/1424913009/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp#newcode1450 third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1450: offset = LayoutUnit(0); On 2015/11/10 ...
3 years, 6 months ago (2015-11-10 10:56:38 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1424913009/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1424913009/20001
3 years, 6 months ago (2015-11-11 11:12:53 UTC) #7
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/117443)
3 years, 6 months ago (2015-11-11 11:16:46 UTC) #9
svillar
Removed unused constructor
3 years, 6 months ago (2015-11-11 12:57:13 UTC) #10
svillar
rune@ could you please take a look? We need an OWNER for the core/style/ changes. ...
3 years, 6 months ago (2015-11-11 13:45:49 UTC) #12
svillar
3 years, 6 months ago (2015-11-11 15:18:01 UTC) #14
esprehn
Can you do the move and the simplification in two patches?
3 years, 6 months ago (2015-11-11 18:02:01 UTC) #15
rune
https://codereview.chromium.org/1424913009/diff/40001/third_party/WebKit/Source/core/style/GridResolvedPosition.cpp File third_party/WebKit/Source/core/style/GridResolvedPosition.cpp (right): https://codereview.chromium.org/1424913009/diff/40001/third_party/WebKit/Source/core/style/GridResolvedPosition.cpp#newcode13 third_party/WebKit/Source/core/style/GridResolvedPosition.cpp:13: static GridSpan resolveNamedGridLinePositionAgainstOppositePosition(const ComputedStyle& gridContainerStyle, const GridResolvedPosition& resolvedOppositePosition, const ...
3 years, 6 months ago (2015-11-11 18:16:52 UTC) #16
Manuel Rego
I've been thinking about all the placement structs/classes we currently have and I'm not sure ...
3 years, 6 months ago (2015-11-12 10:01:24 UTC) #17
svillar
3 years, 6 months ago (2015-11-12 16:06:17 UTC) #18
Message was sent while issue was closed.
On 2015/11/12 10:01:24, Manuel Rego wrote:
> I've been thinking about all the placement structs/classes we currently have
> and I'm not sure if they're the right structure anymore.
> 
> Since a while ago, the grid spec has changed and it allows now
> to add implicit tracks before the explicit grid (on the top/left).
> There's a bug to fix that, but now more issues are depending on it
> as more parts of the spec need this behavior:
> https://code.google.com/p/chromium/issues/detail?id=444011
> 
> We'd need to differentiate things like "grid-column: auto / 1;"
> vs "grid-column: auto / 2;". And for that we might need to store
> the grid lines instead of the GridResolvedPositions.
> 
> I'm still considering different alternatives, but if my current approach is
> right
> we might be refactoring these structures and code in a while.
> So I'd keep this patch on hold until we clarify those issues,
> and maybe reject it eventually.
> 
> I'd add more info in the bug commented above
> explaining my conclusions on the topic.

OK let's forget about this one then.

Powered by Google App Engine
This is Rietveld 408576698