Bug 205434

Summary:

Web Inspector: add instrumentation for showing existing Web Animations

Product:

WebKit

Reporter:

Devin Rousso <hi>

Component:

Web Inspector

Assignee:

Devin Rousso <hi>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bburg, benjamin, cdumez, cmarcelo, commit-queue, dbates, ews-watchlist, graouts, hi, inspector-bugzilla-changes, joepeck, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Local Build

Hardware:

All

OS:

All

See Also:

https://bugs.webkit.org/show_bug.cgi?id=206899

Bug Depends on:

205791, 206899

Bug Blocks:

205827, 210838

Attachments:

Description	Flags
Patch	none
[Image] After Patch is applied	none
Patch	none
Patch	none
Patch	none
[Image] After Patch is applied	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
[Patch] test modifying JavaScriptCore/inspector/scripts for win	none
[Patch] test modifying WTF/wtf/Assertions.h for win	none
Patch	none
Patch	none

Devin Rousso

Reported 2019-12-18 21:56:51 PST

I'm thinking we expand the scope of the Canvas Tab and make it a Graphics Tab instead, with four different sections: - Canvases (horizontally scrollable) - Web Animations - CSS Animations - CSS Transitions

Attachments
Patch (238.56 KB, patch) 2019-12-18 22:36 PST, Devin Rousso	no flags	Details Formatted Diff Diff
[Image] After Patch is applied (937.53 KB, image/png) 2019-12-18 22:37 PST, Devin Rousso	no flags	Details
Patch (238.63 KB, patch) 2019-12-18 22:43 PST, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (246.54 KB, patch) 2019-12-19 11:20 PST, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (320.24 KB, patch) 2019-12-19 14:55 PST, Devin Rousso	no flags	Details Formatted Diff Diff
[Image] After Patch is applied (1.25 MB, image/png) 2019-12-19 15:20 PST, Devin Rousso	no flags	Details
Patch (318.14 KB, patch) 2019-12-19 20:25 PST, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (319.15 KB, patch) 2019-12-21 22:39 PST, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (316.48 KB, patch) 2019-12-29 12:48 PST, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (315.99 KB, patch) 2020-01-06 13:01 PST, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (317.17 KB, patch) 2020-01-27 15:10 PST, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (318.19 KB, patch) 2020-01-28 10:22 PST, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (318.49 KB, patch) 2020-01-28 11:59 PST, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (318.90 KB, patch) 2020-01-28 12:26 PST, Devin Rousso	no flags	Details Formatted Diff Diff
[Patch] test modifying JavaScriptCore/inspector/scripts for win (336.97 KB, patch) 2020-01-28 23:20 PST, Devin Rousso	no flags	Details Formatted Diff Diff
[Patch] test modifying WTF/wtf/Assertions.h for win (319.20 KB, patch) 2020-01-29 09:53 PST, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (317.89 KB, patch) 2020-01-29 12:17 PST, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (317.40 KB, patch) 2020-01-29 14:34 PST, Devin Rousso	no flags	Details Formatted Diff Diff
Show Obsolete (16) View All Add attachment proposed patch, testcase, etc.

Devin Rousso

Comment 1 2019-12-18 21:57:03 PST

<rdar://problem/28328087>

Devin Rousso

Comment 2 2019-12-18 22:36:53 PST

Created attachment 386064 [details] Patch Needs tests. Too tired. Will write tomorrow :/

Devin Rousso

Comment 3 2019-12-18 22:37:13 PST

Created attachment 386065 [details] [Image] After Patch is applied

EWS Watchlist

Comment 4 2019-12-18 22:38:16 PST

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

Devin Rousso

Comment 5 2019-12-18 22:43:28 PST

Created attachment 386066 [details] Patch Minor dark mode adjustments

Devin Rousso

Comment 6 2019-12-19 11:20:57 PST

Created attachment 386124 [details] Patch Add a Keyframes section to the Animation panel in the details sidebar of the Graphics Tab that contains a CodeMirror for the CSS style of each keyframe

Devin Rousso

Comment 7 2019-12-19 14:55:02 PST

Created attachment 386149 [details] Patch Add tests.\nSend millsecond values from the backend, not seconds.\nFix canvas tests from the changes made to `WI.CanvasManager`.

Devin Rousso

Comment 8 2019-12-19 14:59:23 PST

(In reply to Devin Rousso from comment #7) > Created attachment 386149 [details] > Patch > > Add tests. > Send millsecond values from the backend, not seconds. > Fix canvas tests from the changes made to `WI.CanvasManager`. This should've been formatted better -.-

Devin Rousso

Comment 9 2019-12-19 15:20:09 PST

Created attachment 386154 [details] [Image] After Patch is applied

Devin Rousso

Comment 10 2019-12-19 20:25:24 PST

Created attachment 386176 [details] Patch Switch back to a vertical scrolling list for canvases.

Blaze Burg

Comment 11 2019-12-20 11:09:23 PST

Windows build failure: C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\inspector\agents\InspectorAnimationAgent.h(63,10): error C3668: 'WebCore::InspectorAnimationAgent::resolveAnimation': method with override specifier 'override' did not override any base class methods (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource-84c9f43f-6.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj] Mac WK1 test failures: Regressions: Unexpected text-only failures (2) inspector/animation/lifecycle-css-animation.html [ Failure ] inspector/animation/lifecycle-css-transition.html [ Failure ]

Blaze Burg

Comment 12 2019-12-20 13:51:12 PST

Comment on attachment 386176 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386176&action=review Finished reviewing the backend changes. Looks great so far. > Source/JavaScriptCore/inspector/protocol/Animation.json:44 > + { "name": "iterations", "type": "number", "optional": true }, Nit: iterationCount? > Source/JavaScriptCore/inspector/protocol/Animation.json:46 > + { "name": "iterationDuration", "type": "number", "optional": true }, Please include labels for the units. I assume it's seconds as a double? > Source/JavaScriptCore/inspector/protocol/Animation.json:47 > + { "name": "timingFunction", "type": "string", "optional": true }, Is this a free-form string or should it be an enum? > Source/JavaScriptCore/inspector/protocol/Animation.json:57 > + { "name": "offset", "type": "number" }, Needs units. Is the range [0,1] or [0, 100]? > Source/JavaScriptCore/inspector/protocol/Animation.json:59 > + { "name": "style", "type": "string", "optional": true } Ditto: are these enums, free-form, or what? EDIT: this is a CSS rule body text fragment with multiple property names and values. Please state that. > Source/JavaScriptCore/inspector/protocol/Animation.json:84 > + "name": "requestNode", Nit: requestAffectedNode? requestNodeForAnimation? requestNodeForAnimationEffect? This could probably match the verb of 'resolveAnimation' (which I'm more agreeable to using as-is). Do all animations have a target? Is it associated with the effect or the animation? I'm not sure what the practical difference is, if Effects do not stack into one Animation. > Source/JavaScriptCore/inspector/protocol/Animation.json:131 > + "description": "Dispatched whenever the target of any effect of any animation is changed in any way.", Since the target is not a field of Animation or Effect, I'm a little confused (without reading onward) as to what this would be used for. Does this indicate that the node that would be resolved via Animation.resolveNode is now different than before? > Source/WebCore/animation/KeyframeEffect.cpp:1108 > + m_shouldRunAccelerated = m_blendingKeyframes.size(); I'd do !!m_blendingKeyframes.size(); Just to make it super obvious. > Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:108 > + return document->page() == &m_inspectedPage; Neat! > Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:181 > + result = injectedScript.wrapObject(value, objectGroupName); V. nice. > Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:354 > + return seconds.milliseconds(); ... I see, it's milliseconds. Why not just seconds? It's a double, it doesn't matter either way except milliseconds are harder to read. > Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:380 > + switch (effect->direction()) { Since these are 1:1 with protocol enum values, can you extract it to a static helper protocolValueForPlaybackDirection()? > Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:396 > + case FillMode::None: Ditto (extract it) > Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:415 > + Nit: extra line > Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:418 > + if (is<DeclarativeAnimation>(keyframeEffect.animation())) { This build method is huge. I think the keyframe payload creation could go into its own helper function, as well as the aforementioned enum conversions. > Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:432 > + .setOffset(blendingKeyframe.key()) Still not sure what the units are here (for key()/offset) > Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:436 > + keyframePayload->setEasing(timingFunction->cssText()); I see, it's a CSS timing function (which can be user defined, right?) > Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:441 > + for (auto cssPropertyId : cssPropertyIds) { Needs a comment: "The frontend expects a CSS rule body per each keyframe. Synthesize CSS style property text for each property of the keyframe." > Source/WebCore/inspector/agents/InspectorDOMAgent.h:184 > + int pushNodePathToFrontend(ErrorString, Node*); I think this change could safely go into another patch, but I won't force it.

Devin Rousso

Comment 13 2019-12-20 17:19:44 PST

Comment on attachment 386176 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386176&action=review >> Source/JavaScriptCore/inspector/protocol/Animation.json:44 >> + { "name": "iterations", "type": "number", "optional": true }, > > Nit: iterationCount? That's much better :) >> Source/JavaScriptCore/inspector/protocol/Animation.json:46 >> + { "name": "iterationDuration", "type": "number", "optional": true }, > > Please include labels for the units. I assume it's seconds as a double? Milliseconds, as that's what the Web Animations API uses. >> Source/JavaScriptCore/inspector/protocol/Animation.json:47 >> + { "name": "timingFunction", "type": "string", "optional": true }, > > Is this a free-form string or should it be an enum? This is a CSS string, like "ease" or "cubic-bezier(...)". I'll add a description describing it as such. >> Source/JavaScriptCore/inspector/protocol/Animation.json:57 >> + { "name": "offset", "type": "number" }, > > Needs units. Is the range [0,1] or [0, 100]? It's a decimal percentage, so [0, 1]. >> Source/JavaScriptCore/inspector/protocol/Animation.json:59 >> + { "name": "style", "type": "string", "optional": true } > > Ditto: are these enums, free-form, or what? > > EDIT: this is a CSS rule body text fragment with multiple property names and values. Please state that. Yes, this is a CSS style declaration. >> Source/JavaScriptCore/inspector/protocol/Animation.json:84 >> + "name": "requestNode", > > Nit: requestAffectedNode? requestNodeForAnimation? requestNodeForAnimationEffect? > > This could probably match the verb of 'resolveAnimation' (which I'm more agreeable to using as-is). > > Do all animations have a target? Is it associated with the effect or the animation? I'm not sure what the practical difference is, if Effects do not stack into one Animation. I was trying to match the naming of `Canvas.requestNode`, but the semantics are different enough that we should really call it `requestTarget`. The target (DOM node) is associated with the effect of the animation, both (the target and the effect) of which are nullable. It is possible for the target of an animation to change after it has been created, hence the `Animation.targetChanged` event. >> Source/JavaScriptCore/inspector/protocol/Animation.json:131 >> + "description": "Dispatched whenever the target of any effect of any animation is changed in any way.", > > Since the target is not a field of Animation or Effect, I'm a little confused (without reading onward) as to what this would be used for. Does this indicate that the node that would be resolved via Animation.resolveNode is now different than before? Yes. See above. >> Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:354 >> + return seconds.milliseconds(); > > ... I see, it's milliseconds. Why not just seconds? It's a double, it doesn't matter either way except milliseconds are harder to read. This matches the Web Animations API, as all of the duration values are in milliseconds. I personally would rather use milliseconds as well, as integers (e.g. 400ms) are easier to deal with than decimals (e.g. 0.4s). >> Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:380 >> + switch (effect->direction()) { > > Since these are 1:1 with protocol enum values, can you extract it to a static helper protocolValueForPlaybackDirection()? Sure. >> Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:396 >> + case FillMode::None: > > Ditto (extract it) Ditto sure >> Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:418 >> + if (is<DeclarativeAnimation>(keyframeEffect.animation())) { > > This build method is huge. I think the keyframe payload creation could go into its own helper function, as well as the aforementioned enum conversions. Sounds good >> Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:436 >> + keyframePayload->setEasing(timingFunction->cssText()); > > I see, it's a CSS timing function (which can be user defined, right?) Yes, it is user defined. >> Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:441 >> + for (auto cssPropertyId : cssPropertyIds) { > > Needs a comment: "The frontend expects a CSS rule body per each keyframe. Synthesize CSS style property text for each property of the keyframe." Good call.

Devin Rousso

Comment 14 2019-12-21 22:39:01 PST

Created attachment 386313 [details] Patch Addressed Brian's feedback.

Blaze Burg

Comment 15 2019-12-27 08:34:07 PST

Comment on attachment 386313 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386313&action=review > Source/WebInspectorUI/UserInterface/Base/Main.js:178 > + let canvasIndex = WI._openTabsSetting.value.indexOf("canvas"); Nit: please add an expiration date for this code (Summer 2020?) > Source/WebInspectorUI/UserInterface/Controllers/AnimationManager.js:2 > + * Copyright (C) 2017 Apple Inc. All rights reserved. Nit: 2019 > Source/WebInspectorUI/UserInterface/Controllers/AnimationManager.js:26 > +// FIXME: AnimationManager lacks advanced multi-target support. (Animations per-target) Here's the problem with renaming the protocol method to Animation.requestTarget: I can't tell whether this comment refers to multiple effect targets or multiple Page targets. Calling it something more verbose like .requestEffectTarget may help reduce this semantic overloading. > Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:326 > + return this._idToDOMNode[nodeId] || null; Okay, but do other callers rely on this returning undefined if the lookup fails? > Source/WebInspectorUI/UserInterface/Images/Graphics.svg:2 > + Nit: 2019 > Source/WebInspectorUI/UserInterface/Main.html:355 > + <script src="Models/Collection.js"></script> Nit: ordering is wrong > Source/WebInspectorUI/UserInterface/Models/Animation.js:2 > + * Copyright (C) 2017 Apple Inc. All rights reserved. Ditto. > Source/WebInspectorUI/UserInterface/Models/Animation.js:185 > + requestTarget() Oh boy 😵

Devin Rousso

Comment 16 2019-12-29 12:37:47 PST

Comment on attachment 386313 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386313&action=review >> Source/WebInspectorUI/UserInterface/Base/Main.js:178 >> + let canvasIndex = WI._openTabsSetting.value.indexOf("canvas"); > > Nit: please add an expiration date for this code (Summer 2020?) I think we should keep this for at least a full release, as without this the Canvas Tab could seem to disappear with no replacement for users who haven't used Web Inspector in a while. I'll make this into a FIXME comment with a bug link. >> Source/WebInspectorUI/UserInterface/Controllers/AnimationManager.js:2 >> + * Copyright (C) 2017 Apple Inc. All rights reserved. > > Nit: 2019 Oops :P >> Source/WebInspectorUI/UserInterface/Controllers/AnimationManager.js:26 >> +// FIXME: AnimationManager lacks advanced multi-target support. (Animations per-target) > > Here's the problem with renaming the protocol method to Animation.requestTarget: I can't tell whether this comment refers to multiple effect targets or multiple Page targets. Calling it something more verbose like .requestEffectTarget may help reduce this semantic overloading. Good point! `Animation.requestEffectTarget` makes the most sense and is the most technically accurate too. >> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:326 >> + return this._idToDOMNode[nodeId] || null; > > Okay, but do other callers rely on this returning undefined if the lookup fails? Not that I could tell. >> Source/WebInspectorUI/UserInterface/Images/Graphics.svg:2 >> + > > Nit: 2019 AFAIU, we only update copyright when making major modifications. Since this is just a rename, I didn't think it warranted an update, but I don't really care either way so I'll update it. >> Source/WebInspectorUI/UserInterface/Main.html:355 >> + <script src="Models/Collection.js"></script> > > Nit: ordering is wrong Oops :P >> Source/WebInspectorUI/UserInterface/Models/Animation.js:2 >> + * Copyright (C) 2017 Apple Inc. All rights reserved. > > Ditto. Oops :P >> Source/WebInspectorUI/UserInterface/Models/Animation.js:185 >> + requestTarget() > > Oh boy 😵 Yeah I see how this could be confusing, especially if we added a `get target()` 😅

Devin Rousso

Comment 17 2019-12-29 12:48:10 PST

Created attachment 386496 [details] Patch

Devin Rousso

Comment 18 2019-12-31 13:17:05 PST

(In reply to Brian Burg from comment #11) > Windows build failure: > > C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\inspector\agents\InspectorAnimationAgent.h(63,10): error C3668: 'WebCore::InspectorAnimationAgent::resolveAnimation': method with override specifier 'override' did not override any base class methods (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource-84c9f43f-6.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj] I have no idea why this is happening. It builds just fine on all other platforms. Perhaps there is something funky with the generated output of InspectorBackendDispatchers.h? Is there any way for me to see the generated output file? > Mac WK1 test failures: > > Regressions: Unexpected text-only failures (2) > inspector/animation/lifecycle-css-animation.html [ Failure ] > inspector/animation/lifecycle-css-transition.html [ Failure ] Apparently, Web Animations are not used for CSS animations/transitions in WK1. As such, I've skipped these tests for mac-wk1.

Antoine Quint

Comment 19 2020-01-06 02:36:05 PST

(In reply to Devin Rousso from comment #18) > (In reply to Brian Burg from comment #11) > Apparently, Web Animations are not used for CSS animations/transitions in > WK1. As such, I've skipped these tests for mac-wk1. That shouldn't be the case! I filed https://bugs.webkit.org/show_bug.cgi?id=205791 to address that.

Antoine Quint

Comment 20 2020-01-06 11:58:42 PST

(In reply to Antoine Quint from comment #19) > (In reply to Devin Rousso from comment #18) > > (In reply to Brian Burg from comment #11) > > Apparently, Web Animations are not used for CSS animations/transitions in > > WK1. As such, I've skipped these tests for mac-wk1. > > That shouldn't be the case! I filed > https://bugs.webkit.org/show_bug.cgi?id=205791 to address that. And now it's fixed, so you shouldn't have to skip your tests on WK1 anymore.

Devin Rousso

Comment 21 2020-01-06 13:01:54 PST

Created attachment 386879 [details] Patch

Blaze Burg

Comment 22 2020-01-10 11:43:13 PST

Comment on attachment 386879 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386879&action=review > Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:303 > + this._canvasCollection.clear(); I really like this refactor. It did seem kind of strange for the manager to not have a collection of canvas instances and it was being handled by the tab view class. > Source/WebInspectorUI/UserInterface/Models/Animation.js:75 > + case WI.Animation.PlaybackDirection.Normal: I think these should have the optional description to say they are animation playback directions. > Source/WebInspectorUI/UserInterface/Models/Animation.js:92 > + case WI.Animation.FillMode.None: I think these should have the optional description to say they are animation fill modes. > Source/WebInspectorUI/UserInterface/Models/Animation.js:187 > + if (!this._requestEffectTargetPromise) { As we discussed its a little weird to have an infallible promise. That said, I don't think that there is anything to do in case the AnimationAgent.requestEffectTarget command fails. > Source/WebInspectorUI/UserInterface/Models/Canvas.js:100 > + return WI.UIString("Bitmap Renderer"); Definitely needs a comment for localization. "Bitmap Renderer is a type of context associated with a <canvas> element" or something. > Source/WebInspectorUI/UserInterface/Models/Recording.js:156 > + case Recording.Type.CanvasBitmapRenderer: Ditto the localization comment. "Bitmap Renderer (Recording Type)", "Bitmap Renderer", "A type of canvas recording in the Graphics Tab." > Source/WebInspectorUI/UserInterface/Views/AnimationCollectionContentView.js:61 > + handleRefreshButtonClicked() Per our discussion, this should go away and instead invoke the normal WI.View needsLayout. This isn't really performance sensitive as it only draws once in response to user input. > Source/WebInspectorUI/UserInterface/Views/AnimationCollectionContentView.js:100 > + if (!node || !node.ownerDocument) Have you checked whether this mouse-driven lookup feels responsive enough when inspecting a device? > Source/WebInspectorUI/UserInterface/Views/AnimationContentView.css:59 > + content: "\00A0\2014\00A0"; /*  —  */; Fancy. Does it look correct in RTL layout? We try to do something similar with Source Locations but it needed extra care for RTL. > Source/WebInspectorUI/UserInterface/Views/AnimationContentView.css:68 > + opacity: 1; Is this needed given that it also has a huge focus ring when selected? I would expect the background to change on hover but not the border. Not a big deal either way. > Source/WebInspectorUI/UserInterface/Views/AnimationContentView.js:2 > + * Copyright (C) 2017 Apple Inc. All rights reserved. 2020, please > Source/WebInspectorUI/UserInterface/Views/AnimationContentView.js:160 > + is squeezeYStart related to the markerHead size? I would have guessed squeezeYStart = markerHeadRadius + markerHeadPadding*2 > Source/WebInspectorUI/UserInterface/Views/AnimationContentView.js:162 > + const adjustEasingY = 0.5; This looks good. > Source/WebInspectorUI/UserInterface/Views/AnimationContentView.js:191 > + addTitle(startDelayElement, WI.UIString("Start Delay %s").format(Number.secondsToString(this.representedObject.startDelay / 1000))); Localizers will need help to find out what this refers to. > Source/WebInspectorUI/UserInterface/Views/AnimationContentView.js:208 > + addTitle(endDelayElement, WI.UIString("End Delay %s").format(Number.secondsToString(this.representedObject.endDelay / 1000))); Ditto. > Source/WebInspectorUI/UserInterface/Views/AnimationContentView.js:239 > + easingPath.setAttribute("d", `M 0 ${height + adjustEasingY} C ${x1} ${y1} ${x2} ${y2} ${width} ${adjustEasingY} V ${height + adjustEasingY} Z`); 🥵 > Source/WebInspectorUI/UserInterface/Views/AnimationContentView.js:279 > + this._refreshSubtitle(); I can't think of any reason these need to be different than calling .needsLayout(), and it may cut down the number of relayouts if we load a bunch of animations quickly. > Source/WebInspectorUI/UserInterface/Views/AnimationContentView.js:289 > + const text = WI.UIString("Selected Animation"); Nit: add localizer note that this is a label for an animation that's logged to console.

Blaze Burg

Comment 23 2020-01-10 12:51:05 PST

Comment on attachment 386879 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386879&action=review r=me, excellent work. Please address smaller items immediately such as localizer comments, other changes such as moving to .needsLayout should come in a followup to minimize breakage. > Source/WebInspectorUI/UserInterface/Views/AnimationDetailsSidebarPanel.js:40 > + { What happens in the case of multiple animations being selected? Is that not supported? > Source/WebInspectorUI/UserInterface/Views/AnimationDetailsSidebarPanel.js:82 > + this._targetRow = new WI.DetailsSectionSimpleRow(WI.UIString("Target")); Needs a better localizer comment. > Source/WebInspectorUI/UserInterface/Views/AnimationDetailsSidebarPanel.js:83 > + let identitySection = new WI.DetailsSection("animation-identity", WI.UIString("Identity"), [new WI.DetailsSectionGroup([this._nameRow, this._typeRow, this._targetRow])]); Needs a better localizer comment. > Source/WebInspectorUI/UserInterface/Views/AnimationDetailsSidebarPanel.js:89 > + let iterationsGroup = new WI.DetailsSectionGroup([this._iterationCountRow, this._iterationStartRow, this._iterationDurationRow]); (throughout) needs a better localizer comment. > Source/WebInspectorUI/UserInterface/Views/AnimationDetailsSidebarPanel.js:112 > + let backtraceRow = new WI.DetailsSectionRow; This looks weird in the screenshot. Is each frame clickable? I would have expected more SourceLocation information (file, line number) especially if the frames are not selectable. > Source/WebInspectorUI/UserInterface/Views/AnimationDetailsSidebarPanel.js:142 > + this._animation.requestEffectTarget().then((domNode) => { Is this a good idea under layout()? Seems it will cause another repaint sometime later even if we have the data. I think it would be best to cache the effect inside set animation() or in response to the effecteUpdated event and make this function synchronous. > Source/WebInspectorUI/UserInterface/Views/AnimationDetailsSidebarPanel.js:163 > + for (let section of this._codeMirrorSectionMap.keys()) I think it would be clearer to clean up the old elements we don't want at the top of the function. > Source/WebInspectorUI/UserInterface/Views/AnimationDetailsSidebarPanel.js:178 > + subtitle.textContent = ` ${emDash} ${keyframe.easing.toString()}`; Does this look right in RTL layout? > Source/WebInspectorUI/UserInterface/Views/AnimationDetailsSidebarPanel.js:202 > + createCodeMirrorCubicBezierTextMarkers(codeMirror, range, optionsForType(WI.InlineSwatch.Type.Bezier)); It's kind of a bummer this won't be shown for the easing function since that's in the header. I don't think it needs to be there other than to save space. What about prepending the easing property name+value to the keyframe's style text? > Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.js:86 > + handleRefreshButtonClicked() Per previous comment this can just trigger a new layout directly if it's not going to fetch new data. > Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:98 > + if (item) { Was this just a latent bug or are we now expecting to pass null to set selectedItem()? > Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:100 > + console.assert(contentView, "Missing contet view for item.", item); Nit: content view > Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:304 > + this._selectItem(null); I see, now we deselect the item if the hit test failed. > Source/WebInspectorUI/UserInterface/Views/GraphicsOverviewContentView.js:61 > + if (!WI.animationManager.supported) { I don't understand. Don't we always want these canvas related navigation items? Or is this supposed to be items from the animation overview content view? EDIT: Oh, I see, if Animations are not supported then there are no sub-sections in the main content area and [ ] Record First [n] Frames needs to be added to the main bar in that case.

Devin Rousso

Comment 24 2020-01-27 15:07:44 PST

Comment on attachment 386879 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386879&action=review >> Source/WebInspectorUI/UserInterface/Views/AnimationCollectionContentView.js:100 >> + if (!node || !node.ownerDocument) > > Have you checked whether this mouse-driven lookup feels responsive enough when inspecting a device? It's never been a problem with canvases, and they can potentially highlight multiple nodes at once (e.g. `-webkit-canvas` CSS canvas clients). This is effectively the same logic. >> Source/WebInspectorUI/UserInterface/Views/AnimationContentView.css:59 >> + content: "\00A0\2014\00A0"; /*  —  */; > > Fancy. Does it look correct in RTL layout? We try to do something similar with Source Locations but it needed extra care for RTL. I tested it using our debug setting, but not changing the system language. It looked fine with our debug setting. >> Source/WebInspectorUI/UserInterface/Views/AnimationContentView.css:68 >> + opacity: 1; > > Is this needed given that it also has a huge focus ring when selected? I would expect the background to change on hover but not the border. Not a big deal either way. This matches what we do with the navigation bar in the canvas cards. >> Source/WebInspectorUI/UserInterface/Views/AnimationContentView.js:160 >> + > > is squeezeYStart related to the markerHead size? I would have guessed squeezeYStart = markerHeadRadius + markerHeadPadding*2 Oh lol good point 😂 >> Source/WebInspectorUI/UserInterface/Views/AnimationDetailsSidebarPanel.js:40 >> + { > > What happens in the case of multiple animations being selected? Is that not supported? No, that is not supported. This is just our usual logic for details sidebars ¯\_(ツ)_/¯ >> Source/WebInspectorUI/UserInterface/Views/AnimationDetailsSidebarPanel.js:112 >> + let backtraceRow = new WI.DetailsSectionRow; > > This looks weird in the screenshot. Is each frame clickable? I would have expected more SourceLocation information (file, line number) especially if the frames are not selectable. Yes, they are all clickable. The screenshot is a bad example (sorry) because it was created by a Console evaluation (also in the screenshot), not from some script. >> Source/WebInspectorUI/UserInterface/Views/AnimationDetailsSidebarPanel.js:142 >> + this._animation.requestEffectTarget().then((domNode) => { > > Is this a good idea under layout()? Seems it will cause another repaint sometime later even if we have the data. I think it would be best to cache the effect inside set animation() or in response to the effecteUpdated event and make this function synchronous. The main reason why we use the `Promise` approach is that something in the UI has to be the first caller, as the effect target is never sent to the frontend. I don't want to eagerly fetch the effect target, as we may not need that info (such as if the Graphics Tab isn't even visible). A better solution might be to not use a `Promise`, and instead take a callback function directly so that it can be called synchronously if the value is already there. That way, the majority of calls won't trigger another layout, but some still might instead of all. >> Source/WebInspectorUI/UserInterface/Views/AnimationDetailsSidebarPanel.js:178 >> + subtitle.textContent = ` ${emDash} ${keyframe.easing.toString()}`; > > Does this look right in RTL layout? It looks fine to me using the debug setting. >> Source/WebInspectorUI/UserInterface/Views/AnimationDetailsSidebarPanel.js:202 >> + createCodeMirrorCubicBezierTextMarkers(codeMirror, range, optionsForType(WI.InlineSwatch.Type.Bezier)); > > It's kind of a bummer this won't be shown for the easing function since that's in the header. I don't think it needs to be there other than to save space. What about prepending the easing property name+value to the keyframe's style text? That could be confusing, as the style text could itself contain a `transition-timing-function` or `animation-timing-function` property. I put it in the header not primarily as a way to save space, but more to avoid this confusion. >> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:98 >> + if (item) { > > Was this just a latent bug or are we now expecting to pass null to set selectedItem()? Yes, we can now pass `null` as a way to deselect. >> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:304 >> + this._selectItem(null); > > I see, now we deselect the item if the hit test failed. Yup! >> Source/WebInspectorUI/UserInterface/Views/GraphicsOverviewContentView.js:61 >> + if (!WI.animationManager.supported) { > > I don't understand. Don't we always want these canvas related navigation items? Or is this supposed to be items from the animation overview content view? > > EDIT: Oh, I see, if Animations are not supported then there are no sub-sections in the main content area and [ ] Record First [n] Frames needs to be added to the main bar in that case. Yup!

Devin Rousso

Comment 25 2020-01-27 15:10:14 PST

Created attachment 388923 [details] Patch

Devin Rousso

Comment 26 2020-01-28 10:22:36 PST

Created attachment 389028 [details] Patch Add debug logging

Devin Rousso

Comment 27 2020-01-28 11:59:05 PST

Created attachment 389047 [details] Patch

Devin Rousso

Comment 28 2020-01-28 12:26:53 PST

Created attachment 389049 [details] Patch Rebase

Devin Rousso

Comment 29 2020-01-28 23:20:27 PST

Created attachment 389108 [details] [Patch] test modifying JavaScriptCore/inspector/scripts for win Modify 'Source/JavaScriptCore/inspector/scripts/*' so that it generates a fresh build

Devin Rousso

Comment 30 2020-01-29 09:53:47 PST

Created attachment 389152 [details] [Patch] test modifying WTF/wtf/Assertions.h for win

Devin Rousso

Comment 31 2020-01-29 12:01:14 PST

Comment on attachment 389049 [details] Patch My suspicion is that the windows build failure can be solved by a clean build, which isn't possible to do via bugzilla. The build failure suggests that 'DerivedSources/JavaScript/inspector/InspectorProtocolObjects.h' is not being regenerated correctly, which is why only _some_ of the `override` functions added to `WebCore::InspectorAnimationAgent` are erroring (e.g. `startTracking` vs `resolveAnimation`), as well as why only _some_ values used from `Inspector::Protocol::Animation` are erroring (e.g. `Inspector::Protocol::Animation::AnimationState` vs `Inspector::Protocol::Animation::FillMode`).

Devin Rousso

Comment 32 2020-01-29 12:01:51 PST

Comment on attachment 389049 [details] Patch Oh wait oops there's a test expectation to update now that <https://webkit.org/b/206899> landed 😅

Devin Rousso

Comment 33 2020-01-29 12:17:01 PST

Created attachment 389170 [details] Patch

WebKit Commit Bot

Comment 34 2020-01-29 14:22:56 PST Comment hidden (obsolete)

Comment on attachment 389170 [details] Patch Rejecting attachment 389170 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 389170, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 5000 characters of output: ews/CanvasSidebarPanel.js patching file Source/WebInspectorUI/UserInterface/Views/CanvasTabContentView.css rm 'Source/WebInspectorUI/UserInterface/Views/CanvasTabContentView.css' patching file Source/WebInspectorUI/UserInterface/Views/CanvasTabContentView.js rm 'Source/WebInspectorUI/UserInterface/Views/CanvasTabContentView.js' patching file Source/WebInspectorUI/UserInterface/Views/CollectionContentView.css patching file Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js patching file Source/WebInspectorUI/UserInterface/Views/ContentView.js patching file Source/WebInspectorUI/UserInterface/Views/DetailsSectionSimpleRow.js patching file Source/WebInspectorUI/UserInterface/Views/GraphicsOverviewContentView.css patching file Source/WebInspectorUI/UserInterface/Views/GraphicsOverviewContentView.js patching file Source/WebInspectorUI/UserInterface/Views/GraphicsTabContentView.css patching file Source/WebInspectorUI/UserInterface/Views/GraphicsTabContentView.js patching file Source/WebInspectorUI/UserInterface/Views/Main.css patching file Source/WebInspectorUI/UserInterface/Views/RecordingContentView.css patching file Source/WebInspectorUI/UserInterface/Views/ResourceCollectionContentView.css patching file Source/WebInspectorUI/UserInterface/Views/ResourceCollectionContentView.js patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/inspector/animation/effectChanged-expected.txt patching file LayoutTests/inspector/animation/effectChanged.html patching file LayoutTests/inspector/animation/lifecycle-css-animation-expected.txt patching file LayoutTests/inspector/animation/lifecycle-css-animation.html patching file LayoutTests/inspector/animation/lifecycle-css-transition-expected.txt patching file LayoutTests/inspector/animation/lifecycle-css-transition.html patching file LayoutTests/inspector/animation/lifecycle-web-animation-expected.txt patching file LayoutTests/inspector/animation/lifecycle-web-animation.html patching file LayoutTests/inspector/animation/resolveAnimation-expected.txt patching file LayoutTests/inspector/animation/resolveAnimation.html patching file LayoutTests/inspector/animation/resources/lifecycle-utilities.js patching file LayoutTests/inspector/animation/targetChanged-expected.txt patching file LayoutTests/inspector/animation/targetChanged.html patching file LayoutTests/inspector/canvas/context-attributes.html patching file LayoutTests/inspector/canvas/create-context-webgpu.html patching file LayoutTests/inspector/canvas/extensions.html patching file LayoutTests/inspector/canvas/memory.html patching file LayoutTests/inspector/canvas/recording.html patching file LayoutTests/inspector/canvas/requestClientNodes.html patching file LayoutTests/inspector/canvas/requestContent-2d.html patching file LayoutTests/inspector/canvas/requestContent-bitmaprenderer.html patching file LayoutTests/inspector/canvas/requestContent-webgl.html patching file LayoutTests/inspector/canvas/requestContent-webgl2.html patching file LayoutTests/inspector/canvas/requestNode.html patching file LayoutTests/inspector/canvas/resolveContext-2d.html patching file LayoutTests/inspector/canvas/resolveContext-bitmaprenderer.html patching file LayoutTests/inspector/canvas/resolveContext-webgl.html patching file LayoutTests/inspector/canvas/resolveContext-webgl2.html patching file LayoutTests/inspector/canvas/resolveContext-webgpu.html patching file LayoutTests/inspector/canvas/resources/create-context-utilities.js patching file LayoutTests/inspector/canvas/resources/recording-utilities.js patching file LayoutTests/inspector/canvas/resources/shaderProgram-utilities-webgl.js patching file LayoutTests/inspector/canvas/resources/shaderProgram-utilities-webgpu.js patching file LayoutTests/inspector/canvas/setRecordingAutoCaptureFrameCount.html patching file LayoutTests/inspector/canvas/shaderProgram-add-remove-webgpu.html patching file LayoutTests/inspector/canvas/updateShader-webgpu-sharedVertexFragment.html patching file LayoutTests/inspector/unit-tests/test-harness-expect-functions-expected.txt patching file LayoutTests/inspector/unit-tests/test-harness-expect-functions.html Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Updating OpenSource From git://git.webkit.org/WebKit e110b9e0de2..70a545afe2e master -> origin/master Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ... Currently at 255380 = e110b9e0de226ce9b36848479e11e6aa48a27847 r255381 = c500666fd0e34bf281fc4b2df948ba89bf9c2e84 r255382 = e3f7f0f52a0ff209edb9ff506247ecc6e27917be r255383 = 0071f13e56d8e5b774c7a9bbb2d09f3d16fedbbc r255384 = 70a545afe2e24be94b9af1d18bf1b77b9a6c76eb Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/origin/master. Full output: https://webkit-queues.webkit.org/results/13313584

Devin Rousso

Comment 35 2020-01-29 14:34:34 PST

Created attachment 389188 [details] Patch Rebase

WebKit Commit Bot

Comment 36 2020-01-29 15:46:22 PST

Comment on attachment 389188 [details] Patch Clearing flags on attachment: 389188 Committed r255396: <https://trac.webkit.org/changeset/255396>

WebKit Commit Bot

Comment 37 2020-01-29 15:46:25 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.