Bug 232086

<rdar://problem/84508394>

Created attachment 448286 [details] Patch for EWS

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

I have filed https://github.com/web-platform-tests/wpt/pull/32237 to upstream WPT coverage for this new property.

I have filed https://github.com/w3c/csswg-drafts/pull/6930 to add support for animation-composition within the animation shorthand.

Created attachment 448297 [details] Patch for EWS

Created attachment 448308 [details] Patch for EWS

Created attachment 448364 [details] Patch for EWS

Created attachment 449752 [details] Patch

Created attachment 449755 [details] Patch

Comment on attachment 449755 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449755&action=review Looks great, a few comments about improving the coding style a bit > Source/WebCore/animation/CompositeOperation.h:28 > +#include "CSSPrimitiveValue.h" Seems unfortunate to have to add so many headers to this header. Is it super important that the function be inline? > Source/WebCore/animation/CompositeOperation.h:36 > +inline std::optional<CompositeOperation> compositeOperationFromCSSValue(const CSSValue& value) I would omit the words “FromCSSValue” from this function name. We only need to mention the argument types in the function name if it’s particularly surprising or there is some kind of overloading ambiguity we are trying to work around. Otherwise, it just makes our code unnecessarily wordy. > Source/WebCore/animation/KeyframeEffect.cpp:611 > + auto supportsCompositeOperation = is<Document>(scriptExecutionContext) && downcast<Document>(*scriptExecutionContext).settings().webAnimationsCompositeOperationsEnabled(); Should we add a way to get to settings from any script execution context? It seems unnecessary here that we are down casting to document. Or maybe we should change the arguments that are passed to this by DOM bindings? There is a CallWith=Document available. > Source/WebCore/animation/KeyframeEffect.cpp:710 > + computedKeyframe.composite = CompositeOperationOrAuto::Replace; Seems like we need a function that converts CompositeOperation into CompositeOperationOrAuto. No way we want to write this out as a switch statement here. Even better there may be a way to make that an implicit conversion but a named function would still be better than a switch statement here. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1381 > +static Ref<CSSPrimitiveValue> valueForAnimationComposition(CompositeOperation compositeOperation) Same comment about the function name including the words “ForAnimationComposition”. A shorter name for this function would be better. Also I suggest naming the argument a single word operation. In the context of this function there aren’t different types of operation that we need to disambiguate.

(In reply to Darin Adler from comment #11) > Comment on attachment 449755 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=449755&action=review > > Looks great, a few comments about improving the coding style a bit > > > Source/WebCore/animation/CompositeOperation.h:28 > > +#include "CSSPrimitiveValue.h" > > Seems unfortunate to have to add so many headers to this header. Is it super > important that the function be inline? No, I can introduce a CompositeOperation.cpp. > > Source/WebCore/animation/CompositeOperation.h:36 > > +inline std::optional<CompositeOperation> compositeOperationFromCSSValue(const CSSValue& value) > > I would omit the words “FromCSSValue” from this function name. We only need > to mention the argument types in the function name if it’s particularly > surprising or there is some kind of overloading ambiguity we are trying to > work around. Otherwise, it just makes our code unnecessarily wordy. Do you think `toCompositeOperation(const CSSValue&)` would be better? Simply `compositeOperation(const CSSValue&)` seems to not make it clear that this method returns a CompositeOperation, it could be instead be an action of some sort… Maybe we could expose this as CSSValue::toCompositeOperation()? I'll go for `toCompositeOperation(const CSSValue&)` in my patch for landing. > > Source/WebCore/animation/KeyframeEffect.cpp:611 > > + auto supportsCompositeOperation = is<Document>(scriptExecutionContext) && downcast<Document>(*scriptExecutionContext).settings().webAnimationsCompositeOperationsEnabled(); > > Should we add a way to get to settings from any script execution context? It > seems unnecessary here that we are down casting to document. > Or maybe we should change the arguments that are passed to this by DOM > bindings? There is a CallWith=Document available. The need to have access to Document here is only to access settings, and hopefully this won't be around much longer once we don't need a setting for this anymore. I'd rather not expose something to the IDL because it's not actually required for the proper implementation of the binding itself, but maybe my misgiving is unfounded. > > Source/WebCore/animation/KeyframeEffect.cpp:710 > > + computedKeyframe.composite = CompositeOperationOrAuto::Replace; > > Seems like we need a function that converts CompositeOperation into > CompositeOperationOrAuto. No way we want to write this out as a switch > statement here. Even better there may be a way to make that an implicit > conversion but a named function would still be better than a switch > statement here. I suppose we could have a toCompositeOperationOrAuto(), although simply doing static_cast<CompositeOperationOrAuto>(*compositeOperation) works, but I don't know if that's advisable. I don't actually know how to write the implicit conversion between two enums, but it does sound nice. I'll go for the static_cast approach in my patch for landing. > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1381 > > +static Ref<CSSPrimitiveValue> valueForAnimationComposition(CompositeOperation compositeOperation) > > Same comment about the function name including the words > “ForAnimationComposition”. A shorter name for this function would be better. > Also I suggest naming the argument a single word operation. In the context > of this function there aren’t different types of operation that we need to > disambiguate. The "AnimationComposition" here points to the CSS property name and follows a pattern in CSSComputedStyleDeclaration.cpp. As such, I think it should stay, though I'll change the parameter name.

Created attachment 449760 [details] Patch for landing

Created attachment 449761 [details] Patch for landing

Comment on attachment 449755 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449755&action=review >>> Source/WebCore/animation/CompositeOperation.h:36 >>> +inline std::optional<CompositeOperation> compositeOperationFromCSSValue(const CSSValue& value) >> >> I would omit the words “FromCSSValue” from this function name. We only need to mention the argument types in the function name if it’s particularly surprising or there is some kind of overloading ambiguity we are trying to work around. Otherwise, it just makes our code unnecessarily wordy. > > Do you think `toCompositeOperation(const CSSValue&)` would be better? Simply `compositeOperation(const CSSValue&)` seems to not make it clear that this method returns a CompositeOperation, it could be instead be an action of some sort… Maybe we could expose this as CSSValue::toCompositeOperation()? > > I'll go for `toCompositeOperation(const CSSValue&)` in my patch for landing. toCompositeOperation is OK. I also like compositeOperation despite the things you said. >>> Source/WebCore/animation/KeyframeEffect.cpp:611 >>> + auto supportsCompositeOperation = is<Document>(scriptExecutionContext) && downcast<Document>(*scriptExecutionContext).settings().webAnimationsCompositeOperationsEnabled(); >> >> Should we add a way to get to settings from any script execution context? It seems unnecessary here that we are down casting to document. >> >> Or maybe we should change the arguments that are passed to this by DOM bindings? There is a CallWith=Document available. > > The need to have access to Document here is only to access settings, and hopefully this won't be around much longer once we don't need a setting for this anymore. I'd rather not expose something to the IDL because it's not actually required for the proper implementation of the binding itself, but maybe my misgiving is unfounded. I guess that’s OK, but I not thrilled with writing custom code that walks around between objects if our IDL generator can do a great job giving us exactly what we need. If you read this code you have to think about why we have a guarantee the global object is JSDOMGlobalObject, why we don’t have a guarantee that the scriptExecutionContext is a document, why it’s OK to have this unconditionally on if called from a non-Document ScriptExecutionContext. These are not just theoretical concerns, practical ones as well. By the way, I am almost certain the the bindings pass us a JSDOMGlobalObject&, and we are the ones asking only for the JSGlobalObject& base class, so we can change the getKeyframes argument and then we don’t need a jsCast. That will make the code slightly smaller and faster, as well as more elegant. Separate thought about getKeyframes future cleanup: The fact that this colossal function includes all kinds of custom JS stuff is unpleasant. Typically we want DOM code that doesn’t cater to JS specifically, and separate bindings code that does the JS wrapping. This function is an unpleasant combination of both right now. The problem starts with "keyframes are represented by a partially open-ended dictionary type that is not currently able to be expressed with WebIDL", but a good starting point would be to find a way to express this dictionary without JS types. Then we would have a function that builds that out of non-JS-specific DOM types, and a separate binding function that just translates that into JS objects. Those can both be in the KeyframeEffect class if we don’t yet have support for it in WebIDL. Interspersing the logic to compute the appropriate key frames with the JS binding is not a good pattern. Unless there is some massive performance benefit to doing it that way? Even then we could use lambdas to separate the code that determines what goes into the dictionary with the JS-specific rules about how they are turned into JS values. >>> Source/WebCore/animation/KeyframeEffect.cpp:710 >>> + computedKeyframe.composite = CompositeOperationOrAuto::Replace; >> >> Seems like we need a function that converts CompositeOperation into CompositeOperationOrAuto. No way we want to write this out as a switch statement here. Even better there may be a way to make that an implicit conversion but a named function would still be better than a switch statement here. > > I suppose we could have a toCompositeOperationOrAuto(), although simply doing static_cast<CompositeOperationOrAuto>(*compositeOperation) works, but I don't know if that's advisable. I don't actually know how to write the implicit conversion between two enums, but it does sound nice. > > I'll go for the static_cast approach in my patch for landing. Simply doing static_cast<CompositeOperationOrAuto>(*compositeOperation) works? That’s frustrating, because I believe that would work equally well if the two enumerations had different values for the same named constants or if one was not a superset of the other. Would convert between two unrelated types, which would hide a bug. Or could be used to convert CompositeOperationOrAuto to CompositeOperation, hiding the fact that we didn’t check for auto. None of those are true about this call site, which is doing something super-simple and safe. I think we should add a constexpr conversion function to CompositeOperationOrAuto.h or CompositeOperation.h, even if that function simply contains the same static_cast we are talking about here. We could also put static_assert into that function about each of the values being the same, if we wanted, and it would be extremely likely that anyone changing the enumeration would see this. Separately, maybe there’s even a pattern to use inheritance to make one enum out of another plus one extra value? I guess not, but I wish there was. We can call the constexpr conversion function toCompositeOperationOrAuto, following the naming convention you selected above. It’s fine to do this after landing, but I think it’s valuable to make it even more clear the code is correct and safe.

Great work, by the way, love how well this is done. That’s what’s getting me to comment about how to do it even better.

Comment on attachment 449761 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=449761&action=review > Source/WebCore/animation/CompositeOperation.h:34 > +std::optional<CompositeOperation> toCompositeOperation(const CSSValue&); For non-unified builds, need to add: #include <optional> to this header.

Comment on attachment 449761 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=449761&action=review >> Source/WebCore/animation/CompositeOperation.h:34 >> +std::optional<CompositeOperation> toCompositeOperation(const CSSValue&); > > For non-unified builds, need to add: > > #include <optional> > > to this header. Also, not 100% sure it should be in this header; could have it anywhere sensible where both CSSStyleMap.cpp and StyleResolver.cpp could find it. In a way it’s more about CSS values than just about the CompositeOperation enumeration.

I'll look into the suggested changes some more, want to make this the best version it can be.

Created attachment 449787 [details] Patch for landing

Patch for landing has a new toCompositeOperationOrAuto() constexpr function that uses a switch which I think is the safest option to convert. I've also added `Document` in the CallWith instruction in the IDL for KeyframeEffect::getKeyframes(). I will make a similar change for other binding methods that need to access the settings in bug 235503. As for the inner-working on getKeyframes(), at the time I initially implemented it I consulted with the bindings experts and this is what we came up with. A challenge is that the properties on the returned JS object are unknown because we need to serialize all CSS properties set for keyframes. That said we can probably clean it up a fair bit, I'll look into it in bug 235504.

Committed r288433 (246322@trunk): <https://commits.webkit.org/246322@trunk>