[webkit-reviews] review granted: [Bug 235661] [Web Inspector] Update return value name for Animation.requestEffectTarget() : [Attachment 450049] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Antoine Quint
<graouts at webkit.org>'s request for review:
Bug 235661: [Web Inspector] Update return value name for
Animation.requestEffectTarget()
https://bugs.webkit.org/show_bug.cgi?id=235661

Attachment 450049: Patch

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




--- Comment #4 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 450049
  --> https://bugs.webkit.org/attachment.cgi?id=450049
Patch

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

r=me thanks for fixing :)

> Source/WebInspectorUI/UserInterface/Models/Animation.js:213
>	       // COMPATIBILITY (iOS 15): effectTarget changed from DOM.NodeId
to DOM.Styleable.

NIT: Please adjust this comment to also indicate that the name of the parameter
changed.  This is important because if one does not provide a callback to a
protocol command, the response is encoded into an object that's used as the
resolved value of a returned promise (e.g. `let {effectTarget} = await
target.AnimationAgent.requestEffectTarget(this._animationId);`), so the
property name in the protocol could affect that.
```
    // COMPATIBILITY (iOS 15): nodeId was renamed to effectTarget and changed
from DOM.NodeId to DOM.Styleable.
```

Also, you could/should remove the comment above on :194 since it's a duplicate
(and it's also not really accurate, because
`WI.Animation.prototype.requestEffectTarget` will always return a
`WI.DOMStyleable` regardless of the protocol).	It's only the `if
(!isNaN(effectTarget))` line that matters for compatibility.