Use ScopedCSSName for animation-name to fix cross-scope keyframes lookup [chromium/src : main]
Robert Flack (Gerrit)
Robert Flack added 7 comments![]( "Open in Gerrit")
bool CSSAnimationData::NamesMatch(const CSSAnimationData& other) const {What's the difference between this method and the HeapVector::operator==? Why can we not use the == operator?
const TreeScope* name_tree_scope = nullptr) {When do we not pass the tree scope for the name? Can we require that a value is passed here? It looks like all calls already pass a value.
AtomicString name = scoped_name ? scoped_name->GetName() : AtomicString();In CalculateAnimationUpdate we expected CSSAnimationData::InitialName() for these cases, is there a difference between AtomicString() and AtomicString("")? Should we continue to have a single static method to serve as the unset string value?
scoped_name ? scoped_name->GetName() : g_empty_atom;Here you use g_empty_atom where in another function you use AtomicString, and where previously we used a static local AtomicString(""). What's the expected consistent name to use?
const TreeScope* name_tree_scope = nullptr);It looks like all call sites pass a name_tree_scope, don't have a default value to ensure that all future code passes a tree scope. Otherwise we could end up with inconsistent lookups.
if (name_tree_scope) {Is there any case where we do not pass in a name_tree_scope? What does this look like?
}Rather than duplicating this code in the else block, move this resolvers.empty() check and code after the if block.
Related details
- Hyowon Kim
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. |
Hyowon Kim (Gerrit)
Hyowon Kim added 7 comments![]( "Open in Gerrit")
bool CSSAnimationData::NamesMatch(const CSSAnimationData& other) const {What's the difference between this method and the HeapVector::operator==? Why can we not use the == operator?
`name_list_` is `HeapVector<Member<const ScopedCSSName>>`.
`HeapVector<Member<T>>::operator==` compares `Member` pointers, not the pointed-to values. Since two different `ScopedCSSName` objects can have the same name and tree scope, we need `base::ValuesEquivalent` to do value comparison.
This is the same pattern used by `ScopedCSSNameList::operator==`.
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/style/scoped_css_name.h;l=72-77
const TreeScope* name_tree_scope = nullptr) {When do we not pass the tree scope for the name? Can we require that a value is passed here? It looks like all calls already pass a value.
You're right. All callers already pass a value. I'll remove the default and make it a required parameter.
AtomicString name = scoped_name ? scoped_name->GetName() : AtomicString();In CalculateAnimationUpdate we expected CSSAnimationData::InitialName() for these cases, is there a difference between AtomicString() and AtomicString("")? Should we continue to have a single static method to serve as the unset string value?
Good catch - added CSSAnimationData::InitialNameString() as a single static method for the unset name string, and updated this call site to use it.
scoped_name ? scoped_name->GetName() : g_empty_atom;Here you use g_empty_atom where in another function you use AtomicString, and where previously we used a static local AtomicString(""). What's the expected consistent name to use?
Right, same fix - replaced g_empty_atom with CSSAnimationData::InitialNameString() for consistency.
const TreeScope* name_tree_scope = nullptr);It looks like all call sites pass a name_tree_scope, don't have a default value to ensure that all future code passes a tree scope. Otherwise we could end up with inconsistent lookups.
`name_tree_scope` can be null in two cases:
1) test code in style_engine_test.cc calls `FindKeyframesRule` without it.
- [style_engine_test.cc;l=473](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/style_engine_test.cc;l=473)
2) `ScopedCSSName::GetTreeScope()` returns null for UA stylesheet names.
- [scoped_css_name.h;l=23-26](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/style/scoped_css_name.h;l=23-26)
I'll remove the default value and have these call sites pass nullptr explicitly.
if (name_tree_scope) {Is there any case where we do not pass in a name_tree_scope? What does this look like?
Please see my reply above.
Rather than duplicating this code in the else block, move this resolvers.empty() check and code after the if block.
These two branches can't be merged because they work differently:
- `if (name_tree_scope)` branch:
- uses `ScopedResolverFor` only as a fallback — only when `name_tree_scope` has no resolver.
- `else` branch:
- always adds `ScopedResolverFor` in addition to hosted shadow tree resolvers.
If we move the `resolvers.empty()` check after the if/else block, the `else` branch would skip `ScopedResolverFor` when `CollectScopedResolversForHostedShadowTrees` already added resolvers.
Related details
- Robert Flack
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. |