[webkit-reviews] review granted: [Bug 182741] [Web Animations] Make KeyframeEffect target nullable and read-write : [Attachment 334196] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Antoine Quint <graouts at apple.com>'s
request for review:
Bug 182741: [Web Animations] Make KeyframeEffect target nullable and read-write
https://bugs.webkit.org/show_bug.cgi?id=182741

Attachment 334196: Patch

https://bugs.webkit.org/attachment.cgi?id=334196&action=review




--- Comment #41 from Dean Jackson <dino at apple.com> ---
Comment on attachment 334196
  --> https://bugs.webkit.org/attachment.cgi?id=334196
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=334196&action=review

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:53
> +    element->invalidateStyleAndLayerComposition();
> +    element->document().updateStyleIfNeeded();

Did you ever work out why we need to call both here? Shouldn't
Element::invalidateStyleAndLayerComposition call
document().updateStyleIfNeeded()?

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:72
> +    // 1. If attribute conforms to the <custom-property-name> production,
return attribute.

Add a comment as to why this isn't being handled.

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:76
> +    // 3. If attribute is the string "cssOffset", then return an animation
property representing the CSS offset property.

Add a blank line here.

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:167
> +		   easing = convert<IDLDOMString>(state, keyframe->get(&state,
ownPropertyName));

Why do you have to pass the ExecState to the keyframe?

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:649
> +    auto previousTarget = std::exchange(m_target, WTFMove(newTarget));

std::exchange, nice.