Add CDP support for @counter-style rules [chromium/src : main]
Alex Rudenko (Gerrit)
Alex Rudenko added 2 comments![]( "Open in Gerrit")
LOG(ERROR) << "skipping source data of type " << source_data_->type;forgotten logs?
LOG(ERROR) << "skipping invalid property " << name;ditto
Related details
- Eric Leese
- Philip Pfaffe
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. |
Eric Leese (Gerrit)
Eric Leese added 2 comments![]( "Open in Gerrit")
LOG(ERROR) << "skipping source data of type " << source_data_->type;Eric Leeseforgotten logs?
Done
LOG(ERROR) << "skipping invalid property " << name;Eric Leeseditto
Done
Related details
- Alex Rudenko
- Philip Pfaffe
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 4 comments![]( "Open in Gerrit")
} // Is this needed?This helper doesn't seem to be used anywhere in this change. If it's intended for future use, it's fine, but otherwise it can be removed along with the "Is this needed?" comment.
#include "third_party/blink/renderer/core/css/css_property_value_set.h" // Was this needed?This include seems necessary because `MutableProperties()` returns a `MutableCSSPropertyValueSet&`, which requires the full definition of the class (it's a subclass of `CSSPropertyValueSet`). You can remove the "Was this needed?" comment.
if (fallback_value.starts_with("--")) {Is it fine to check with `--`? AI says it might not be always used/customized?
style = counter_style_rule->style();AI-generated below but seems plausible? it looks like https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/style_rule_counter_style.h;l=75;drc=08b2c243ef0d95044d60d5de1d28c1281804c25f needs to be incremented on mutations and we might be bypassing it when editing?
>>>
When a `CSSCounterStyleRule`'s properties are modified via the `StyleRuleCSSStyleDeclaration` (which is what `style()` returns), the underlying `StyleRuleCounterStyle`'s `version_` must be incremented.
Currently, `SetDescriptorValue` increments the version, but that method is only called by CSSOM setters (like `rule.system = '...'`). The inspector uses `setStyleTexts`, which calls `setCSSText` on the `CSSStyleDeclaration`. This modifies the property set directly but doesn't know about the `version_` field.
If the version isn't incremented, `CounterStyleMap` might not re-resolve the style, and the page won't update to reflect the inspector's changes. You might need to override `DidModify` or similar in `CSSCounterStyleRule` to ensure the internal rule's version is updated when the declaration changes.
Related details
- Anders Hartvoll Ruud
- Eric Leese
- Philip Pfaffe
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. |
Eric Leese (Gerrit)
Eric Leese added 3 comments![]( "Open in Gerrit")
This helper doesn't seem to be used anywhere in this change. If it's intended for future use, it's fine, but otherwise it can be removed along with the "Is this needed?" comment.
Done
#include "third_party/blink/renderer/core/css/css_property_value_set.h" // Was this needed?This include seems necessary because `MutableProperties()` returns a `MutableCSSPropertyValueSet&`, which requires the full definition of the class (it's a subclass of `CSSPropertyValueSet`). You can remove the "Was this needed?" comment.
Wasn't needed
Is it fine to check with `--`? AI says it might not be always used/customized?
- Alex Rudenko
- Anders Hartvoll Ruud
- Philip Pfaffe
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. |
Philip Pfaffe (Gerrit)
Philip Pfaffe added 3 comments![]( "Open in Gerrit")
} // Is this needed?Left over comment?
CSSStyleDeclaration* style();Does @content have an official CSSOM version? If not, IIRC the convention was to call this `Style()`
AtomicString next_name = counter_style_name;std::move?
Related details
- Alex Rudenko
- Anders Hartvoll Ruud
- Eric Leese
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. |
Eric Leese (Gerrit)
Eric Leese added 5 comments![]( "Open in Gerrit")
} // Is this needed?Eric LeeseLeft over comment?
Done
} // Is this needed?Eric LeeseLeft over comment?
Done
CSSStyleDeclaration* style();Does @content have an official CSSOM version? If not, IIRC the convention was to call this `Style()`
Done
AtomicString next_name = counter_style_name;Eric Leesestd::move?
Let's get rid of it.
style = counter_style_rule->style();AI-generated below but seems plausible? it looks like https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/style_rule_counter_style.h;l=75;drc=08b2c243ef0d95044d60d5de1d28c1281804c25f needs to be incremented on mutations and we might be bypassing it when editing?
>>>
When a `CSSCounterStyleRule`'s properties are modified via the `StyleRuleCSSStyleDeclaration` (which is what `style()` returns), the underlying `StyleRuleCounterStyle`'s `version_` must be incremented.
Currently, `SetDescriptorValue` increments the version, but that method is only called by CSSOM setters (like `rule.system = '...'`). The inspector uses `setStyleTexts`, which calls `setCSSText` on the `CSSStyleDeclaration`. This modifies the property set directly but doesn't know about the `version_` field.
If the version isn't incremented, `CounterStyleMap` might not re-resolve the style, and the page won't update to reflect the inspector's changes. You might need to override `DidModify` or similar in `CSSCounterStyleRule` to ensure the internal rule's version is updated when the declaration changes.
There does seem to be some sort of dirty tracking going on in `StyleRuleCounterStyle` that none of the other style rule classes have. I don't see any `DidModify` function or similar. Needs further investigation. The author is on leave. @andruud, any suggestions?
Related details
- Alex Rudenko
- Anders Hartvoll Ruud
- Philip Pfaffe
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. |
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud added 2 comments![]( "Open in Gerrit")
bool HasValidSymbols(const CSSValue* system,Did you mean to make this static?
style = counter_style_rule->style();Eric LeeseAI-generated below but seems plausible? it looks like https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/style_rule_counter_style.h;l=75;drc=08b2c243ef0d95044d60d5de1d28c1281804c25f needs to be incremented on mutations and we might be bypassing it when editing?
>>>
When a `CSSCounterStyleRule`'s properties are modified via the `StyleRuleCSSStyleDeclaration` (which is what `style()` returns), the underlying `StyleRuleCounterStyle`'s `version_` must be incremented.
Currently, `SetDescriptorValue` increments the version, but that method is only called by CSSOM setters (like `rule.system = '...'`). The inspector uses `setStyleTexts`, which calls `setCSSText` on the `CSSStyleDeclaration`. This modifies the property set directly but doesn't know about the `version_` field.
If the version isn't incremented, `CounterStyleMap` might not re-resolve the style, and the page won't update to reflect the inspector's changes. You might need to override `DidModify` or similar in `CSSCounterStyleRule` to ensure the internal rule's version is updated when the declaration changes.
There does seem to be some sort of dirty tracking going on in `StyleRuleCounterStyle` that none of the other style rule classes have. I don't see any `DidModify` function or similar. Needs further investigation. The author is on leave. @andruud, any suggestions?
Most at-rules affect how `ComputedStyle`s are produced, and so need to invalidate style when they change. If the `ComputedStyle`s change in a way that affects layout, then layout will be invalidated via the regular mechanisms of diffing the old vs. new `ComputedStyle` objects.
If I remember correctly, counters don't actually affect `ComputedStyle` objects, but _do_ affect layout. So they need a special path to invalidate layout _directly_.
So there is indeed a problem here. I need to think a bit about the solution ...
Related details
- Alex Rudenko
- Eric Leese
- Philip Pfaffe
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. |
Eric Leese (Gerrit)
Eric Leese added 1 comment![]( "Open in Gerrit")
style = counter_style_rule->style();Eric LeeseAI-generated below but seems plausible? it looks like https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/style_rule_counter_style.h;l=75;drc=08b2c243ef0d95044d60d5de1d28c1281804c25f needs to be incremented on mutations and we might be bypassing it when editing?
>>>
When a `CSSCounterStyleRule`'s properties are modified via the `StyleRuleCSSStyleDeclaration` (which is what `style()` returns), the underlying `StyleRuleCounterStyle`'s `version_` must be incremented.
Currently, `SetDescriptorValue` increments the version, but that method is only called by CSSOM setters (like `rule.system = '...'`). The inspector uses `setStyleTexts`, which calls `setCSSText` on the `CSSStyleDeclaration`. This modifies the property set directly but doesn't know about the `version_` field.
If the version isn't incremented, `CounterStyleMap` might not re-resolve the style, and the page won't update to reflect the inspector's changes. You might need to override `DidModify` or similar in `CSSCounterStyleRule` to ensure the internal rule's version is updated when the declaration changes.
Anders Hartvoll RuudThere does seem to be some sort of dirty tracking going on in `StyleRuleCounterStyle` that none of the other style rule classes have. I don't see any `DidModify` function or similar. Needs further investigation. The author is on leave. @andruud, any suggestions?
Most at-rules affect how `ComputedStyle`s are produced, and so need to invalidate style when they change. If the `ComputedStyle`s change in a way that affects layout, then layout will be invalidated via the regular mechanisms of diffing the old vs. new `ComputedStyle` objects.
If I remember correctly, counters don't actually affect `ComputedStyle` objects, but _do_ affect layout. So they need a special path to invalidate layout _directly_.
So there is indeed a problem here. I need to think a bit about the solution ...
So the straightforward if not pretty solution I see is:
1. Add an UpdateVersion() method to CSSCounterStyleRule and StyleRuleCounterStyle to bump the version.
2. Add comments to the header file warning that if you use the style() wrapper to modify the properties you must also call UpdateVersion().
3. Below this, after the call to setCSSText, check again if it is a CSSCounterStyleRule and call UpdateVersion() then.
Would that solution be acceptable here?
Related details
- Alex Rudenko
- Anders Hartvoll Ruud
- Philip Pfaffe
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. |
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
style = counter_style_rule->style();Eric LeeseAI-generated below but seems plausible? it looks like https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/style_rule_counter_style.h;l=75;drc=08b2c243ef0d95044d60d5de1d28c1281804c25f needs to be incremented on mutations and we might be bypassing it when editing?
>>>
When a `CSSCounterStyleRule`'s properties are modified via the `StyleRuleCSSStyleDeclaration` (which is what `style()` returns), the underlying `StyleRuleCounterStyle`'s `version_` must be incremented.
Currently, `SetDescriptorValue` increments the version, but that method is only called by CSSOM setters (like `rule.system = '...'`). The inspector uses `setStyleTexts`, which calls `setCSSText` on the `CSSStyleDeclaration`. This modifies the property set directly but doesn't know about the `version_` field.
If the version isn't incremented, `CounterStyleMap` might not re-resolve the style, and the page won't update to reflect the inspector's changes. You might need to override `DidModify` or similar in `CSSCounterStyleRule` to ensure the internal rule's version is updated when the declaration changes.
Anders Hartvoll RuudThere does seem to be some sort of dirty tracking going on in `StyleRuleCounterStyle` that none of the other style rule classes have. I don't see any `DidModify` function or similar. Needs further investigation. The author is on leave. @andruud, any suggestions?
Eric LeeseMost at-rules affect how `ComputedStyle`s are produced, and so need to invalidate style when they change. If the `ComputedStyle`s change in a way that affects layout, then layout will be invalidated via the regular mechanisms of diffing the old vs. new `ComputedStyle` objects.
If I remember correctly, counters don't actually affect `ComputedStyle` objects, but _do_ affect layout. So they need a special path to invalidate layout _directly_.
So there is indeed a problem here. I need to think a bit about the solution ...
So the straightforward if not pretty solution I see is:
1. Add an UpdateVersion() method to CSSCounterStyleRule and StyleRuleCounterStyle to bump the version.
2. Add comments to the header file warning that if you use the style() wrapper to modify the properties you must also call UpdateVersion().
3. Below this, after the call to setCSSText, check again if it is a CSSCounterStyleRule and call UpdateVersion() then.Would that solution be acceptable here?
- Anders Hartvoll Ruud
- Eric Leese
- Philip Pfaffe
Eric Leese (Gerrit)
Eric Leese added 1 comment![]( "Open in Gerrit")
Did you mean to make this static?
- Anders Hartvoll Ruud
- Philip Pfaffe
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. |
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud added 1 comment![]( "Open in Gerrit")
style = counter_style_rule->style();Eric LeeseAI-generated below but seems plausible? it looks like https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/style_rule_counter_style.h;l=75;drc=08b2c243ef0d95044d60d5de1d28c1281804c25f needs to be incremented on mutations and we might be bypassing it when editing?
>>>
When a `CSSCounterStyleRule`'s properties are modified via the `StyleRuleCSSStyleDeclaration` (which is what `style()` returns), the underlying `StyleRuleCounterStyle`'s `version_` must be incremented.
Currently, `SetDescriptorValue` increments the version, but that method is only called by CSSOM setters (like `rule.system = '...'`). The inspector uses `setStyleTexts`, which calls `setCSSText` on the `CSSStyleDeclaration`. This modifies the property set directly but doesn't know about the `version_` field.
If the version isn't incremented, `CounterStyleMap` might not re-resolve the style, and the page won't update to reflect the inspector's changes. You might need to override `DidModify` or similar in `CSSCounterStyleRule` to ensure the internal rule's version is updated when the declaration changes.
Anders Hartvoll RuudThere does seem to be some sort of dirty tracking going on in `StyleRuleCounterStyle` that none of the other style rule classes have. I don't see any `DidModify` function or similar. Needs further investigation. The author is on leave. @andruud, any suggestions?
Eric LeeseMost at-rules affect how `ComputedStyle`s are produced, and so need to invalidate style when they change. If the `ComputedStyle`s change in a way that affects layout, then layout will be invalidated via the regular mechanisms of diffing the old vs. new `ComputedStyle` objects.
If I remember correctly, counters don't actually affect `ComputedStyle` objects, but _do_ affect layout. So they need a special path to invalidate layout _directly_.
So there is indeed a problem here. I need to think a bit about the solution ...
Alex RudenkoSo the straightforward if not pretty solution I see is:
1. Add an UpdateVersion() method to CSSCounterStyleRule and StyleRuleCounterStyle to bump the version.
2. Add comments to the header file warning that if you use the style() wrapper to modify the properties you must also call UpdateVersion().
3. Below this, after the call to setCSSText, check again if it is a CSSCounterStyleRule and call UpdateVersion() then.Would that solution be acceptable here?
(leaving the decision here to Anders)
(Apologies for not getting back to this, like I said I would.)
OK, let's do roughly what you're saying, Eric, but I think we can make it a little more solid by adding a `CSSStyleDeclaration* CSSCounterStyleRule::MutateStyleForInspector()` [1] which always does `UpdateVersion()` internally? In `SetStyleText`, it seems that once we call `Style()`, we've already made up our mind to modify it, so it wouldn't even over-invalidate? (And if it does, maybe that's non-catastrophic.)
I would like to avoid explicitly managing versions from the outside. It feels a bit fragile.
[1] Or some other name that gives a hint that calling the function causes invalidation.
Related details
- Eric Leese
- Philip Pfaffe
Eric Leese (Gerrit)
Eric Leese added 1 comment![]( "Open in Gerrit")
Okay, that is even simpler. It feels wrong to bump the version before modifying the value, but since that doesn't directly trigger layout it doesn't matter which order these happen in.
Related details
- Alex Rudenko
- Anders Hartvoll Ruud
- Philip Pfaffe
Eric Leese (Gerrit)
Eric Leese added 1 comment![]( "Open in Gerrit")
style = counter_style_rule->style();Done
Related details
- Alex Rudenko
- Anders Hartvoll Ruud
- Philip Pfaffe
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Pfaffe (Gerrit)
Philip Pfaffe voted and added 1 comment![]( "Open in Gerrit")
Votes added by Philip Pfaffe
| Code-Review | +1 |
1 comment
counter_style_rule_->UpdateVersion();Love this!
Related details
- Alex Rudenko
- Anders Hartvoll Ruud
- Eric Leese
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Pfaffe (Gerrit)
Philip Pfaffe added 1 comment![]( "Open in Gerrit")
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud voted and added 2 comments![]( "Open in Gerrit")
Votes added by Anders Hartvoll Ruud
| Code-Review | +1 |
2 comments
lgtm if we can remove `UpdateVersion()`.
void UpdateVersion() { ++version_; }I think you should drop this, rename `MutableProperties` to `MutableStyleForInspector`, and do `++version_` on the inside. (And maybe add a comment saying "See CSSCounterStyleRule::MutableStyleForInspector`.)
Related details
- Alex Rudenko
- Eric Leese
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. |
Eric Leese (Gerrit)
Eric Leese added 1 comment![]( "Open in Gerrit")
void UpdateVersion() { ++version_; }I think you should drop this, rename `MutableProperties` to `MutableStyleForInspector`, and do `++version_` on the inside. (And maybe add a comment saying "See CSSCounterStyleRule::MutableStyleForInspector`.)
This means we must always update layout when getMatchedStyleForNode is called and it returns a @counter-style rule, even if no styles were changed, and also we can't keep the cssom wrapper object around but must create a new one each time. This isn't necessarily an unreasonable cost, just wanted to note it.
Related details
- Alex Rudenko
- Anders Hartvoll Ruud
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. |
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud added 1 comment![]( "Open in Gerrit")
void UpdateVersion() { ++version_; }Eric LeeseI think you should drop this, rename `MutableProperties` to `MutableStyleForInspector`, and do `++version_` on the inside. (And maybe add a comment saying "See CSSCounterStyleRule::MutableStyleForInspector`.)
This means we must always update layout when getMatchedStyleForNode is called and it returns a @counter-style rule, even if no styles were changed, and also we can't keep the cssom wrapper object around but must create a new one each time. This isn't necessarily an unreasonable cost, just wanted to note it.
Even if you add a function here `Properties()` analogous to `CSSCounterStyleRule::Style()` which does _not_ call `++version_`?
Related details
- Alex Rudenko
- Eric Leese
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. |
Eric Leese (Gerrit)
Eric Leese added 1 comment![]( "Open in Gerrit")
Eric LeeseI think you should drop this, rename `MutableProperties` to `MutableStyleForInspector`, and do `++version_` on the inside. (And maybe add a comment saying "See CSSCounterStyleRule::MutableStyleForInspector`.)
Anders Hartvoll RuudThis means we must always update layout when getMatchedStyleForNode is called and it returns a @counter-style rule, even if no styles were changed, and also we can't keep the cssom wrapper object around but must create a new one each time. This isn't necessarily an unreasonable cost, just wanted to note it.
Even if you add a function here `Properties()` analogous to `CSSCounterStyleRule::Style()` which does _not_ call `++version_`?
Done
Related details
- Alex Rudenko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |