Bug 235661 - [Web Inspector] Update return value name for Animation.requestEffectTarget()
Summary: [Web Inspector] Update return value name for Animation.requestEffectTarget()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-01-26 11:36 PST by Antoine Quint
Modified: 2022-03-01 02:05 PST (History)
11 users (show)

See Also:


Attachments
Patch (1.58 KB, patch)
2022-01-26 11:37 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (3.68 KB, patch)
2022-01-26 11:43 PST, Antoine Quint
hi: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2022-01-26 11:36:20 PST
[Web Inspector] Update return value name for Animation.requestEffectTarget()
Comment 1 Antoine Quint 2022-01-26 11:37:50 PST
Created attachment 450047 [details]
Patch
Comment 2 EWS Watchlist 2022-01-26 11:40:17 PST
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Comment 3 Antoine Quint 2022-01-26 11:43:19 PST
Created attachment 450049 [details]
Patch
Comment 4 Devin Rousso 2022-01-26 13:58:25 PST
Comment on attachment 450049 [details]
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.
Comment 5 Antoine Quint 2022-01-26 23:21:48 PST
Committed r288668 (246474@trunk): <https://commits.webkit.org/246474@trunk>
Comment 6 Radar WebKit Bug Importer 2022-01-26 23:22:18 PST
<rdar://problem/88116899>
Comment 7 Patrick Angle 2022-01-27 15:42:10 PST
Comment on attachment 450049 [details]
Patch

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

>> 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.

FYI this will actually need to be `// COMPATIBILITY (iOS 15.4)`, the protocol definition for which is in Bug 235741.