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

Issue 166623002: [CSS Grid Layout] Introduce an explicit type for resolved grid positions (Closed)

Created:
5 years, 10 months ago by Manuel Rego
Modified:
5 years, 8 months ago
CC:
blink-reviews, jfernandez, leviw+renderwatch, zoltan1, ed+blinkwatch_opera.com, eae+blinkwatch, dglazkov+blink, apavlov+blink_chromium.org, jchaffraix+rendering, darktears, bemjb+rendering_chromium.org, pdr., dsinclair, rune+blink
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

[CSS Grid Layout] Introduce an explicit type for resolved grid positions We were using simple size_t integers to represent resolved grid positions in our internal data structures. This change allows us to clarify the code, avoid potential off by one mistakes, and move the resolving code to a central place. Based in a patch by Xan Lopez <xlopez@igalia.com>; https://codereview.chromium.org/113943003/. BUG=258258 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171156

Patch Set 1 #

Total comments: 3

Patch Set 2 : Removed implicit constructor and renamed getter and setter #

Patch Set 3 : Full patch including all the missing bits in previous one #

Total comments: 9

Patch Set 4 : Apply suggested changes #

Total comments: 2

Patch Set 5 : Applied review suggestions #

Total comments: 3

Patch Set 6 : Add toInt() and remove size_t operator and comparators #

Patch Set 7 : Fix compilation issue #

Unified diffs Side-by-side diffs Delta from patch set Stats (+407 lines, -314 lines) Patch
M Source/core/core.gypi View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/css/CSSGridTemplateAreasValue.cpp View 1 2 3 4 5 1 chunk +2 lines, -2 lines 0 comments Download
M Source/core/css/parser/CSSPropertyParser.cpp View 1 2 3 4 5 1 chunk +4 lines, -4 lines 0 comments Download
M Source/core/rendering/RenderGrid.h View 1 2 3 4 5 chunks +2 lines, -18 lines 0 comments Download
M Source/core/rendering/RenderGrid.cpp View 1 2 3 4 5 21 chunks +65 lines, -241 lines 0 comments Download
M Source/core/rendering/style/GridCoordinate.h View 1 2 3 4 5 3 chunks +29 lines, -28 lines 0 comments Download
M Source/core/rendering/style/GridPosition.h View 1 2 3 2 chunks +0 lines, -21 lines 0 comments Download
A Source/core/rendering/style/GridResolvedPosition.h View 1 2 3 4 5 6 1 chunk +114 lines, -0 lines 0 comments Download
A Source/core/rendering/style/GridResolvedPosition.cpp View 1 2 3 4 1 chunk +189 lines, -0 lines 0 comments Download

Messages

Total messages: 20 (0 generated)
Manuel Rego
5 years, 10 months ago (2014-02-14 11:56:46 UTC) #1
Julien - ping for review
Sorry for the delay (my review queue is winning these days). What's your longer term ...
5 years, 9 months ago (2014-03-06 22:38:02 UTC) #2
Manuel Rego
On 2014/03/06 22:38:02, Julien Chaffraix - PST wrote: > Sorry for the delay (my review ...
5 years, 9 months ago (2014-03-07 14:32:14 UTC) #3
Julien - ping for review
On 2014/03/07 14:32:14, Manuel Rego wrote: > On 2014/03/06 22:38:02, Julien Chaffraix - PST wrote: ...
5 years, 9 months ago (2014-03-10 18:29:04 UTC) #4
Manuel Rego
On 2014/03/10 18:29:04, Julien Chaffraix - PST wrote: > As it seems that your follow-up ...
5 years, 9 months ago (2014-03-11 17:00:26 UTC) #5
Julien - ping for review
The gist of the patch is here. Some comments but it looks pretty neat now. ...
5 years, 8 months ago (2014-03-21 23:49:27 UTC) #6
Manuel Rego
Thanks for the review, I've just uploaded a new version of the patch. On 2014/03/21 ...
5 years, 8 months ago (2014-03-24 23:28:26 UTC) #7
Julien - ping for review
lgtm. Elliott should really be the one to give the final review as it's what ...
5 years, 8 months ago (2014-04-03 22:26:18 UTC) #8
Manuel Rego
On 2014/04/03 22:26:18, Julien Chaffraix - PST wrote: > lgtm. Uploaded new version with the ...
5 years, 8 months ago (2014-04-04 21:44:54 UTC) #9
esprehn
This seems okay, I don't remember all the details of the original patch though. lgtm ...
5 years, 8 months ago (2014-04-08 22:14:58 UTC) #10
Manuel Rego
On 2014/04/08 22:14:58, esprehn wrote: > This seems okay, I don't remember all the details ...
5 years, 8 months ago (2014-04-08 22:42:32 UTC) #11
svillar
On 2014/04/08 22:42:32, Manuel Rego wrote: > On 2014/04/08 22:14:58, esprehn wrote: > https://codereview.chromium.org/166623002/diff/120001/Source/core/rendering/style/GridResolvedPosition.h#newcode97 > ...
5 years, 8 months ago (2014-04-09 08:30:45 UTC) #12
Manuel Rego
On 2014/04/09 08:30:45, svillar wrote: > On 2014/04/08 22:42:32, Manuel Rego wrote: > > On ...
5 years, 8 months ago (2014-04-09 16:32:15 UTC) #13
Manuel Rego
The CQ bit was checked by rego@igalia.com
5 years, 8 months ago (2014-04-09 16:32:20 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rego@igalia.com/166623002/140001
5 years, 8 months ago (2014-04-09 16:32:25 UTC) #15
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
5 years, 8 months ago (2014-04-09 17:05:20 UTC) #16
commit-bot: I haz the power
Try jobs failed on following builders: tryserver.blink on mac_blink_compile_dbg
5 years, 8 months ago (2014-04-09 17:05:20 UTC) #17
Manuel Rego
The CQ bit was checked by rego@igalia.com
5 years, 8 months ago (2014-04-09 17:23:01 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rego@igalia.com/166623002/160001
5 years, 8 months ago (2014-04-09 17:23:11 UTC) #19
commit-bot: I haz the power
5 years, 8 months ago (2014-04-09 18:43:27 UTC) #20
Message was sent while issue was closed.
Change committed as 171156

Powered by Google App Engine
This is Rietveld 408576698