[Masonry] Update grid-lanes-direction syntax [chromium/src : main]
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt added 7 comments![]( "Open in Gerrit")
CHECK_EQ(value->GetValueID(), CSSValueID::kNormal);What about just `row` or `column`? Per the grammar above, this is allowed:
`'grid-lanes-direction: normal | [ row | column ] [ fill-reverse
|| track-reverse ]?'`
EDIT: Looking below, it looks like those get parsed as a list with one entry. Could use a comment here indicating that.
CHECK_GE(list->length(), 1u);
return To<CSSIdentifierValue>(&list->Item(0))->GetValueID() ==
CSSValueID::kColumn;Same here, `row` and `column` are valid identifiers for this, so this looks like a bug. Could use a comment.
CSSValueID second_reverse_id = stream.Peek().Id();
if ((second_reverse_id != CSSValueID::kFillReverse &&
second_reverse_id != CSSValueID::kTrackReverse) ||
second_reverse_id == first_reverse_id) {
return list;
}This will allow parsing the same reverse keyword twice, e.g. `row fill-reverse fill-reverse` or `row track-reverse track-reverse`. You'll need to add some bools to determine whether `fill-reverse` or `track-reverse` was previously parsed.
bool has_both_reversals = list.length() == 3;nit: `const bool`
GridLanesOrientation orientation;Nit: for consistency, this can be
```
GridLanesOrientation orientation = GridLanesOrientation::kNormal;
```
...and you can remove it from the default constructor
bool operator==(const GridLanesDirection& other) const {
return orientation == other.orientation &&
is_fill_reverse == other.is_fill_reverse &&
is_track_reverse == other.is_track_reverse;
}
bool operator!=(const GridLanesDirection& other) const {
return !(*this == other);
}Since these are trivial memberwise comparisons, I believe they can both be `= default;` instead of the function body.
test_invalid_value("grid-lanes-direction", "column reverse");Could use cases for duplicated `fill-reverse` and `track-reverse` as mentioned above:
```
test_invalid_value("grid-lanes-direction", "row fill-reverse fill-reverse");
test_invalid_value("grid-lanes-direction", "row track-reverse track-reverse");
```
You also might want to add some `!important` tokens in the middle, e.g.
```
test_invalid_value("grid-lanes-direction", "row !important fill-reverse fill-reverse");
test_invalid_value("grid-lanes-direction", "row track-reverse !important fill-reverse");
```
Related details
- Alison Maher
- Ian Kilpatrick
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher voted and added 7 comments![]( "Open in Gerrit")
Votes added by Alison Maher
| Commit-Queue | +1 |
7 comments
CHECK_EQ(value->GetValueID(), CSSValueID::kNormal);What about just `row` or `column`? Per the grammar above, this is allowed:
`'grid-lanes-direction: normal | [ row | column ] [ fill-reverse
|| track-reverse ]?'`EDIT: Looking below, it looks like those get parsed as a list with one entry. Could use a comment here indicating that.
Yeah, if normal also allowed the revsere keywords, I would have done everything as a list so we didn't have different paths for this. But comment added to make it more obvious
CHECK_GE(list->length(), 1u);
return To<CSSIdentifierValue>(&list->Item(0))->GetValueID() ==
CSSValueID::kColumn;Same here, `row` and `column` are valid identifiers for this, so this looks like a bug. Could use a comment.
Done
CSSValueID second_reverse_id = stream.Peek().Id();
if ((second_reverse_id != CSSValueID::kFillReverse &&
second_reverse_id != CSSValueID::kTrackReverse) ||
second_reverse_id == first_reverse_id) {
return list;
}This will allow parsing the same reverse keyword twice, e.g. `row fill-reverse fill-reverse` or `row track-reverse track-reverse`. You'll need to add some bools to determine whether `fill-reverse` or `track-reverse` was previously parsed.
This is actually covered by the check that `second_reverse_id == first_reverse_id`, we will return early, then the callers will check if we are at the end of the stream, and treat it as invalid if not.
Added additional test cases, though
bool has_both_reversals = list.length() == 3;Alison Mahernit: `const bool`
Done
Nit: for consistency, this can be
```
GridLanesOrientation orientation = GridLanesOrientation::kNormal;
```...and you can remove it from the default constructor
Done
bool operator==(const GridLanesDirection& other) const {
return orientation == other.orientation &&
is_fill_reverse == other.is_fill_reverse &&
is_track_reverse == other.is_track_reverse;
}
bool operator!=(const GridLanesDirection& other) const {
return !(*this == other);
}Since these are trivial memberwise comparisons, I believe they can both be `= default;` instead of the function body.
Done
test_invalid_value("grid-lanes-direction", "column reverse");Could use cases for duplicated `fill-reverse` and `track-reverse` as mentioned above:
```
test_invalid_value("grid-lanes-direction", "row fill-reverse fill-reverse");
test_invalid_value("grid-lanes-direction", "row track-reverse track-reverse");
```You also might want to add some `!important` tokens in the middle, e.g.
```
test_invalid_value("grid-lanes-direction", "row !important fill-reverse fill-reverse");
test_invalid_value("grid-lanes-direction", "row track-reverse !important fill-reverse");
```
- Ian Kilpatrick
- Kurt Catti-Schmidt
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |