[webkit-reviews] review granted: [Bug 182741] [Web Animations] Make KeyframeEffect target nullable and read-write : [Attachment 333722] 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 333722: Patch

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




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

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

> Source/WebCore/ChangeLog:9
> +	   We used to completely disregard null targets, for instance not
parsing keyframes, but targets
> +	   can be null and are also supposed to be read-write for
KeyframeEffect. We now update the IDL

This sentence is weird.

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:555
> +    if (!scriptExecutionContext.isDocument())
> +	   return Exception { TypeError };

Is there any way to create a keyframe animation in a worker that operates on a
non-element?

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:558
> +    auto& document = downcast<Document>(scriptExecutionContext);
> +    auto documentElement = document.documentElement();

No need for the document local variable.

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:586
> +	   renderStyle->fontCascade().update(nullptr);

Maybe put a comment here?

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

Why do you need both?

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

Ditto?