[grid] Fix parsing of <grid-line> [chromium/src : main]
Ian Kilpatrick (Gerrit)
Ian Kilpatrick voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ian Kilpatrick
| Commit-Queue | +1 |
1 comment
return nullptr;Do I need a `savepoint.Release()` here?
Related details
- Alison Maher
- Anders Hartvoll Ruud
- Kurt Catti-Schmidt
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. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 1 comment![]( "Open in Gerrit")
assert_equals: serialization should be canonical expected "span calc(-2)" but got "span"@andruud after this patch I'll shift the bug to Blink>CSS as this looks like a general serialization issue with calc(-2) etc in properties.
Related details
- Alison Maher
- Anders Hartvoll Ruud
- Kurt Catti-Schmidt
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 added 5 comments![]( "Open in Gerrit")
auto is_at_end = [&stream]() -> bool {General question - I've seen these sort of var functions be camel case and snake case in different parts of the code. Couldn't find any info in the style guide for this, other than camel case in general is preferred for functions and snake case for accessors is ok.
Do you know if there is a preferred naming schema for these?
// <custom-ident>In the method above this, we use ConsumeGridLineNames() for the custom-ident. Can we reuse that here, or will this be slightly different?
ConsumeCustomIdentForGridLine(stream, context, local_context);Same here, can we use ConsumeGridLineNames (although since we use the same savepoint for this and the int case, might not be as clean to reuse, but if we split it up, it might be possible)
if (!span_value) {I thought span has to come first. Why do we check it again after we check for the count and line name values as opposed to returning nullptr if the span keyword doesn't exist when we query it at line 7368?
if (is_span_position) {nit: Could you add a comment for this new logic?
Related details
- Anders Hartvoll Ruud
- Ian Kilpatrick
- Kurt Catti-Schmidt
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. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 5 comments![]( "Open in Gerrit")
General question - I've seen these sort of var functions be camel case and snake case in different parts of the code. Couldn't find any info in the style guide for this, other than camel case in general is preferred for functions and snake case for accessors is ok.
Do you know if there is a preferred naming schema for these?
Yeah there was a change in style at some point, but now we just follow -
https://google.github.io/styleguide/cppguide.html#Formatting_Lambda_Expressions
e.g. just treat it as a local variable. Ok just to match surrounding code, or convert existing code.
In the method above this, we use ConsumeGridLineNames() for the custom-ident. Can we reuse that here, or will this be slightly different?
I had this previously, but its tricky to get correct due to the unwinding in other branches.
E.g. if we do:
```
CSSCustomIdentValue* line_name = ConsumeCustom();
{
CSSParserSavePoint savepoint(stream);
line_name = ConsumeCustom(); // writes a new value as <custom-ident> was trailing.
}
// Rewinds - but leaves line_name as old value. Would have to add logic to clear.
```Basically its simpler to reason about if every block only touches its local variables.
ConsumeCustomIdentForGridLine(stream, context, local_context);Same here, can we use ConsumeGridLineNames (although since we use the same savepoint for this and the int case, might not be as clean to reuse, but if we split it up, it might be possible)
See above - its tricky to get correct with rewinding, and ends up being more complex to follow.
I thought span has to come first. Why do we check it again after we check for the count and line name values as opposed to returning nullptr if the span keyword doesn't exist when we query it at line 7368?
`span` can be trailing - the grammar is weird basically.
The following are valid:
```
span 1 foo
span foo 1
foo 1 span
1 foo span
```
The `&&` means it can be before or after, however it can't be in the general loop as this is invalid:
```
foo span 1
```
nit: Could you add a comment for this new logic?
- Alison Maher
- Anders Hartvoll Ruud
- Kurt Catti-Schmidt
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 1 comment![]( "Open in Gerrit")
Votes added by Alison Maher
| Code-Review | +1 |
1 comment
if (!span_value) {Ian KilpatrickI thought span has to come first. Why do we check it again after we check for the count and line name values as opposed to returning nullptr if the span keyword doesn't exist when we query it at line 7368?
`span` can be trailing - the grammar is weird basically.
The following are valid:
```
span 1 foo
span foo 1
foo 1 span
1 foo span
```The `&&` means it can be before or after, however it can't be in the general loop as this is invalid:
```
foo span 1
```
- Anders Hartvoll Ruud
- Ian Kilpatrick
- Kurt Catti-Schmidt
- Rune Lillesveen
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. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 4 comments![]( "Open in Gerrit")
@almaher - I removed these tests as they are obsolete. Additionally didn't add any new ones as I realized we don't technically need to be clamping within StyleBuilderConverter::ConvertGridPosition either.
Technically we should be allowing max<int>() and relying on the clamping logic within GridSpan here:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/style/grid_area.h;l=187-200;drc=2e070bfa61f747388fddaafd8eaf9519f89e80de?q=gridspan&ss=chromium
However we need a little more hardening as we do "+ 1" and "- 1" in the codebase on raw ints.
assert_equals: serialization should be canonical expected "span calc(-2)" but got "span"@andruud after this patch I'll shift the bug to Blink>CSS as this looks like a general serialization issue with calc(-2) etc in properties.
Acknowledged
assert_equals: serialization should be canonical expected "span calc(sibling-index() - 2)" but got "span calc(-2 + sibling-index())"@futhark - after this patch I'll shift the bug to Blink>CSS as this looks like a general serialization issue with calc(sibling-index() - 2) potentially? Or is the test wrong?
Related details
- Alison Maher
- Anders Hartvoll Ruud
- Kurt Catti-Schmidt
- Rune Lillesveen
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 1 comment![]( "Open in Gerrit")
Votes added by Alison Maher
| Code-Review | +1 |
1 comment
@almaher - I removed these tests as they are obsolete. Additionally didn't add any new ones as I realized we don't technically need to be clamping within StyleBuilderConverter::ConvertGridPosition either.
Technically we should be allowing max<int>() and relying on the clamping logic within GridSpan here:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/style/grid_area.h;l=187-200;drc=2e070bfa61f747388fddaafd8eaf9519f89e80de?q=gridspan&ss=chromiumHowever we need a little more hardening as we do "+ 1" and "- 1" in the codebase on raw ints.
- Anders Hartvoll Ruud
- Ian Kilpatrick
- Kurt Catti-Schmidt
- Rune Lillesveen
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. |
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt added 2 comments![]( "Open in Gerrit")
if (line_count->GetValueIfKnown() == 0) {Random, non-blocking: do we know what the expected behavior is for `span calc(0)`? I don't see any WPT's for this scenario.
// We must have the 'span' ident.
if (!span_value) {
return nullptr;
}Nit: Would this be cleaner in the conditional body above?
```
if (!span_value) {
if(!(span_value = ConsumeIdent<CSSValueID::kSpan>(stream))) {
// We must have the 'span' ident.
return nullptr;
}
}
```
Related details
- Anders Hartvoll Ruud
- Ian Kilpatrick
- Rune Lillesveen