Pass CSSParserLocalContext down to CSSMathExpressionNodeParser [chromium/src : main]
Munira Tursunova (Gerrit)
Munira Tursunova added 6 comments![]( "Open in Gerrit")
Munira TursunovaIt's generally hard for me (and you later on) to tell whether the default constructions (without a property) of CSSParserLocalContext are intentional or an oversight.
I think we should disallow the default construction and require that we pass some argument that annotates why we can ignore the property. Otherwise it's hard to tell when you're done.
I can disallow the default constructor and pass a pointer to CSSPropertyName to the constructor(so we would pass nullptr if there is no property specified, for instance media queries case), what do you think?
Pass CSSParserContext down to CSSMathExpressionNodeParserMunira TursunovaCSSParserLocalContext?
Done
This CL does not include passing CSSParser context with relevantMunira Tursunova"the relevant"
Done
This CL does not include passing CSSParser context with relevantMunira Tursunova"pass a CSSParserLocalContext"
Done
property name in case of at rule descriptors and substitution functions,Munira Tursunova"for"
Done
CSSParserLocalContext local_context_2 = CSSParserLocalContext();Rune LillesveenThis is a workaround, since using the argument CSSParserLocalContext will cause `animations/interpolation/webkit-clip-path-interpolation.html` test to fail, see https://ci.chromium.org/ui/p/chromium/builders/try/linux-rel/2504640/overview. I'm not sure if the test is correct, I think we should be using alias parsing here, but using local_context here will make alias parsing for css animation, but not css transitions, needs more investigation.
Munira TursunovaCould you elaborate?
In what way is this related to transitions and animations?
What do you mean by alias parsing for transitions? Transitions work on computed values of the standardized property since this is a pure alias, right?
Before we used default constructor for local_context here. Then this value is passed to `ConsumeRadii` which checks `local_context.UseAliasParsing()` for legacy syntax pasrsing, which for default constructor will always return false.
This causes `animations/interpolation/webkit-clip-path-interpolation.html` test to fail, see https://ci.chromium.org/ui/p/chromium/builders/try/linux-rel/2504640/overview. Please check the test expectations there to better understand the problem. If I just use local_context that uses `CSSParserLocalContext` with the property, so `local_context.UseAliasParsing()` will return true for --webkit properties (failing tests uses -webkit-clip-path property). I will get errors like this:
```
[FAIL] CSS Transitions: property <-webkit-clip-path> from [inset(1px 2px round 100px 200px)] to [inset(101px 102px 101px 102px)] at (-0.3) is [inset(-29px -28px round 130px 260px)]
assert_equals: expected "inset ( - 29px - 28px round 130px / 260px ) " but got "inset ( - 29px - 28px round 130px 260px ) "
[FAIL] CSS Transitions with transition: all: property <-webkit-clip-path> from [inset(1px 2px round 100px 200px)] to [inset(101px 102px 101px 102px)] at (-0.3) is [inset(-29px -28px round 130px 260px)]
assert_equals: expected "inset ( - 29px - 28px round 130px / 260px ) " but got "inset ( - 29px - 28px round 130px 260px ) "
```
I don't know exactly why this happens only for transitions, that's why I added a workaround to use a default constructor here, so that we get the same behaviour as before this change. I can fix it later, not to expand this CL even further.
Related details
- 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. |
Munira Tursunova (Gerrit)
Munira Tursunova added 2 comments![]( "Open in Gerrit")
CSSParserLocalContext local_context_2 = CSSParserLocalContext();Munira Tursunovanit: Variable name 'local_context_2' contains a number. Since the 'CSSParserLocalContext&' parameter is unnamed, consider naming this variable 'local_context'. (Blink Style Guide: Naming)
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Done
CSSParserLocalContext& local_context) const {Munira TursunovaParameter 'local_context' appears to be unused. Consider omitting the name to avoid unused parameter warnings.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Done
Related details
- 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. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 3 comments![]( "Open in Gerrit")
Munira TursunovaIt's generally hard for me (and you later on) to tell whether the default constructions (without a property) of CSSParserLocalContext are intentional or an oversight.
I think we should disallow the default construction and require that we pass some argument that annotates why we can ignore the property. Otherwise it's hard to tell when you're done.
I can disallow the default constructor and pass a pointer to CSSPropertyName to the constructor(so we would pass nullptr if there is no property specified, for instance media queries case), what do you think?
I can disallow the default constructor and pass a pointer to CSSPropertyName to the constructor(so we would pass nullptr if there is no property specified, for instance media queries case), what do you think?
That is basically the same as the default constructor. I want to make sure for every case we're not passing a property that I can immediately read from the construction whether it is done intentionally, whether it is covered by the spec or needs a spec resolution, or if it should be a TODO because we haven't propagated the property where we need it.
It's currently super-hard for me to review what the case is for these default constructor instances, and unless you document your intent, it's impossible for anyone coming back to this code later to understand what your intent was.
Take typed OM as an example. If it's OK to pass a CSSParserLocalContext without a property when parsing typed OM values, make a constructor or a function for constructing that context that you use commonly for all such invocations, and document that constructor/function in a way that it's clear that it's intentional that we don't pass a property, and that we will populate the random() function values with the correct property when the value is set for an actual property (assuming that's how it's supposed to work).
CSSParserLocalContext local_context_2 = CSSParserLocalContext();nit: Variable name 'local_context_2' contains a number. Since the 'CSSParserLocalContext&' parameter is unnamed, consider naming this variable 'local_context'. (Blink Style Guide: Naming)
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
+1
CSSParserLocalContext local_context_2 = CSSParserLocalContext();Rune LillesveenThis is a workaround, since using the argument CSSParserLocalContext will cause `animations/interpolation/webkit-clip-path-interpolation.html` test to fail, see https://ci.chromium.org/ui/p/chromium/builders/try/linux-rel/2504640/overview. I'm not sure if the test is correct, I think we should be using alias parsing here, but using local_context here will make alias parsing for css animation, but not css transitions, needs more investigation.
Munira TursunovaCould you elaborate?
In what way is this related to transitions and animations?
What do you mean by alias parsing for transitions? Transitions work on computed values of the standardized property since this is a pure alias, right?
Before we used default constructor for local_context here. Then this value is passed to `ConsumeRadii` which checks `local_context.UseAliasParsing()` for legacy syntax pasrsing, which for default constructor will always return false.
This causes `animations/interpolation/webkit-clip-path-interpolation.html` test to fail, see https://ci.chromium.org/ui/p/chromium/builders/try/linux-rel/2504640/overview. Please check the test expectations there to better understand the problem. If I just use local_context that uses `CSSParserLocalContext` with the property, so `local_context.UseAliasParsing()` will return true for --webkit properties (failing tests uses -webkit-clip-path property). I will get errors like this:
```
[FAIL] CSS Transitions: property <-webkit-clip-path> from [inset(1px 2px round 100px 200px)] to [inset(101px 102px 101px 102px)] at (-0.3) is [inset(-29px -28px round 130px 260px)]
assert_equals: expected "inset ( - 29px - 28px round 130px / 260px ) " but got "inset ( - 29px - 28px round 130px 260px ) "
[FAIL] CSS Transitions with transition: all: property <-webkit-clip-path> from [inset(1px 2px round 100px 200px)] to [inset(101px 102px 101px 102px)] at (-0.3) is [inset(-29px -28px round 130px 260px)]
assert_equals: expected "inset ( - 29px - 28px round 130px / 260px ) " but got "inset ( - 29px - 28px round 130px 260px ) "
```I don't know exactly why this happens only for transitions, that's why I added a workaround to use a default constructor here, so that we get the same behaviour as before this change. I can fix it later, not to expand this CL even further.
This causes `animations/interpolation/webkit-clip-path-interpolation.html` test to fail, see https://ci.chromium.org/ui/p/chromium/builders/try/linux-rel/2504640/overview. Please check the test expectations there to better understand the problem.
Yes, I did see that. I wanted you to explain it to me so I didn't have to dive into the problem :-)
I don't know exactly why this happens only for transitions, that's why I added a workaround to use a default constructor here, so that we get the same behaviour as before this change. I can fix it later, not to expand this CL even further.
- Munira Tursunova
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. |
Munira Tursunova (Gerrit)
Munira Tursunova added 5 comments![]( "Open in Gerrit")
Munira TursunovaWould it be possible to split this into smaller CLs?
Munira TursunovaI don't think so, there is not much logical changes here, all the logical changes are in `third_party/blink/renderer/core/css/properties/css_parsing_utils.cc`, the rest is mostly creating blank CSSParserContext and passing it as an new argument to the functions, so that it would compile.
Done
Munira TursunovaIt's generally hard for me (and you later on) to tell whether the default constructions (without a property) of CSSParserLocalContext are intentional or an oversight.
I think we should disallow the default construction and require that we pass some argument that annotates why we can ignore the property. Otherwise it's hard to tell when you're done.
Rune LillesveenI can disallow the default constructor and pass a pointer to CSSPropertyName to the constructor(so we would pass nullptr if there is no property specified, for instance media queries case), what do you think?
I can disallow the default constructor and pass a pointer to CSSPropertyName to the constructor(so we would pass nullptr if there is no property specified, for instance media queries case), what do you think?
That is basically the same as the default constructor. I want to make sure for every case we're not passing a property that I can immediately read from the construction whether it is done intentionally, whether it is covered by the spec or needs a spec resolution, or if it should be a TODO because we haven't propagated the property where we need it.
It's currently super-hard for me to review what the case is for these default constructor instances, and unless you document your intent, it's impossible for anyone coming back to this code later to understand what your intent was.
Take typed OM as an example. If it's OK to pass a CSSParserLocalContext without a property when parsing typed OM values, make a constructor or a function for constructing that context that you use commonly for all such invocations, and document that constructor/function in a way that it's clear that it's intentional that we don't pass a property, and that we will populate the random() function values with the correct property when the value is set for an actual property (assuming that's how it's supposed to work).
Added separate constructors for groups of cases without property names as we discussed offline.
CSSParserLocalContext local_context = CSSParserLocalContext();Munira TursunovaWhy the assignment here? Why not just `CSSParserLocalContext local_context;`?
(Goes for all similar declarations)
But also see the general comment about default construction.
Done
CSSParserLocalContext local_context_2 = CSSParserLocalContext();Rune Lillesveennit: Variable name 'local_context_2' contains a number. Since the 'CSSParserLocalContext&' parameter is unnamed, consider naming this variable 'local_context'. (Blink Style Guide: Naming)
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
+1
Done already?
CSSParserLocalContext local_context_2 = CSSParserLocalContext();Rune LillesveenThis is a workaround, since using the argument CSSParserLocalContext will cause `animations/interpolation/webkit-clip-path-interpolation.html` test to fail, see https://ci.chromium.org/ui/p/chromium/builders/try/linux-rel/2504640/overview. I'm not sure if the test is correct, I think we should be using alias parsing here, but using local_context here will make alias parsing for css animation, but not css transitions, needs more investigation.
Munira TursunovaCould you elaborate?
In what way is this related to transitions and animations?
What do you mean by alias parsing for transitions? Transitions work on computed values of the standardized property since this is a pure alias, right?
Rune LillesveenBefore we used default constructor for local_context here. Then this value is passed to `ConsumeRadii` which checks `local_context.UseAliasParsing()` for legacy syntax pasrsing, which for default constructor will always return false.
This causes `animations/interpolation/webkit-clip-path-interpolation.html` test to fail, see https://ci.chromium.org/ui/p/chromium/builders/try/linux-rel/2504640/overview. Please check the test expectations there to better understand the problem. If I just use local_context that uses `CSSParserLocalContext` with the property, so `local_context.UseAliasParsing()` will return true for --webkit properties (failing tests uses -webkit-clip-path property). I will get errors like this:
```
[FAIL] CSS Transitions: property <-webkit-clip-path> from [inset(1px 2px round 100px 200px)] to [inset(101px 102px 101px 102px)] at (-0.3) is [inset(-29px -28px round 130px 260px)]
assert_equals: expected "inset ( - 29px - 28px round 130px / 260px ) " but got "inset ( - 29px - 28px round 130px 260px ) "
[FAIL] CSS Transitions with transition: all: property <-webkit-clip-path> from [inset(1px 2px round 100px 200px)] to [inset(101px 102px 101px 102px)] at (-0.3) is [inset(-29px -28px round 130px 260px)]
assert_equals: expected "inset ( - 29px - 28px round 130px / 260px ) " but got "inset ( - 29px - 28px round 130px 260px ) "
```I don't know exactly why this happens only for transitions, that's why I added a workaround to use a default constructor here, so that we get the same behaviour as before this change. I can fix it later, not to expand this CL even further.
This causes `animations/interpolation/webkit-clip-path-interpolation.html` test to fail, see https://ci.chromium.org/ui/p/chromium/builders/try/linux-rel/2504640/overview. Please check the test expectations there to better understand the problem.
Yes, I did see that. I wanted you to explain it to me so I didn't have to dive into the problem :-)
I don't know exactly why this happens only for transitions, that's why I added a workaround to use a default constructor here, so that we get the same behaviour as before this change. I can fix it later, not to expand this CL even further.
Could you leave a TODO then?
- 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. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 1 comment![]( "Open in Gerrit")
static CSSParserLocalContext CreateWithoutPropertyForCSSOM() {Now, each of these need documentation.
For the cases where we clearly don't need a property covered by the spec, say why a property is not relevant.
For the cases that needs fixing, or needs spec clarification, please add TODOs with an explanation.
Related details
- Munira Tursunova
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. |
Munira Tursunova (Gerrit)
Munira Tursunova added 2 comments![]( "Open in Gerrit")
static CSSParserLocalContext CreateWithoutPropertyForCSSOM() {Now, each of these need documentation.
For the cases where we clearly don't need a property covered by the spec, say why a property is not relevant.
For the cases that needs fixing, or needs spec clarification, please add TODOs with an explanation.
Done
CSSParserLocalContext local_context_2 = CSSParserLocalContext();Rune Lillesveennit: Variable name 'local_context_2' contains a number. Since the 'CSSParserLocalContext&' parameter is unnamed, consider naming this variable 'local_context'. (Blink Style Guide: Naming)
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Munira Tursunova+1
Done already?
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |