WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
237073
[css-cascade] Support 'revert' in @keyframes
https://bugs.webkit.org/show_bug.cgi?id=237073
Summary
[css-cascade] Support 'revert' in @keyframes
Oriol Brufau
Reported
2022-02-22 19:28:07 PST
Created
attachment 452930
[details]
testcase 'initial', 'unset' and 'inherit' work well in @keyframes. But 'revert' doesn't work, it just behaves as 'unset'. It works in Blink:
https://chromium-review.googlesource.com/c/chromium/src/+/2132249
Attachments
testcase
(560 bytes, text/html)
2022-02-22 19:28 PST
,
Oriol Brufau
no flags
Details
Patch
(8.78 KB, patch)
2022-02-22 19:41 PST
,
Oriol Brufau
no flags
Details
Formatted Diff
Diff
Patch
(8.79 KB, patch)
2022-02-22 19:48 PST
,
Oriol Brufau
no flags
Details
Formatted Diff
Diff
Patch
(10.50 KB, patch)
2022-02-23 05:53 PST
,
Oriol Brufau
no flags
Details
Formatted Diff
Diff
Patch
(10.50 KB, patch)
2022-02-23 05:55 PST
,
Oriol Brufau
no flags
Details
Formatted Diff
Diff
Patch
(10.48 KB, patch)
2022-02-23 10:11 PST
,
Oriol Brufau
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Oriol Brufau
Comment 1
2022-02-22 19:41:06 PST
Created
attachment 452931
[details]
Patch
Oriol Brufau
Comment 2
2022-02-22 19:48:07 PST
Created
attachment 452933
[details]
Patch
Oriol Brufau
Comment 3
2022-02-23 05:53:03 PST
Created
attachment 452968
[details]
Patch
Oriol Brufau
Comment 4
2022-02-23 05:55:37 PST
Created
attachment 452969
[details]
Patch
Darin Adler
Comment 5
2022-02-23 08:55:21 PST
Comment on
attachment 452969
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=452969&action=review
Seems fine as is, a few thoughts
> Source/WebCore/style/StyleResolver.cpp:283 > + unsigned propertyCount = keyframe.properties().propertyCount(); > + bool hasRevert = false; > + for (unsigned i = 0; i < propertyCount; ++i) {
I think in future we should add begin/end to StyleProperties so loops like this can be a modern style for loop instead of an indexed one like this. Not in this patch, but important for the future, I think.
> Source/WebCore/style/StyleResolver.cpp:284 > + const auto& propertyReference = keyframe.properties().propertyAt(i);
This should just be "auto". The wordier "const auto&" has no benefit here as far as I can tell.
> Source/WebCore/style/StyleResolver.cpp:292 > + if (const CSSValue* value = propertyReference.value()) > + hasRevert = hasRevert || value->isRevertValue();
I would write it this way: if (auto* value = propertyReference.value(); value && value->isRevertValue()) hasRevert = true; There are many different styles, but once we are doing an if, I think it’s best to include both checks in the if, it’s useful to use auto in a case like this to make sure we don’t accidentally upcast? If we had a helper function, could also consider: hasRevert = hasRevert || isRevertValue(propertyReference.value()); Might be nice to save the work of getting the value and type-checking it entirely once we found a revert, but also that doesn’t seem to be an important optimization.
> Source/WebCore/style/StyleResolver.cpp:309 > + ElementRuleCollector collector(element, m_ruleSets, context.selectorMatchingState); > + collector.setPseudoElementRequest({ elementStyle.styleType() }); > + if (hasRevert) { > + // In the animation origin, 'revert' rolls back the cascaded value to the user level. > + // Therefore, we need to collect UA and user rules. > + collector.setMedium(&m_mediaQueryEvaluator); > + collector.matchUARules(); > + collector.matchUserRules(); > + } > + collector.addAuthorKeyframeRules(keyframe);
This looks kind of expensive. Would like to know whether Antti or some other expert thinks this is OK. Not necessarily before landing? Also, if there is a lot of state in the ElementRuleCollector might be nice to get the match result and then let that go out of scope before allocating the Builder, by using a lambda, braces, or a helper function.
Oriol Brufau
Comment 6
2022-02-23 09:15:42 PST
Comment on
attachment 452969
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=452969&action=review
>> Source/WebCore/style/StyleResolver.cpp:309 >> + collector.addAuthorKeyframeRules(keyframe); > > This looks kind of expensive. Would like to know whether Antti or some other expert thinks this is OK. Not necessarily before landing? Also, if there is a lot of state in the ElementRuleCollector might be nice to get the match result and then let that go out of scope before allocating the Builder, by using a lambda, braces, or a helper function.
But ElementRuleCollector::matchResult() returns a reference to a member, if the ElementRuleCollector instance goes out of scope, wouldn't using the match result be undefined behavior? Or if I copy it, then it can also be slow if the vectors of matched properties are big.
Darin Adler
Comment 7
2022-02-23 09:30:10 PST
Comment on
attachment 452969
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=452969&action=review
>>> Source/WebCore/style/StyleResolver.cpp:309 >>> + collector.addAuthorKeyframeRules(keyframe); >> >> This looks kind of expensive. Would like to know whether Antti or some other expert thinks this is OK. Not necessarily before landing? Also, if there is a lot of state in the ElementRuleCollector might be nice to get the match result and then let that go out of scope before allocating the Builder, by using a lambda, braces, or a helper function. > > But ElementRuleCollector::matchResult() returns a reference to a member, if the ElementRuleCollector instance goes out of scope, wouldn't using the match result be undefined behavior? Or if I copy it, then it can also be slow if the vectors of matched properties are big.
Good point, we would have to make a copy. For now, I guess we can leave this as-is. We can later add takeMatchResult which moves it instead of copying. For now, I we can leave this as-is, but I am not sure the current approach is ideal for memory use.
Oriol Brufau
Comment 8
2022-02-23 09:48:15 PST
Comment on
attachment 452969
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=452969&action=review
>> Source/WebCore/style/StyleResolver.cpp:284 >> + const auto& propertyReference = keyframe.properties().propertyAt(i); > > This should just be "auto". The wordier "const auto&" has no benefit here as far as I can tell.
Not an expert, doesn't "auto" alone make an unnecessary copy?
https://stackoverflow.com/questions/7138588/c11-auto-what-if-it-gets-a-constant-reference
Darin Adler
Comment 9
2022-02-23 10:05:09 PST
Comment on
attachment 452969
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=452969&action=review
>>> Source/WebCore/style/StyleResolver.cpp:284 >>> + const auto& propertyReference = keyframe.properties().propertyAt(i); >> >> This should just be "auto". The wordier "const auto&" has no benefit here as far as I can tell. > > Not an expert, doesn't "auto" alone make an unnecessary copy?
https://stackoverflow.com/questions/7138588/c11-auto-what-if-it-gets-a-constant-reference
No. The return value of propertyAt is a PropertyReference object. That object will always be copied (or moved). Using the "const auto&" syntax has no effect on that one way or the other. If the return value was a reference, that would be different, and then I would suggest "auto&", not "const auto&".
Oriol Brufau
Comment 10
2022-02-23 10:11:13 PST
Created
attachment 452997
[details]
Patch
Oriol Brufau
Comment 11
2022-02-23 10:16:00 PST
Comment on
attachment 452969
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=452969&action=review
>>>> Source/WebCore/style/StyleResolver.cpp:284 >>>> + const auto& propertyReference = keyframe.properties().propertyAt(i); >>> >>> This should just be "auto". The wordier "const auto&" has no benefit here as far as I can tell. >> >> Not an expert, doesn't "auto" alone make an unnecessary copy?
https://stackoverflow.com/questions/7138588/c11-auto-what-if-it-gets-a-constant-reference
> > No. > > The return value of propertyAt is a PropertyReference object. That object will always be copied (or moved). Using the "const auto&" syntax has no effect on that one way or the other. > > If the return value was a reference, that would be different, and then I would suggest "auto&", not "const auto&".
Ah, sure, somehow "PropertyReference" made my mind think it was a a reference XD. Done.
>> Source/WebCore/style/StyleResolver.cpp:292 >> + hasRevert = hasRevert || value->isRevertValue(); > > I would write it this way: > > if (auto* value = propertyReference.value(); value && value->isRevertValue()) > hasRevert = true; > > There are many different styles, but once we are doing an if, I think it’s best to include both checks in the if, it’s useful to use auto in a case like this to make sure we don’t accidentally upcast? If we had a helper function, could also consider: > > hasRevert = hasRevert || isRevertValue(propertyReference.value()); > > Might be nice to save the work of getting the value and type-checking it entirely once we found a revert, but also that doesn’t seem to be an important optimization.
Done.
>>>> Source/WebCore/style/StyleResolver.cpp:309 >>>> + collector.addAuthorKeyframeRules(keyframe); >>> >>> This looks kind of expensive. Would like to know whether Antti or some other expert thinks this is OK. Not necessarily before landing? Also, if there is a lot of state in the ElementRuleCollector might be nice to get the match result and then let that go out of scope before allocating the Builder, by using a lambda, braces, or a helper function. >> >> But ElementRuleCollector::matchResult() returns a reference to a member, if the ElementRuleCollector instance goes out of scope, wouldn't using the match result be undefined behavior? Or if I copy it, then it can also be slow if the vectors of matched properties are big. > > Good point, we would have to make a copy. For now, I guess we can leave this as-is. > > We can later add takeMatchResult which moves it instead of copying. For now, I we can leave this as-is, but I am not sure the current approach is ideal for memory use.
OK, I guess I will wait a bit in case Antti says something, otherwise land anyways.
EWS
Comment 12
2022-02-24 13:20:51 PST
Committed
r290457
(
247758@main
): <
https://commits.webkit.org/247758@main
> All reviewed patches have been landed. Closing bug and clearing flags on
attachment 452997
[details]
.
Radar WebKit Bug Importer
Comment 13
2022-02-24 13:21:17 PST
<
rdar://problem/89436026
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug