Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2335673002:
[css-grid] New syntax for the 'grid' shorthand (Closed)
|
Created:
4 years, 3 months ago by jfernandez Modified:
4 years, 3 months ago CC:
chromium-reviews, jfernandez, svillar, blink-reviews-css, Manuel Rego, dglazkov+blink, apavlov+blink_chromium.org, darktears, blink-reviews, rwlbuis Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[css-grid] New syntax for the 'grid' shorthand
The spec has changed the syntax of the 'grid' shorthand.
https://drafts.csswg.org/css-grid/#grid-shorthand
In order to implement the new syntax a new keyword has
been added (auto-flow) and the already defined tests have
been changed to follow the new syntax.
BUG=618971
Committed: https://crrev.com/04b7b5477ccf1b873ca0351b6923ba1007bf294f
Cr-Commit-Position: refs/heads/master@{#419797}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Applied suggested changes. #
Total comments: 10
Patch Set 3 : Fixed bug in the parsing logic. #
Total comments: 11
Patch Set 4 : Applied suggested changes. #
Messages
Total messages: 19 (7 generated)
Description was changed from ========== [css-grid] New syntax for the 'grid' shorthand The spec has changd the syntaxt of the 'grid' shorthand. https://drafts.csswg.org/css-grid/#grid-shorthand BUG=618971 ========== to ========== [css-grid] New syntax for the 'grid' shorthand The spec has changed the syntax of the 'grid' shorthand. https://drafts.csswg.org/css-grid/#grid-shorthand In order to implement the new syntax a new keyword has been added (auto-flow) and the already defined tests has been changed to follow the new syntax. BUG=618971 ==========
jfernandez@igalia.com changed reviewers: + cbiesinger@chromium.org, rego@igalia.com, rob.buis@chromium.org, svillar@igalia.com, timloh@chromium.org
Nice that we're getting really close to the current spec. https://codereview.chromium.org/2335673002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-shorthand-get-set.html (right): https://codereview.chromium.org/2335673002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-shorthand-get-set.html:32: } Nit: you don't have a test for: grid: auto-flow / 10px; https://codereview.chromium.org/2335673002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-shorthand-get-set.html:58: grid: dense auto-flow / 10px; I believe this is right, as the spec says: [ auto-flow && dense? ] <‘grid-auto-rows’>? / <‘grid-template-columns’> So the order between auto-flow and dense is no important: https://drafts.csswg.org/css-values-4/#comb-all https://codereview.chromium.org/2335673002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-shorthand-get-set.html:88: grid: auto-flow 20px a / 10px; Nit: We could add another wrong case only with dense: grid: dense / 10px; or: grid: 10px / dense; https://codereview.chromium.org/2335673002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2335673002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4503: // 2- [ auto-flow && dense? ] <grid-auto-rows>? / <grid-template-columns> Maybe we could refactor the part that consumes: [ auto-flow && dense? ] <grid-auto-rows>? And use it in both parts, first for rows and later for columns. https://codereview.chromium.org/2335673002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4506: if (identMatches<CSSValueDense>(m_range.peek().id())) As noted on the tests "dense" could be before "auto-flow". https://codereview.chromium.org/2335673002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4509: if (!((autoRowsValue = consumeGridTrackList(m_range, m_context.mode(), GridAuto)) && consumeSlashIncludingWhitespace(m_range))) Probably it'd be easier to read in several lines. https://codereview.chromium.org/2335673002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4513: return false; We could do after this if: autoColumnsValue = CSSInitialValue::createLegacyImplicit(); templateRows = CSSInitialValue::createLegacyImplicit(); So we simplify the code later, and we don't need to check if they're or not null. The same could be done in the other part for autoRowsValue and templateColumns. https://codereview.chromium.org/2335673002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4517: return false; Wrong indentation?
Description was changed from ========== [css-grid] New syntax for the 'grid' shorthand The spec has changed the syntax of the 'grid' shorthand. https://drafts.csswg.org/css-grid/#grid-shorthand In order to implement the new syntax a new keyword has been added (auto-flow) and the already defined tests has been changed to follow the new syntax. BUG=618971 ========== to ========== [css-grid] New syntax for the 'grid' shorthand The spec has changed the syntax of the 'grid' shorthand. https://drafts.csswg.org/css-grid/#grid-shorthand In order to implement the new syntax a new keyword has been added (auto-flow) and the already defined tests have been changed to follow the new syntax. BUG=618971 ==========
https://codereview.chromium.org/2335673002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-shorthand-get-set.html (right): https://codereview.chromium.org/2335673002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-shorthand-get-set.html:32: } On 2016/09/12 20:48:17, Manuel Rego wrote: > Nit: you don't have a test for: > grid: auto-flow / 10px; Done. https://codereview.chromium.org/2335673002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-shorthand-get-set.html:58: grid: dense auto-flow / 10px; On 2016/09/12 20:48:17, Manuel Rego wrote: > I believe this is right, as the spec says: > [ auto-flow && dense? ] <‘grid-auto-rows’>? / <‘grid-template-columns’> > > So the order between auto-flow and dense is no important: > https://drafts.csswg.org/css-values-4/#comb-all Acknowledged. https://codereview.chromium.org/2335673002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-shorthand-get-set.html:88: grid: auto-flow 20px a / 10px; On 2016/09/12 20:48:17, Manuel Rego wrote: > Nit: We could add another wrong case only with dense: > grid: dense / 10px; > or: > grid: 10px / dense; Acknowledged. https://codereview.chromium.org/2335673002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2335673002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4503: // 2- [ auto-flow && dense? ] <grid-auto-rows>? / <grid-template-columns> On 2016/09/12 20:48:17, Manuel Rego wrote: > Maybe we could refactor the part that consumes: > [ auto-flow && dense? ] <grid-auto-rows>? > And use it in both parts, first for rows and later for columns. I thought about that, but it made parsing a bit more complicated and it's just 2 lines of code, so it didn't pay off. However, not that I have to modify the logic to allow 'dense' and 'auto-flow' keywords in any order it might be useful to define an independent function. https://codereview.chromium.org/2335673002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4506: if (identMatches<CSSValueDense>(m_range.peek().id())) On 2016/09/12 20:48:18, Manuel Rego wrote: > As noted on the tests "dense" could be before "auto-flow". Acknowledged. https://codereview.chromium.org/2335673002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4509: if (!((autoRowsValue = consumeGridTrackList(m_range, m_context.mode(), GridAuto)) && consumeSlashIncludingWhitespace(m_range))) On 2016/09/12 20:48:18, Manuel Rego wrote: > Probably it'd be easier to read in several lines. That was my first implementation, actually, and I changed it to reduce the number of different 'if' sentences. I understand it could be a bit more legible, but I found the whole function simpler using this approach. https://codereview.chromium.org/2335673002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4513: return false; On 2016/09/12 20:48:18, Manuel Rego wrote: > We could do after this if: > autoColumnsValue = CSSInitialValue::createLegacyImplicit(); > templateRows = CSSInitialValue::createLegacyImplicit(); > > So we simplify the code later, and we don't need to check > if they're or not null. > > The same could be done in the other part for autoRowsValue > and templateColumns. Done. https://codereview.chromium.org/2335673002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4517: return false; On 2016/09/12 20:48:17, Manuel Rego wrote: > Wrong indentation? Done.
The patch LGTM, but I'd prefer someone with more experience on the parsing code to ruber stamp it too. Please, @timloh and/or @rob.buis could you take a look? Thanks! https://codereview.chromium.org/2335673002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-shorthand-get-set.html (right): https://codereview.chromium.org/2335673002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-shorthand-get-set.html:149: Nit: Extra blank line. https://codereview.chromium.org/2335673002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2335673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4477: static CSSValueList* consumeImplicitAutoFlow(CSSParserTokenRange& range, AutoFlowType direction) You're passing the AutoFlowType, just to set the flowDirection. Maybe we can avoid this AutoFlowType enum. In that case you could change this method to something like: static bool consumeImplicitAutoFlow(CSSParserTokenRange& range, CSSValue*& denseAlogrithm) You parse "auto-flow && dense?" and then set or not the "denseAlgorithm" argument. Of course it'll return false if there's something invalid. Then in consumeGridShorthand() you'll set the flowDirection manually. Another idea I'm wondering if we could parse in this method the track list too. Maybe that would make more complex the return value and so on. https://codereview.chromium.org/2335673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4525: else if (!((autoRowsValue = consumeGridTrackList(m_range, m_context.mode(), GridAuto)) && consumeSlashIncludingWhitespace(m_range))) For me this kind of lines are complex to understand. In several lines this could be: else { autoRowsValue = consumeGridTrackList(m_range, m_context.mode(), GridAuto)); if (!consumeSlashIncludingWhitespace(m_range)) return false; } https://codereview.chromium.org/2335673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4533: if (!((templateRows = consumeGridTemplatesRowsOrColumns(m_range, m_context.mode())) && consumeSlashIncludingWhitespace(m_range) && (gridAutoFlow = consumeImplicitAutoFlow(m_range, GridColumn)))) And this one is even more complex: 3 conditions and 2 assignments in just one line.
I think this will interpret "dense 10px / auto-flow" as "10px / auto-flow" since we can consume the "dense" and then return false and try the other path. https://codereview.chromium.org/2335673002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2335673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4477: static CSSValueList* consumeImplicitAutoFlow(CSSParserTokenRange& range, AutoFlowType direction) On 2016/09/14 21:14:35, Manuel Rego wrote: > You're passing the AutoFlowType, just to set the flowDirection. > Maybe we can avoid this AutoFlowType enum. > In that case you could change this method to something like: > static bool consumeImplicitAutoFlow(CSSParserTokenRange& range, CSSValue*& > denseAlogrithm) > You parse "auto-flow && dense?" and then set or not the "denseAlgorithm" > argument. > Of course it'll return false if there's something invalid. > Then in consumeGridShorthand() you'll set the flowDirection manually. > > Another idea I'm wondering if we could parse in this method the track list too. > Maybe that would make more complex the return value and so on. I think the suggested function header here is good. https://codereview.chromium.org/2335673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4533: if (!((templateRows = consumeGridTemplatesRowsOrColumns(m_range, m_context.mode())) && consumeSlashIncludingWhitespace(m_range) && (gridAutoFlow = consumeImplicitAutoFlow(m_range, GridColumn)))) On 2016/09/14 21:14:35, Manuel Rego wrote: > And this one is even more complex: > 3 conditions and 2 assignments in just one line. This one I'd definitely split up
Thanks for both reviews. It's true that the code may have some bug, allowing "dense 10px / auto-flow", which is invalid. I'll send a new patch with this bug fixed and some of the suggested improvements. https://codereview.chromium.org/2335673002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2335673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4477: static CSSValueList* consumeImplicitAutoFlow(CSSParserTokenRange& range, AutoFlowType direction) On 2016/09/16 05:40:46, Timothy Loh wrote: > On 2016/09/14 21:14:35, Manuel Rego wrote: > > You're passing the AutoFlowType, just to set the flowDirection. > > Maybe we can avoid this AutoFlowType enum. > > In that case you could change this method to something like: > > static bool consumeImplicitAutoFlow(CSSParserTokenRange& range, CSSValue*& > > denseAlogrithm) > > You parse "auto-flow && dense?" and then set or not the "denseAlgorithm" > > argument. > > Of course it'll return false if there's something invalid. > > Then in consumeGridShorthand() you'll set the flowDirection manually. > > > > Another idea I'm wondering if we could parse in this method the track list > too. > > Maybe that would make more complex the return value and so on. > > I think the suggested function header here is good. I understand the point of avoiding the enum. However, I don't agree with the benefits of the proposed function because that would imply managing the CSSValueList outside this utility function. Notice that the solely purpose of this function is to refactoring code that it's shared among both branches of the main parsing logic. What I can do is to pass the resolved PrimitiveValue instead of the enum. However, I'd try to avoid creating any CSSValue until we know we are going to use it, since an invalid parsing would lead to an early return of the main parsing logic. https://codereview.chromium.org/2335673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4525: else if (!((autoRowsValue = consumeGridTrackList(m_range, m_context.mode(), GridAuto)) && consumeSlashIncludingWhitespace(m_range))) On 2016/09/14 21:14:34, Manuel Rego wrote: > For me this kind of lines are complex to understand. > In several lines this could be: > else { > autoRowsValue = consumeGridTrackList(m_range, m_context.mode(), GridAuto)); > if (!consumeSlashIncludingWhitespace(m_range)) > return false; > } Your proposed code is not equivalent, and incorrect BTW. The autoRowsValue must be nonNull if there is not a slash after the auto-flow component. I perfectly understand that having more lines and perhaps extra variables helps to have a more readable code, however, I think it's better to keep the parsing code as compact as possible and I really think the current design is perfectly understandable. It matches better what the syntax state; like "if NOT a slash, then we MUST have a auto-size AND a slash". https://codereview.chromium.org/2335673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4533: if (!((templateRows = consumeGridTemplatesRowsOrColumns(m_range, m_context.mode())) && consumeSlashIncludingWhitespace(m_range) && (gridAutoFlow = consumeImplicitAutoFlow(m_range, GridColumn)))) On 2016/09/16 05:40:45, Timothy Loh wrote: > On 2016/09/14 21:14:35, Manuel Rego wrote: > > And this one is even more complex: > > 3 conditions and 2 assignments in just one line. > > This one I'd definitely split up Again, I understand the point behind splitting up conditions, but this is the same case than before, but adding an additional condition. Again, it perfectly matches what syntax states; "<grid-template-rows> / [ auto-flow " All these 3 components MUST be present and it that order. I don't see the point of having 3 conditions, all of them returning false if they are not satisfied.
lgtm https://codereview.chromium.org/2335673002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2335673002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4533: if (!((templateRows = consumeGridTemplatesRowsOrColumns(m_range, m_context.mode())) && consumeSlashIncludingWhitespace(m_range) && (gridAutoFlow = consumeImplicitAutoFlow(m_range, GridColumn)))) On 2016/09/16 09:48:50, jfernandez wrote: > On 2016/09/16 05:40:45, Timothy Loh wrote: > > On 2016/09/14 21:14:35, Manuel Rego wrote: > > > And this one is even more complex: > > > 3 conditions and 2 assignments in just one line. > > > > This one I'd definitely split up > > Again, I understand the point behind splitting up conditions, but this is > the same case than before, but adding an additional condition. Again, it > perfectly matches what syntax states; "<grid-template-rows> / [ auto-flow " > All these 3 components MUST be present and it that order. I don't see the > point of having 3 conditions, all of them returning false if they are not > satisfied. OK, but let's at least use some more whitespace here: if (!(.... && .... && ....))
lgtm from the functional POV. As mentioned inline I'd prefer to have a different kind of conditions, otherwise this will be hard to understand for future readers. https://codereview.chromium.org/2335673002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2335673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4486: } Although your representation is pretty compact, I think it's rather difficult to read. Something like denseAlgorithm = consume<Dense>() if (!denseAlgorithm) return nullptr; if (!consume <AutoFlow>()) return nullptr; is more understandable IMO (granted, it uses one more conditional statement). https://codereview.chromium.org/2335673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4521: if (!(gridAutoFlow = consumeImplicitAutoFlow(m_range, GridRow))) IMO this kind of initializations are good when you want to define a variable that is going to be used inside the if block, i.e if (int i = j + 2) { // do something with "i" } but here it makes the condition more complex to understand. gridAutoFlow = consumeImplicitAutoFlow(m_range, GridRow); if (!gridAutoFlow) return false; is better IMO. https://codereview.chromium.org/2335673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4526: else if (!((autoRowsValue = consumeGridTrackList(m_range, m_context.mode(), GridAuto)) && consumeSlashIncludingWhitespace(m_range))) Again difficult to understand IMO. else { autoRowsValue = consumeGridTrackList(m_range, m_context.mode(), GridAuto)) if (!autoRowsValue) return false; if (!consumeSlash()) return false; } https://codereview.chromium.org/2335673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4528: if (!(templateColumns = consumeGridTemplatesRowsOrColumns(m_range, m_context.mode()))) Ditto. https://codereview.chromium.org/2335673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4534: if (!((templateRows = consumeGridTemplatesRowsOrColumns(m_range, m_context.mode())) && consumeSlashIncludingWhitespace(m_range) && (gridAutoFlow = consumeImplicitAutoFlow(m_range, GridColumn)))) Ditto. https://codereview.chromium.org/2335673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4538: else if (!(autoColumnsValue = consumeGridTrackList(m_range, m_context.mode(), GridAuto))) Ditto.
https://codereview.chromium.org/2335673002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2335673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4486: } On 2016/09/19 15:04:12, svillar wrote: > Although your representation is pretty compact, I think it's rather difficult to > read. Something like > > denseAlgorithm = consume<Dense>() > if (!denseAlgorithm) > return nullptr; > if (!consume <AutoFlow>()) > return nullptr; > > is more understandable IMO (granted, it uses one more conditional statement). Done. https://codereview.chromium.org/2335673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4521: if (!(gridAutoFlow = consumeImplicitAutoFlow(m_range, GridRow))) On 2016/09/19 15:04:12, svillar wrote: > IMO this kind of initializations are good when you want to define a variable > that is going to be used inside the if block, i.e > > if (int i = j + 2) { > // do something with "i" > } > > but here it makes the condition more complex to understand. > > gridAutoFlow = consumeImplicitAutoFlow(m_range, GridRow); > if (!gridAutoFlow) > return false; > > is better IMO. Done. https://codereview.chromium.org/2335673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4526: else if (!((autoRowsValue = consumeGridTrackList(m_range, m_context.mode(), GridAuto)) && consumeSlashIncludingWhitespace(m_range))) On 2016/09/19 15:04:12, svillar wrote: > Again difficult to understand IMO. > > else { > autoRowsValue = consumeGridTrackList(m_range, m_context.mode(), GridAuto)) > if (!autoRowsValue) > return false; > if (!consumeSlash()) > return false; > } Done. https://codereview.chromium.org/2335673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4534: if (!((templateRows = consumeGridTemplatesRowsOrColumns(m_range, m_context.mode())) && consumeSlashIncludingWhitespace(m_range) && (gridAutoFlow = consumeImplicitAutoFlow(m_range, GridColumn)))) On 2016/09/19 15:04:12, svillar wrote: > Ditto. Done. https://codereview.chromium.org/2335673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4538: else if (!(autoColumnsValue = consumeGridTrackList(m_range, m_context.mode(), GridAuto))) On 2016/09/19 15:04:12, svillar wrote: > Ditto. Done.
The CQ bit was checked by jfernandez@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from rego@igalia.com, svillar@igalia.com, timloh@chromium.org Link to the patchset: https://codereview.chromium.org/2335673002/#ps60001 (title: "Applied suggested changes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [css-grid] New syntax for the 'grid' shorthand The spec has changed the syntax of the 'grid' shorthand. https://drafts.csswg.org/css-grid/#grid-shorthand In order to implement the new syntax a new keyword has been added (auto-flow) and the already defined tests have been changed to follow the new syntax. BUG=618971 ========== to ========== [css-grid] New syntax for the 'grid' shorthand The spec has changed the syntax of the 'grid' shorthand. https://drafts.csswg.org/css-grid/#grid-shorthand In order to implement the new syntax a new keyword has been added (auto-flow) and the already defined tests have been changed to follow the new syntax. BUG=618971 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [css-grid] New syntax for the 'grid' shorthand The spec has changed the syntax of the 'grid' shorthand. https://drafts.csswg.org/css-grid/#grid-shorthand In order to implement the new syntax a new keyword has been added (auto-flow) and the already defined tests have been changed to follow the new syntax. BUG=618971 ========== to ========== [css-grid] New syntax for the 'grid' shorthand The spec has changed the syntax of the 'grid' shorthand. https://drafts.csswg.org/css-grid/#grid-shorthand In order to implement the new syntax a new keyword has been added (auto-flow) and the already defined tests have been changed to follow the new syntax. BUG=618971 Committed: https://crrev.com/04b7b5477ccf1b873ca0351b6923ba1007bf294f Cr-Commit-Position: refs/heads/master@{#419797} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/04b7b5477ccf1b873ca0351b6923ba1007bf294f Cr-Commit-Position: refs/heads/master@{#419797} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
