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
Created attachment 452931 [details] Patch
Created attachment 452933 [details] Patch
Created attachment 452968 [details] Patch
Created attachment 452969 [details] Patch
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.
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.
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.
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
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&".
Created attachment 452997 [details] Patch
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.
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].
<rdar://problem/89436026>