{at: 0, expect: '0px'},
this feels like this test case was wrong to begin with. If it is inheriting from the parent, why would it start out at 0px rather than at 30px, like the parent does?
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Looks good overall tbh, few things to clarify here and there but things looking good fr
using CSSColumnRuleColorInterpolationType =
The typedefs used here seem repeated for both Column/Row. Is it possible to have just two rather than four (i.e. `CSSRuleColorInterpolationType` && `CSSRuleWidthInterpolationType`).
// int specializations (column-rule-width & row-rule-width)
Guessing StyleColor specialization doesn't need the other `Maybe*` funcs
CSSGapDataListInterpolationType<StyleColor>::GetGapDataListForProperty(
Question: Should this be called `GetDataListForProperty` or just `GetProperty`, I ask because this isn't really getting the underlying data list and right after this is called we then call `GetGapDataList()` on the returned val.
So maybe `GetProperty()` or keep as `GetDataListForProperty()` but it returns `property.GetDataList`
result.push_back(gap_data.GetValue());
Would also add a `TODO` that says this is expected to fail for repeaters for now.
if (property.PropertyID() == CSSPropertyID::kColumnRuleColor) {
nit (if you want), but block could be neater as per:
```
rule_color = property.PropertyID() == CSSPropertyID::kColumnRuleColor ?
style.ColumnRuleColor() : style.RowRuleColor();
for (const auto& gap_data : rule_color.GetGapDataList()) {
result.push_back(gap_data.GetValue());
}
```
applicable_types->push_back(
nit (if you want): Instead of breaking switch cases into `CSSLength*` -> `CSSGap*` -> `CSSLength*)`, we can just pull out the Rule width properties and have them at the npttp, similar to aspect ratio, grid ...
I wonder if we'd want to keep this duplicated in both places rather than moving to gap decor folder ... Not sure though
{at: 0, expect: '0px'},
this feels like this test case was wrong to begin with. If it is inheriting from the parent, why would it start out at 0px rather than at 30px, like the parent does?
I ran into this with a patch I had but I don't think we should run into this for this change. Let's sync offline about this...
But essentially it should start from 0px because although it inherited from the parent, the computed value should be 0px because style is none. That logic shouldn't change in this change.
{at: 0, expect: '0px'},
this feels like this test case was wrong to begin with. If it is inheriting from the parent, why would it start out at 0px rather than at 30px, like the parent does?
This is because of the 0px when none behavior (*I think*)
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
The typedefs used here seem repeated for both Column/Row. Is it possible to have just two rather than four (i.e. `CSSRuleColorInterpolationType` && `CSSRuleWidthInterpolationType`).
I ended up not using them anywhere so just removing.
// int specializations (column-rule-width & row-rule-width)
Guessing StyleColor specialization doesn't need the other `Maybe*` funcs
It does, they're just not implemented in this patch.
CSSGapDataListInterpolationType<StyleColor>::GetGapDataListForProperty(
Question: Should this be called `GetDataListForProperty` or just `GetProperty`, I ask because this isn't really getting the underlying data list and right after this is called we then call `GetGapDataList()` on the returned val.
So maybe `GetProperty()` or keep as `GetDataListForProperty()` but it returns `property.GetDataList`
Done
Would also add a `TODO` that says this is expected to fail for repeaters for now.
Done
if (property.PropertyID() == CSSPropertyID::kColumnRuleColor) {
nit (if you want), but block could be neater as per:
```
rule_color = property.PropertyID() == CSSPropertyID::kColumnRuleColor ?
style.ColumnRuleColor() : style.RowRuleColor();
for (const auto& gap_data : rule_color.GetGapDataList()) {
result.push_back(gap_data.GetValue());
}```
Done
nit (if you want): Instead of breaking switch cases into `CSSLength*` -> `CSSGap*` -> `CSSLength*)`, we can just pull out the Rule width properties and have them at the npttp, similar to aspect ratio, grid ...
yes I was gonna change this but forgot to, thanks for reminder.
Javier ContrerasI wonder if we'd want to keep this duplicated in both places rather than moving to gap decor folder ... Not sure though
I hadn't realized, but these aren't WPT because they make use of a resources file that is only on chromium, so I added my tests to this same folder where the existing tests were before. Otherwise they don't run correctly
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
{at: 0, expect: '0px'},
Sam Davis Omekarathis feels like this test case was wrong to begin with. If it is inheriting from the parent, why would it start out at 0px rather than at 30px, like the parent does?
I ran into this with a patch I had but I don't think we should run into this for this change. Let's sync offline about this...
But essentially it should start from 0px because although it inherited from the parent, the computed value should be 0px because style is none. That logic shouldn't change in this change.
I did some searching and it does seem like the test was previously wrong, and the value at time `0` should be the parent's value, here are some examples, but there are many more:
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Code-Review | +1 |
{at: 0, expect: '0px'},
Sam Davis Omekarathis feels like this test case was wrong to begin with. If it is inheriting from the parent, why would it start out at 0px rather than at 30px, like the parent does?
Javier ContrerasI ran into this with a patch I had but I don't think we should run into this for this change. Let's sync offline about this...
But essentially it should start from 0px because although it inherited from the parent, the computed value should be 0px because style is none. That logic shouldn't change in this change.
I did some searching and it does seem like the test was previously wrong, and the value at time `0` should be the parent's value, here are some examples, but there are many more:
As discussed offline, the examples you've listed don't have the zero if style is hidden behavior. But we can debug to figure out why we're not respecting that restriction in this change.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
CHECK(property_id_ == CSSPropertyID::kColumnRuleColor ||
property_id_ == CSSPropertyID::kColumnRuleWidth ||
property_id_ == CSSPropertyID::kRowRuleColor ||
property_id_ == CSSPropertyID::kRowRuleWidth);
What about `*-rule-style`? Those should be animatable as discrete values, similar to `border-*-style` or `outline-style`.
// TODO(crbug.com/357648037): Turn this on when implementing interpolation for Gap Decorations.
Since we're flipping to `true` should we delete this TODO?
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
CHECK(property_id_ == CSSPropertyID::kColumnRuleColor ||
property_id_ == CSSPropertyID::kColumnRuleWidth ||
property_id_ == CSSPropertyID::kRowRuleColor ||
property_id_ == CSSPropertyID::kRowRuleWidth);
What about `*-rule-style`? Those should be animatable as discrete values, similar to `border-*-style` or `outline-style`.
Not 100% sure but the style properties in chromium don't seem to have the interpolable field.
In mdn[1], it states these should have discrete animation values, but unsure if they are implemented in blink.
[1]: https://developer.mozilla.org/en-US/docs/Web/CSS/border-style
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
CHECK(property_id_ == CSSPropertyID::kColumnRuleColor ||
property_id_ == CSSPropertyID::kColumnRuleWidth ||
property_id_ == CSSPropertyID::kRowRuleColor ||
property_id_ == CSSPropertyID::kRowRuleWidth);
Sam Davis OmekaraWhat about `*-rule-style`? Those should be animatable as discrete values, similar to `border-*-style` or `outline-style`.
Not 100% sure but the style properties in chromium don't seem to have the interpolable field.
In mdn[1], it states these should have discrete animation values, but unsure if they are implemented in blink.
[1]: https://developer.mozilla.org/en-US/docs/Web/CSS/border-style
Moreover, none of those properties have a corresponding interpolation type [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/animation/interpolation_types_map.cc;drc=a5302fd8428c0287f753b2503b2f06d4550ab307;l=91)
// TODO(crbug.com/357648037): Turn this on when implementing interpolation for Gap Decorations.
Since we're flipping to `true` should we delete this TODO?
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Code-Review | +1 |
CHECK(property_id_ == CSSPropertyID::kColumnRuleColor ||
property_id_ == CSSPropertyID::kColumnRuleWidth ||
property_id_ == CSSPropertyID::kRowRuleColor ||
property_id_ == CSSPropertyID::kRowRuleWidth);
Sam Davis OmekaraWhat about `*-rule-style`? Those should be animatable as discrete values, similar to `border-*-style` or `outline-style`.
Javier ContrerasNot 100% sure but the style properties in chromium don't seem to have the interpolable field.
In mdn[1], it states these should have discrete animation values, but unsure if they are implemented in blink.
[1]: https://developer.mozilla.org/en-US/docs/Web/CSS/border-style
Moreover, none of those properties have a corresponding interpolation type [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/animation/interpolation_types_map.cc;drc=a5302fd8428c0287f753b2503b2f06d4550ab307;l=91)
Oh that's interesting, maybe properties that animate as discrete values don't need interpolation logic.
Can we add WPT coverage validating that `*-rule-style` animates with the correct behavior? I'm okay with doing that in a separate CL, as long as we aren't running into this `CHECK()` when an author specifies an animation on `*-rule-style`.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
CHECK(property_id_ == CSSPropertyID::kColumnRuleColor ||
property_id_ == CSSPropertyID::kColumnRuleWidth ||
property_id_ == CSSPropertyID::kRowRuleColor ||
property_id_ == CSSPropertyID::kRowRuleWidth);
Sam Davis OmekaraWhat about `*-rule-style`? Those should be animatable as discrete values, similar to `border-*-style` or `outline-style`.
Javier ContrerasNot 100% sure but the style properties in chromium don't seem to have the interpolable field.
In mdn[1], it states these should have discrete animation values, but unsure if they are implemented in blink.
[1]: https://developer.mozilla.org/en-US/docs/Web/CSS/border-style
Kevin BabbittMoreover, none of those properties have a corresponding interpolation type [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/animation/interpolation_types_map.cc;drc=a5302fd8428c0287f753b2503b2f06d4550ab307;l=91)
Oh that's interesting, maybe properties that animate as discrete values don't need interpolation logic.
Can we add WPT coverage validating that `*-rule-style` animates with the correct behavior? I'm okay with doing that in a separate CL, as long as we aren't running into this `CHECK()` when an author specifies an animation on `*-rule-style`.
Will do. And yeah, this path will only be hit by the properties specified right now, since thats how I've set it up in the file I linked earlier
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Overall, this looks good. Added a few suggestions. +flackr@ for review, as well, who is an owner in the animation directory
`GapDataList` via templates, one for <int> one for <StyleColor>. We then
nit: "and"
initial_list[index], CssProperty(), 1,
nit: This should have a comment to note the var name
// GridTemplatePropertyInterpolationType::MaybeMergeSingles
nit: missing punctuation
// Once we can handle repeaters maybe would have to have a condition here to
I'd make this a TODO instead
} else if (CssProperty().PropertyID() == CSSPropertyID::kRowRuleWidth) {
Should this be just an else with a CHECK that it equals row-rule-width instead, or do we expect any others will need to use this method?
const auto& vals = list.GetGapDataList();
nit: I'd call this values instead of vals
int>::MaybeConvertStandardPropertyUnderlyingValue(const ComputedStyle&
style) const {
This feels like it should be re-wrapped in some way
CHECK(initial_list.HasSingleValue());
Is this always going to be true? (i.e. if it is a repeater, would that just return the first one essentially?
case CSSPropertyID::kColumnRuleWidth:
case CSSPropertyID::kRowRuleWidth:
Out of curiosity, why do we need this? And do we only need it when the feature flag is enabled?
display: flex;
Would it make sense to add tests for grid and multicol, too, just to make sure these are working as expected everywhere?
from: 'initial',
I'd suggest adding some cases for the keywords we accept, as well (ex. thin)
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
`GapDataList` via templates, one for <int> one for <StyleColor>. We then
Javier Contrerasnit: "and"
Done
nit: This should have a comment to note the var name
Done
// GridTemplatePropertyInterpolationType::MaybeMergeSingles
Javier Contrerasnit: missing punctuation
Done
// Once we can handle repeaters maybe would have to have a condition here to
I'd make this a TODO instead
Done
} else if (CssProperty().PropertyID() == CSSPropertyID::kRowRuleWidth) {
Should this be just an else with a CHECK that it equals row-rule-width instead, or do we expect any others will need to use this method?
Done
nit: I'd call this values instead of vals
Done
int>::MaybeConvertStandardPropertyUnderlyingValue(const ComputedStyle&
style) const {
This feels like it should be re-wrapped in some way
I've tried, but the formatter keeps wrapping it like this.
Is this always going to be true? (i.e. if it is a repeater, would that just return the first one essentially?
I believe this will always be true because the `initial` value for both ColumnRuleWidth and RowRuleWidth will never be a repeater, unless I am missing something.
case CSSPropertyID::kColumnRuleWidth:
case CSSPropertyID::kRowRuleWidth:
Out of curiosity, why do we need this? And do we only need it when the feature flag is enabled?
We need it for when we call `CreateLength` to convert from a `InterpolableLength` to a `Length` in `ApplyStandardPropertyValue`. The flag state doesn't really matter since its not something that is called for those properties outside of this change.
display: flex;
Would it make sense to add tests for grid and multicol, too, just to make sure these are working as expected everywhere?
It seems to me like that would be a bit unnecessary, since the animation pipeline is completely separate from any sort of container logic, so its completely agnostic to the container type. It seems unnecessary to me to add layout-related tests on a style/animation change. But if you feel strongly about it I can add a grid test.
There's already [existing](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/animations/interpolation/webkit-column-rule-width-interpolation.html;l=6?q=webkit-column-rule-width&sq=&ss=chromium%2Fchromium%2Fsrc) tests for this in multicol.
I'd suggest adding some cases for the keywords we accept, as well (ex. thin)
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
initial_list[index], CssProperty(), /* zoom */ 1,
`/*zoom=*/1`
// TODO: Once we can handle repeaters maybe would have to have a condition
I'd make this a TODO(javiercon) or associate it with the bug
CHECK(CssProperty().PropertyID() == CSSPropertyID::kRowRuleWidth);
nit: CHECK_EQ
CHECK(initial_list.HasSingleValue());
Javier ContrerasIs this always going to be true? (i.e. if it is a repeater, would that just return the first one essentially?
I believe this will always be true because the `initial` value for both ColumnRuleWidth and RowRuleWidth will never be a repeater, unless I am missing something.
Ok interesting, I guess what was confusing to me was that we are calling GetProperty() to get the initial value (which I thought maybe meant it was the initial style for the animation as opposed to what you would get with the initial keyword).
case CSSPropertyID::kColumnRuleWidth:
case CSSPropertyID::kRowRuleWidth:
Javier ContrerasOut of curiosity, why do we need this? And do we only need it when the feature flag is enabled?
We need it for when we call `CreateLength` to convert from a `InterpolableLength` to a `Length` in `ApplyStandardPropertyValue`. The flag state doesn't really matter since its not something that is called for those properties outside of this change.
The flag state doesn't really matter since its not something that is called for those properties outside of this change.
What do you mean by this? Are you saying we don't ever get here if the flag is disabled?
display: flex;
Javier ContrerasWould it make sense to add tests for grid and multicol, too, just to make sure these are working as expected everywhere?
It seems to me like that would be a bit unnecessary, since the animation pipeline is completely separate from any sort of container logic, so its completely agnostic to the container type. It seems unnecessary to me to add layout-related tests on a style/animation change. But if you feel strongly about it I can add a grid test.
There's already [existing](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/animations/interpolation/webkit-column-rule-width-interpolation.html;l=6?q=webkit-column-rule-width&sq=&ss=chromium%2Fchromium%2Fsrc) tests for this in multicol.
I think it makes sense to add some for grid, then. Even though it shouldn't matter, it is generally safer to have tests for those sorts of combinations in case something breaks in the future in one and not the other for some reason (and hopefully should be fairly straightforward to add)
initial_list[index], CssProperty(), /* zoom */ 1,
Javier Contreras`/*zoom=*/1`
Done
// TODO: Once we can handle repeaters maybe would have to have a condition
I'd make this a TODO(javiercon) or associate it with the bug
Done
CHECK(CssProperty().PropertyID() == CSSPropertyID::kRowRuleWidth);
Javier Contrerasnit: CHECK_EQ
Done
case CSSPropertyID::kColumnRuleWidth:
case CSSPropertyID::kRowRuleWidth:
Javier ContrerasOut of curiosity, why do we need this? And do we only need it when the feature flag is enabled?
Alison MaherWe need it for when we call `CreateLength` to convert from a `InterpolableLength` to a `Length` in `ApplyStandardPropertyValue`. The flag state doesn't really matter since its not something that is called for those properties outside of this change.
The flag state doesn't really matter since its not something that is called for those properties outside of this change.
What do you mean by this? Are you saying we don't ever get here if the flag is disabled?
Yes correct. With the flag disabled we don't go through this path for column/row-rule-width interpolation.
display: flex;
Javier ContrerasWould it make sense to add tests for grid and multicol, too, just to make sure these are working as expected everywhere?
Alison MaherIt seems to me like that would be a bit unnecessary, since the animation pipeline is completely separate from any sort of container logic, so its completely agnostic to the container type. It seems unnecessary to me to add layout-related tests on a style/animation change. But if you feel strongly about it I can add a grid test.
There's already [existing](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/animations/interpolation/webkit-column-rule-width-interpolation.html;l=6?q=webkit-column-rule-width&sq=&ss=chromium%2Fchromium%2Fsrc) tests for this in multicol.
I think it makes sense to add some for grid, then. Even though it shouldn't matter, it is generally safer to have tests for those sorts of combinations in case something breaks in the future in one and not the other for some reason (and hopefully should be fairly straightforward to add)
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Code-Review | +1 |
re stamping
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
template <typename T>
Is this ever constructed with different types? I couldn't find any cases where int wasn't used in the code. If not, it seems like for clarity it would be best to remove the templated type. Also, it seems like if we don't need templated types then we can avoid having the entire implementation in the header which is bad for compile times.
MakeGarbageCollected<CSSGapDataListInterpolationType<int>>(
Do we need a custom type here? Can we not directly use CSSLengthListInterpolationType? It seems like that would significantly reduce the support code needed. Or alternately if specialized code is needed a subclass of CSSLengthListInterpolationType like CSSTransformOriginInterpolationType
gap_data_list_.emplace_back(GapData<int>(length.IntValue()));
Doesn't this lose the type from the lengths? Do you have tests which have mixed units in a single rule and/or different units between rules?
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
template <typename T>
Is this ever constructed with different types? I couldn't find any cases where int wasn't used in the code. If not, it seems like for clarity it would be best to remove the templated type. Also, it seems like if we don't need templated types then we can avoid having the entire implementation in the header which is bad for compile times.
Not in this CL, but it will be.
This class will be used for interpolating `ColumnRuleWidth` and `ColumnRuleColor` (as well as the respective `Row` properties). This CL only handles `width` (template `int`) and a follow up CL will handle `color` (template `StyleColor`).
This CL only handles Width for clarity and to avoid having a massive CL.
MakeGarbageCollected<CSSGapDataListInterpolationType<int>>(
Do we need a custom type here? Can we not directly use CSSLengthListInterpolationType? It seems like that would significantly reduce the support code needed. Or alternately if specialized code is needed a subclass of CSSLengthListInterpolationType like CSSTransformOriginInterpolationType
I believe that a custom type is needed, and the reason for this is that `ColumnRuleWidth` and `RowRuleWidth` have the following [possible syntax](https://drafts.csswg.org/css-gaps-1/#column-row-rule-width), which is as you say, a LengthList (e.g `1px 5px 10px`, but also could be `repeat(5, 10px 10px)` or `10px repeat(3, 5px 6px) 8px`.
Which is why I think we need the custom type. The [follow up CL](https://chromium-review.googlesource.com/c/chromium/src/+/6655208) implements the repeater logic. It was left off this CL to keep this CL contained and not massive.
gap_data_list_.emplace_back(GapData<int>(length.IntValue()));
Doesn't this lose the type from the lengths? Do you have tests which have mixed units in a single rule and/or different units between rules?
I'm not sure if I understand your question. This constructor will only be used to create a GapDataList out of a list of Lengths (we only care about the int value of said lengths).
Code-Review | +1 |
nit: same here, -002.html
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
void GetList(const CSSProperty& property,
This is too generic of a name to be in a header file in the blink namespace when it is only specialized for a couple types, especially when it will usually result in generating code with a runtime error. Please try to move this to a corresponding implementation (.cc) file or scope the function in a way that it is not in the blink namespace (e.g. could be a static function of the class).
// Should not be called, only the specialized types should be used.
This pattern seems problematic to me - shifting something that should be a compile failure to be a runtime failure. Can we not have pure virtual functions in the generic class so that it is a compile failure to use any type other than those which have specializations implementing them?
I.e. this class would not provide an implementation of MaybeConvertNeutral, instead you would derive subclasses which use particular types, e.g.
```
class CORE_EXPORT CSSGapLengthListInterpolationType : public CSSGapDataListInterpolationType<int> {
/* Implementations of MaybeConvertNeutral and other virtual methods */
}
```
Alternately, you could exclude the function body and it would be a linker error to use a type which did not have a specialization in the .cc file, however I feel like this would be less obvious.
template <typename T>
Javier ContrerasIs this ever constructed with different types? I couldn't find any cases where int wasn't used in the code. If not, it seems like for clarity it would be best to remove the templated type. Also, it seems like if we don't need templated types then we can avoid having the entire implementation in the header which is bad for compile times.
Not in this CL, but it will be.
This class will be used for interpolating `ColumnRuleWidth` and `ColumnRuleColor` (as well as the respective `Row` properties). This CL only handles `width` (template `int`) and a follow up CL will handle `color` (template `StyleColor`).
This CL only handles Width for clarity and to avoid having a massive CL.
Understood - though does it need to be templated? Given both lengths and colors will be interpolated as InterpolationValue maybe we could use InterpolationValue directly.
MakeGarbageCollected<CSSGapDataListInterpolationType<int>>(
Javier ContrerasDo we need a custom type here? Can we not directly use CSSLengthListInterpolationType? It seems like that would significantly reduce the support code needed. Or alternately if specialized code is needed a subclass of CSSLengthListInterpolationType like CSSTransformOriginInterpolationType
I believe that a custom type is needed, and the reason for this is that `ColumnRuleWidth` and `RowRuleWidth` have the following [possible syntax](https://drafts.csswg.org/css-gaps-1/#column-row-rule-width), which is as you say, a LengthList (e.g `1px 5px 10px`, but also could be `repeat(5, 10px 10px)` or `10px repeat(3, 5px 6px) 8px`.
Which is why I think we need the custom type. The [follow up CL](https://chromium-review.googlesource.com/c/chromium/src/+/6655208) implements the repeater logic. It was left off this CL to keep this CL contained and not massive.
Got it, so this has a similar repeating syntax to track lists. Ideally we'd share the code as much as possible (e.g. have a common base class for both grid track lists and column lists) but perhaps the syntaxes are sufficiently different that this is not possible?
gap_data_list_.emplace_back(GapData<int>(length.IntValue()));
Javier ContrerasDoesn't this lose the type from the lengths? Do you have tests which have mixed units in a single rule and/or different units between rules?
I'm not sure if I understand your question. This constructor will only be used to create a GapDataList out of a list of Lengths (we only care about the int value of said lengths).
Length has a type_ field, the int value is in the units of the type field. It would be invalid to interpolate between lengths of different types using the int value as is because 1% is not the same length as 1px, but IntValue will return 1 for both. Perhaps in this situation all of the types are already in pixels in which case it would be a good idea to call length.Pixels() instead so that we have the DCHECK that we are in fact dealing with pixels.
void GetList(const CSSProperty& property,
This is too generic of a name to be in a header file in the blink namespace when it is only specialized for a couple types, especially when it will usually result in generating code with a runtime error. Please try to move this to a corresponding implementation (.cc) file or scope the function in a way that it is not in the blink namespace (e.g. could be a static function of the class).
Fixed in other approach from previous comment, will port over if looks good.
// Should not be called, only the specialized types should be used.
This pattern seems problematic to me - shifting something that should be a compile failure to be a runtime failure. Can we not have pure virtual functions in the generic class so that it is a compile failure to use any type other than those which have specializations implementing them?
I.e. this class would not provide an implementation of MaybeConvertNeutral, instead you would derive subclasses which use particular types, e.g.
```
class CORE_EXPORT CSSGapLengthListInterpolationType : public CSSGapDataListInterpolationType<int> {
/* Implementations of MaybeConvertNeutral and other virtual methods */
}
```Alternately, you could exclude the function body and it would be a linker error to use a type which did not have a specialization in the .cc file, however I feel like this would be less obvious.
I have implemented the suggestion of having a pure virtual "base" and subclasses that override it in the follow up [CL](https://chromium-review.googlesource.com/c/chromium/src/+/6684748). If the changes there look good to you, I can port them over here. I didnt do it in this one since some people have already signed off on current implementation.
template <typename T>
Javier ContrerasIs this ever constructed with different types? I couldn't find any cases where int wasn't used in the code. If not, it seems like for clarity it would be best to remove the templated type. Also, it seems like if we don't need templated types then we can avoid having the entire implementation in the header which is bad for compile times.
Robert FlackNot in this CL, but it will be.
This class will be used for interpolating `ColumnRuleWidth` and `ColumnRuleColor` (as well as the respective `Row` properties). This CL only handles `width` (template `int`) and a follow up CL will handle `color` (template `StyleColor`).
This CL only handles Width for clarity and to avoid having a massive CL.
Understood - though does it need to be templated? Given both lengths and colors will be interpolated as InterpolationValue maybe we could use InterpolationValue directly.
I think this is an interesting suggestion that could potentially work. We'd still need to find a way to get those `InterpolationValues` to be the right type for when we feed them back to the style, and I fear there might be other gotchas trying to implement it this way. Would it be ok with you if we leave this as a TODO for potential exploration later?
MakeGarbageCollected<CSSGapDataListInterpolationType<int>>(
Javier ContrerasDo we need a custom type here? Can we not directly use CSSLengthListInterpolationType? It seems like that would significantly reduce the support code needed. Or alternately if specialized code is needed a subclass of CSSLengthListInterpolationType like CSSTransformOriginInterpolationType
Robert FlackI believe that a custom type is needed, and the reason for this is that `ColumnRuleWidth` and `RowRuleWidth` have the following [possible syntax](https://drafts.csswg.org/css-gaps-1/#column-row-rule-width), which is as you say, a LengthList (e.g `1px 5px 10px`, but also could be `repeat(5, 10px 10px)` or `10px repeat(3, 5px 6px) 8px`.
Which is why I think we need the custom type. The [follow up CL](https://chromium-review.googlesource.com/c/chromium/src/+/6655208) implements the repeater logic. It was left off this CL to keep this CL contained and not massive.
Got it, so this has a similar repeating syntax to track lists. Ideally we'd share the code as much as possible (e.g. have a common base class for both grid track lists and column lists) but perhaps the syntaxes are sufficiently different that this is not possible?
If you are referring to the grid tracklist syntax then yes, they are pretty different since the syntax for that one includes may other different units such as `fr`, `ch`, `auto`.
template <typename T>
Javier ContrerasIs this ever constructed with different types? I couldn't find any cases where int wasn't used in the code. If not, it seems like for clarity it would be best to remove the templated type. Also, it seems like if we don't need templated types then we can avoid having the entire implementation in the header which is bad for compile times.
Robert FlackNot in this CL, but it will be.
This class will be used for interpolating `ColumnRuleWidth` and `ColumnRuleColor` (as well as the respective `Row` properties). This CL only handles `width` (template `int`) and a follow up CL will handle `color` (template `StyleColor`).
This CL only handles Width for clarity and to avoid having a massive CL.
Javier ContrerasUnderstood - though does it need to be templated? Given both lengths and colors will be interpolated as InterpolationValue maybe we could use InterpolationValue directly.
I think this is an interesting suggestion that could potentially work. We'd still need to find a way to get those `InterpolationValues` to be the right type for when we feed them back to the style, and I fear there might be other gotchas trying to implement it this way. Would it be ok with you if we leave this as a TODO for potential exploration later?
Alternately, if the only shared code ends up being the repeat extension handling, it may be cleaner to keep the classes concrete and call that as a templated helper function from both of them.
// Should not be called, only the specialized types should be used.
Javier ContrerasThis pattern seems problematic to me - shifting something that should be a compile failure to be a runtime failure. Can we not have pure virtual functions in the generic class so that it is a compile failure to use any type other than those which have specializations implementing them?
I.e. this class would not provide an implementation of MaybeConvertNeutral, instead you would derive subclasses which use particular types, e.g.
```
class CORE_EXPORT CSSGapLengthListInterpolationType : public CSSGapDataListInterpolationType<int> {
/* Implementations of MaybeConvertNeutral and other virtual methods */
}
```Alternately, you could exclude the function body and it would be a linker error to use a type which did not have a specialization in the .cc file, however I feel like this would be less obvious.
Javier ContrerasI have implemented the suggestion of having a pure virtual "base" and subclasses that override it in the follow up [CL](https://chromium-review.googlesource.com/c/chromium/src/+/6684748). If the changes there look good to you, I can port them over here. I didnt do it in this one since some people have already signed off on current implementation.
I have implemented the new appcoach. Let me know of any other suggestions
MakeGarbageCollected<CSSGapDataListInterpolationType<int>>(
Javier ContrerasDo we need a custom type here? Can we not directly use CSSLengthListInterpolationType? It seems like that would significantly reduce the support code needed. Or alternately if specialized code is needed a subclass of CSSLengthListInterpolationType like CSSTransformOriginInterpolationType
Robert FlackI believe that a custom type is needed, and the reason for this is that `ColumnRuleWidth` and `RowRuleWidth` have the following [possible syntax](https://drafts.csswg.org/css-gaps-1/#column-row-rule-width), which is as you say, a LengthList (e.g `1px 5px 10px`, but also could be `repeat(5, 10px 10px)` or `10px repeat(3, 5px 6px) 8px`.
Which is why I think we need the custom type. The [follow up CL](https://chromium-review.googlesource.com/c/chromium/src/+/6655208) implements the repeater logic. It was left off this CL to keep this CL contained and not massive.
Javier ContrerasGot it, so this has a similar repeating syntax to track lists. Ideally we'd share the code as much as possible (e.g. have a common base class for both grid track lists and column lists) but perhaps the syntaxes are sufficiently different that this is not possible?
If you are referring to the grid tracklist syntax then yes, they are pretty different since the syntax for that one includes may other different units such as `fr`, `ch`, `auto`.
Done
gap_data_list_.emplace_back(GapData<int>(length.IntValue()));
Javier ContrerasDoesn't this lose the type from the lengths? Do you have tests which have mixed units in a single rule and/or different units between rules?
Robert FlackI'm not sure if I understand your question. This constructor will only be used to create a GapDataList out of a list of Lengths (we only care about the int value of said lengths).
Length has a type_ field, the int value is in the units of the type field. It would be invalid to interpolate between lengths of different types using the int value as is because 1% is not the same length as 1px, but IntValue will return 1 for both. Perhaps in this situation all of the types are already in pixels in which case it would be a good idea to call length.Pixels() instead so that we have the DCHECK that we are in fact dealing with pixels.
Done
Javier Contrerasnit: same here, -002.html
Done
nit: I'd rename this to end in -001.html
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
inline void CSSGapDataListInterpolationType<int>::GetList(
This can be an override in CSSGapWidthListInterpolationType.
NOTREACHED();
This can also be pure virtual with no implementation. Any implementations of this abstract class should provide an override of this method.
class CORE_EXPORT CSSGapWidthListInterpolationType
Since this isn't specific to width, it might make more sense to call this CSSGapLengthListInterpolationType.
inline void CSSGapDataListInterpolationType<int>::GetList(
This can be an override in CSSGapWidthListInterpolationType.
Yes but we use it in the `InheritedGapDataListChecker` so we need it to be static and thus can't also be virtual
NOTREACHED();
This can also be pure virtual with no implementation. Any implementations of this abstract class should provide an override of this method.
Done
class CORE_EXPORT CSSGapWidthListInterpolationType
Since this isn't specific to width, it might make more sense to call this CSSGapLengthListInterpolationType.
I see why that makes sense, however, I opted for width because the property we are interpolating is column/row-rule-width, even though under the hood we are using `Length` to represent this.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
template <typename T>
Javier ContrerasIs this ever constructed with different types? I couldn't find any cases where int wasn't used in the code. If not, it seems like for clarity it would be best to remove the templated type. Also, it seems like if we don't need templated types then we can avoid having the entire implementation in the header which is bad for compile times.
Robert FlackNot in this CL, but it will be.
This class will be used for interpolating `ColumnRuleWidth` and `ColumnRuleColor` (as well as the respective `Row` properties). This CL only handles `width` (template `int`) and a follow up CL will handle `color` (template `StyleColor`).
This CL only handles Width for clarity and to avoid having a massive CL.
Javier ContrerasUnderstood - though does it need to be templated? Given both lengths and colors will be interpolated as InterpolationValue maybe we could use InterpolationValue directly.
Robert FlackI think this is an interesting suggestion that could potentially work. We'd still need to find a way to get those `InterpolationValues` to be the right type for when we feed them back to the style, and I fear there might be other gotchas trying to implement it this way. Would it be ok with you if we leave this as a TODO for potential exploration later?
Alternately, if the only shared code ends up being the repeat extension handling, it may be cleaner to keep the classes concrete and call that as a templated helper function from both of them.
Yes I think I see what you mean. Could we still leave this as a potential exploration once we have the repeater stuff done so we can see how we could turn it into what you suggest?
class CORE_EXPORT CSSGapWidthListInterpolationType
Javier ContrerasSince this isn't specific to width, it might make more sense to call this CSSGapLengthListInterpolationType.
I see why that makes sense, however, I opted for width because the property we are interpolating is column/row-rule-width, even though under the hood we are using `Length` to represent this.
FWIW, I can envision future applications of this to other gap decoration length lists. For example, I've seen/heard suggestions for more control over outset/offsets, which might necessitate lists similar to what we have for width.
I also think CSSGapLengthListInterpolationType would be more consistent with use of "Length" in names of other objects related to interpolation.
state.StyleBuilder().SetRowRuleWidth(GapDataList<int>(result));
```suggestion
CHECK_EQ(property_id_, CSSPropertyID::kRowRuleWidth);
state.StyleBuilder().SetRowRuleWidth(GapDataList<int>(result));
```
inline void CSSGapDataListInterpolationType<int>::GetList(
Javier ContrerasThis can be an override in CSSGapWidthListInterpolationType.
Yes but we use it in the `InheritedGapDataListChecker` so we need it to be static and thus can't also be virtual
Ah I see. Given this class is only instantiated / directly referenced from third_party/blink/renderer/core/animation/css_gap_width_list_interpolation_type.cc perhaps we can move the implementation into that cc file as seems to be the case with many of the other CSSConversionChecker implementations: https://source.chromium.org/search?q=file:%5C.cc%20%22:%20public%20CSSInterpolationType::CSSConversionChecker%22&start=1
We should aim to only keep things in the header files that people including this need, but this checker can entirely be an implementation detail.
class CORE_EXPORT CSSGapWidthListInterpolationType
Javier ContrerasSince this isn't specific to width, it might make more sense to call this CSSGapLengthListInterpolationType.
Kevin BabbittI see why that makes sense, however, I opted for width because the property we are interpolating is column/row-rule-width, even though under the hood we are using `Length` to represent this.
FWIW, I can envision future applications of this to other gap decoration length lists. For example, I've seen/heard suggestions for more control over outset/offsets, which might necessitate lists similar to what we have for width.
I also think CSSGapLengthListInterpolationType would be more consistent with use of "Length" in names of other objects related to interpolation.
+1 I think it's better for consistency to use the data type rather than the property name.
assertInterpolation({
Can we instead add as WPT tests. As much as possible, I'd love to see tests that don't depend on blink implementation details (e.g. compositing decisions) migrated to WPT.
See third_party/blink/web_tests/external/wpt/css/support/interpolation-testcommon.js
third_party/blink/web_tests/external/wpt/css/motion/animation/offset-position-interpolation.html
Added 2 more WPT tests and responded to feedback. PTAL
inline void CSSGapDataListInterpolationType<int>::GetList(
Javier ContrerasThis can be an override in CSSGapWidthListInterpolationType.
Robert FlackYes but we use it in the `InheritedGapDataListChecker` so we need it to be static and thus can't also be virtual
Ah I see. Given this class is only instantiated / directly referenced from third_party/blink/renderer/core/animation/css_gap_width_list_interpolation_type.cc perhaps we can move the implementation into that cc file as seems to be the case with many of the other CSSConversionChecker implementations: https://source.chromium.org/search?q=file:%5C.cc%20%22:%20public%20CSSInterpolationType::CSSConversionChecker%22&start=1
We should aim to only keep things in the header files that people including this need, but this checker can entirely be an implementation detail.
Done
template <typename T>
Javier ContrerasIs this ever constructed with different types? I couldn't find any cases where int wasn't used in the code. If not, it seems like for clarity it would be best to remove the templated type. Also, it seems like if we don't need templated types then we can avoid having the entire implementation in the header which is bad for compile times.
Robert FlackNot in this CL, but it will be.
This class will be used for interpolating `ColumnRuleWidth` and `ColumnRuleColor` (as well as the respective `Row` properties). This CL only handles `width` (template `int`) and a follow up CL will handle `color` (template `StyleColor`).
This CL only handles Width for clarity and to avoid having a massive CL.
Javier ContrerasUnderstood - though does it need to be templated? Given both lengths and colors will be interpolated as InterpolationValue maybe we could use InterpolationValue directly.
Robert FlackI think this is an interesting suggestion that could potentially work. We'd still need to find a way to get those `InterpolationValues` to be the right type for when we feed them back to the style, and I fear there might be other gotchas trying to implement it this way. Would it be ok with you if we leave this as a TODO for potential exploration later?
Javier ContrerasAlternately, if the only shared code ends up being the repeat extension handling, it may be cleaner to keep the classes concrete and call that as a templated helper function from both of them.
Yes I think I see what you mean. Could we still leave this as a potential exploration once we have the repeater stuff done so we can see how we could turn it into what you suggest?
Resolving for now.
class CORE_EXPORT CSSGapWidthListInterpolationType
Javier ContrerasSince this isn't specific to width, it might make more sense to call this CSSGapLengthListInterpolationType.
Kevin BabbittI see why that makes sense, however, I opted for width because the property we are interpolating is column/row-rule-width, even though under the hood we are using `Length` to represent this.
Robert FlackFWIW, I can envision future applications of this to other gap decoration length lists. For example, I've seen/heard suggestions for more control over outset/offsets, which might necessitate lists similar to what we have for width.
I also think CSSGapLengthListInterpolationType would be more consistent with use of "Length" in names of other objects related to interpolation.
+1 I think it's better for consistency to use the data type rather than the property name.
Done
state.StyleBuilder().SetRowRuleWidth(GapDataList<int>(result));
```suggestion
CHECK_EQ(property_id_, CSSPropertyID::kRowRuleWidth);
state.StyleBuilder().SetRowRuleWidth(GapDataList<int>(result));
```
Done
Can we instead add as WPT tests. As much as possible, I'd love to see tests that don't depend on blink implementation details (e.g. compositing decisions) migrated to WPT.
See third_party/blink/web_tests/external/wpt/css/support/interpolation-testcommon.js
third_party/blink/web_tests/external/wpt/css/motion/animation/offset-position-interpolation.html
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Just a few more comments, I feel like this is very close. Thanks for working through this.
virtual GapDataList<T> GetProperty(const ComputedStyle& style) const = 0;
I'm actually not sure I understand the value of having this function in the base class if it's only used by the derived class.
It's also the only thing using the template type. Perhaps we could re-add the template type when it's actually needed - e.g. if you later end up needing some shared type-dependent functionality. I'm not actually convinced that we will though - both subclasses could likely call a shared templated helper function to perform the list extension. Note that we could define both subclasses in css_grap_data_list_interpolation_type.cc if it helps them be able to share some templated implementation helper functions.
property_id_ == CSSPropertyID::kRowRuleWidth);
Perhaps move this to the derived class and only check for the two relevant properties. We can add the color rules when that class is added later to its constructor.
I suppose the counter-argument would be if we implement repeating semantics in this class - that we assert that the repeating semantics actually make sense for the property. However, given we don't have that yet I'm not sure if it matters.
class InheritedGapLengthListChecker final
I don't think any external classes need access to this checker as its functions are only called via the virtual CSSConversionChecker interface. You can move the implementation entirely into the .cc file similar to the examples I linked earlier in https://chromium-review.googlesource.com/c/chromium/src/+/6643003/comment/6a3dbde7_6f4d53be/
CHECK_EQ(property_id_, CSSPropertyID::kRowRuleWidth);
It seems like it would be better to check at construction that property_id_ is either kColumnRuleWidth or kRowRuleWidth, as there are several places throughout this class where the code assumes that it is one or the other.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
virtual GapDataList<T> GetProperty(const ComputedStyle& style) const = 0;
I'm actually not sure I understand the value of having this function in the base class if it's only used by the derived class.
It's also the only thing using the template type. Perhaps we could re-add the template type when it's actually needed - e.g. if you later end up needing some shared type-dependent functionality. I'm not actually convinced that we will though - both subclasses could likely call a shared templated helper function to perform the list extension. Note that we could define both subclasses in css_grap_data_list_interpolation_type.cc if it helps them be able to share some templated implementation helper functions.
I now very clearly see your vision. I made some changes to my WIP CL with the repeater handling and now we dont need template here, in fact we don't need this class at all.
Perhaps move this to the derived class and only check for the two relevant properties. We can add the color rules when that class is added later to its constructor.
I suppose the counter-argument would be if we implement repeating semantics in this class - that we assert that the repeating semantics actually make sense for the property. However, given we don't have that yet I'm not sure if it matters.
Done
I don't think any external classes need access to this checker as its functions are only called via the virtual CSSConversionChecker interface. You can move the implementation entirely into the .cc file similar to the examples I linked earlier in https://chromium-review.googlesource.com/c/chromium/src/+/6643003/comment/6a3dbde7_6f4d53be/
Done
It seems like it would be better to check at construction that property_id_ is either kColumnRuleWidth or kRowRuleWidth, as there are several places throughout this class where the code assumes that it is one or the other.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Code-Review | +1 |
property_id_ = property.GetCSSProperty().PropertyID();
nit: This should be set in the initializer list, i.e.
```
explicit CSSGapLengthListInterpolationType(
PropertyHandle property,
const PropertyRegistration* registration = nullptr)
: CSSInterpolationType(property, registration),
property_id_(property.GetCSSProperty().PropertyID()) {
```
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
nit: This should be set in the initializer list, i.e.
```
explicit CSSGapLengthListInterpolationType(
PropertyHandle property,
const PropertyRegistration* registration = nullptr)
: CSSInterpolationType(property, registration),
property_id_(property.GetCSSProperty().PropertyID()) {
```
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
38 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/core/animation/css_gap_length_list_interpolation_type.h
Insertions: 2, Deletions: 2.
@@ -19,8 +19,8 @@
explicit CSSGapLengthListInterpolationType(
PropertyHandle property,
const PropertyRegistration* registration = nullptr)
- : CSSInterpolationType(property, registration) {
- property_id_ = property.GetCSSProperty().PropertyID();
+ : CSSInterpolationType(property, registration),
+ property_id_(property.GetCSSProperty().PropertyID()) {
CHECK(property_id_ == CSSPropertyID::kColumnRuleWidth ||
property_id_ == CSSPropertyID::kRowRuleWidth);
}
```
[gap-decorations] Base interpolation for *-rule-width
This CL implements basic interpolation for row-rule-width and
column-rule-width properties for CSSGapDecorations.
This CL only implements the basic interpolation for the width
properties, upcoming CLs will implement the other template specific
functions for StyleColor specializations, and follow up CLs will also
implement the remaining logic for dealing with repeaters.
The overall structure of interpolation for gap decorations will be as
follows:
`CSSGapLengthListInterpolationType` handles interpolation for
`GapDataList` (for *-rule-width). We then hand off the actual
interpolation with an `InterpolableList` of `InterpolableLength`. The
followup CL will account for the `repeat` syntax.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/53580
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |