| Summary: | [Web Inspector] Update return value name for Animation.requestEffectTarget() | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Antoine Quint <graouts> | ||||||
| Component: | Web Inspector | Assignee: | Antoine Quint <graouts> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | ews-watchlist, hi, inspector-bugzilla-changes, joepeck, keith_miller, mark.lam, msaboff, pangle, saam, tzagallo, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Antoine Quint
2022-01-26 11:36:20 PST
Created attachment 450047 [details]
Patch
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features. Created attachment 450049 [details]
Patch
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. Committed r288668 (246474@trunk): <https://commits.webkit.org/246474@trunk> 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. |