With the advent of the Web Animations API, CSS animations/transitions now follow that timeline. Unlike all other forms of Web Animations, however, they are _not_ created by JavaScript, and therefore can seemingly appear out of nowhere. We should add some instrumentation for the Timelines Tab to show information about when CSS animations/transitions are created, started, delayed, iterated, canceled, or finished.
<rdar://problem/56128726>
Created attachment 382403 [details] Patch
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Created attachment 382404 [details] [Image] After Patch is applied (CSS animation/transition)
Created attachment 382405 [details] [Image] After Patch is applied (<video>)
Some quick comments: • The blue circles over the green should have a bg-color outline also, so the colors don’t touch. • The circles icons should coalesce, with the most important one on top. Maybe with a special stack icon background to show there are multiple ones, where there is not enough timescale between them. • The summary in the top overview seems to not match the bottom.
Created attachment 382419 [details] Patch Add white border around icons and fix the summary in the overview.
Created attachment 382424 [details] [Image] After Patch is applied (CSS animation/transition)
Created attachment 382425 [details] Patch Ensure that playing videos are considered unfinished.
Comment on attachment 382425 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382425&action=review I didn't really review the JS part, just the C++ integration with the WebCore Web Animations bits. > Source/WTF/wtf/Markable.h:149 > +template <typename T, typename Traits> constexpr bool operator==(const Markable<T, Traits>& x, const Markable<T, Traits>& y) { return bool(x) != bool(y) ? false : bool(x) == false ? true : *x == *y; } I didn't see anything about nested ternary expressions, but I personally find them hard to read. I would recommend either putting the nested expression between parentheses or using if statements to make it even clearer. > Source/WebCore/animation/KeyframeEffect.cpp:1020 > + setAnimatedPropertiesInStyle(targetStyle, computedTiming.progress.value()); Might as well change here to `*computedTiming.progress`. > Source/WebCore/animation/WebAnimation.cpp:86 > + InspectorInstrumentation::willDestroyWebAnimation(*this); For my own enlightenment, why is this required when we have the call in the destructor? > Source/WebCore/inspector/TimelineRecordFactory.cpp:39 > +#include "TransitionEvent.h" Why are those needed? > Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:105 > + return computedTiming.localTime.value() < (computedTiming.endTime - computedTiming.activeDuration); I think you can use computedTiming.phase here to figure that out, this would match the "before" phase. > Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:170 > + trackingData.lastComputedTiming = computedTiming; Don't we want to track this also when animationProgress isn't resolved? > Source/WebCore/inspector/agents/InspectorTimelineAgent.h:80 > + NICE. > Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:763 > + let mediaTimeline = this._activeRecording.timelineForRecordType(WI.TimelineRecord.Type.Media); This could be `const`. There are other places where const can be used, I guess maybe you don't use it much in the Web Inspector code?
Comment on attachment 382425 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382425&action=review >> Source/WTF/wtf/Markable.h:149 >> +template <typename T, typename Traits> constexpr bool operator==(const Markable<T, Traits>& x, const Markable<T, Traits>& y) { return bool(x) != bool(y) ? false : bool(x) == false ? true : *x == *y; } > > I didn't see anything about nested ternary expressions, but I personally find them hard to read. I would recommend either putting the nested expression between parentheses or using if statements to make it even clearer. This was copied directly from `WTF::Optional`. I'm happy to make changes to this so it conforms to WebKit style, but I wasn't sure whether we'd want code similarity since `Markable` is supposed to basically be a more space efficient `Optional`. >> Source/WebCore/animation/KeyframeEffect.cpp:1020 >> + setAnimatedPropertiesInStyle(targetStyle, computedTiming.progress.value()); > > Might as well change here to `*computedTiming.progress`. I personally prefer using `.value()` for `Markable`/`Optional` instead of `*`, so that I know that it's not a pointer. >> Source/WebCore/animation/WebAnimation.cpp:86 >> + InspectorInstrumentation::willDestroyWebAnimation(*this); > > For my own enlightenment, why is this required when we have the call in the destructor? If the associated `ScriptExecutionContext` is destroyed before this object is destructed, we want to notify Web Inspector, as we need the `ScriptExecutionContext` in order to get to the right agent (via `InspectorInstrumentation` plumbing). I'm not entirely sure if this is "necessary", but it's a good precaution that can help avoid UAF in Web Inspector. >> Source/WebCore/inspector/TimelineRecordFactory.cpp:39 >> +#include "TransitionEvent.h" > > Why are those needed? Oops. Leftovers from an earlier design 😅. Will remove. >> Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:105 >> + return computedTiming.localTime.value() < (computedTiming.endTime - computedTiming.activeDuration); > > I think you can use computedTiming.phase here to figure that out, this would match the "before" phase. From what I could tell in my testing, the time during an `animation-delay`/`transition-delay` still showed up during the "active" phase, and the "before" phase only happened for the briefest of moments between the animation being created and it being first applied. >> Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:170 >> + trackingData.lastComputedTiming = computedTiming; > > Don't we want to track this also when animationProgress isn't resolved? Ooooo, nice catch! >> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:763 >> + let mediaTimeline = this._activeRecording.timelineForRecordType(WI.TimelineRecord.Type.Media); > > This could be `const`. There are other places where const can be used, I guess maybe you don't use it much in the Web Inspector code? Yeah, the sorta informal Web Inspector rule is that we only use `const` for constant values that don't change between invocations of Web Inspector.
Comment on attachment 382425 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382425&action=review Overall the approach looks good. There are some opportunities to reduce the size / risk of the patch by splitting async toJSON/fromJSON refactoring from the rest. > Source/WebInspectorUI/ChangeLog:204 > + Make the importing of timeline records `async` so we can attempt to rehydrate the DOM nodes Can you make this change separately in trunk in case it causes problems? > Source/JavaScriptCore/inspector/protocol/Animation.json:13 > + "id": "AnimationProgress", What about renaming to AnimationState? > Source/JavaScriptCore/inspector/protocol/Animation.json:25 > + { "name": "transitionProperty", "type": "string", "optional": true } It's worth clarifying when these properties are expected to be used or not. I'm assuming that Web Animations would have a different set of properties than CSS animations and transitions. Do you know whether this implementation is suitable for Web Animations, or will that require additional refactoring? > Source/JavaScriptCore/inspector/protocol/Animation.json:41 > + "name": "trackingStart", Please explain in the description for this event the circumstances in which this event could be produced. Is it only caused as a result of Animation.startTracking? I should be able to tell by reading the JSON rather than the implementation. Same comment for trackingComplete. > Source/JavaScriptCore/inspector/protocol/Animation.json:48 > + "description": "Fired for each phase of a declarative (CSS) animation.", If this is reused for Web Animations, then this comment is going to be outdated soon. >> Source/WTF/wtf/Markable.h:149 >> +template <typename T, typename Traits> constexpr bool operator==(const Markable<T, Traits>& x, const Markable<T, Traits>& y) { return bool(x) != bool(y) ? false : bool(x) == false ? true : *x == *y; } > > I didn't see anything about nested ternary expressions, but I personally find them hard to read. I would recommend either putting the nested expression between parentheses or using if statements to make it even clearer. +1, I have no idea what this is doing without rewriting it. > Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:84 > +localizedStrings["Active"] = "Active"; Comment for the entire file: Please add descriptions for the keys, so a localizer will know how to see "Active" translated in context. > Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:351 > this._recordings.push(newRecording); Is this change related to the rest of the patch? > Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:780 > + console.assert(); Add a message, like `Unknown event type: ${event}` > Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:1389 > + if (!domNode.isMediaElement()) Is this a behavior change? > Source/WebInspectorUI/UserInterface/Models/DOMNode.js:154 > + if (this.isMediaElement()) I'm not sure this change buys us anything. What if we want to listen to other active DOM elements like form elements? > Source/WebInspectorUI/UserInterface/Models/HeapAllocationsTimelineRecord.js:-49 > - let workerProxy = WI.HeapSnapshotWorkerProxy.singleton(); Ditto comments as above, this can be split into a separate patch and landed sooner. > Source/WebInspectorUI/UserInterface/Models/MediaInstrument.js:54 > + if (!initiatedByBackend) { Please flip the outer conditional into an early return. No need to nest this. > Source/WebInspectorUI/UserInterface/Models/MediaTimeline.js:32 > + return this._trackingAnimationIdRecordMap.get(trackingAnimationId) || null; As I'm working my way into the second half of the patch, the 'tracking' prefix is starting to become annoying. I think animationIdRecordMap (and similar) would be clear enough. > Source/WebInspectorUI/UserInterface/Models/MediaTimelineRecord.js:104 > + domNodeCSSPath: this._domNode instanceof WI.DOMNode ? WI.cssPath(this._domNode, {full: true}) : this._domNode.cssPath, This line is very confusing. If it's a DOMNode, *don't* use the DOMNode implementation of cssPath. If it is not a DOMNode, use the DOMNode implementation of cssPath. Perhaps both cases can be folded into WI.cssPath > Source/WebInspectorUI/UserInterface/Models/MediaTimelineRecord.js:117 > + item.originatorCSSPath = item.originator instanceof WI.DOMNode ? WI.cssPath(item.originator, {full: true}) : item.originator.cssPath; DItto here. > Source/WebInspectorUI/UserInterface/Views/DOMTreeElementPathComponent.js:88 > + static iconClassNameForNode(domNode) This is a nice cleanup, but it doesn't seem related to the rest of the patch. Please split it out. > Source/WebInspectorUI/UserInterface/Views/MediaTimelineDataGridNode.js:45 > + this._cachedData.source = this.record.domNode; // Timeline Overview Huh? > LayoutTests/inspector/animation/tracking-expected.txt:10 > +PASS: Should have an event object. Eventually there should be test cases for weirder animations, like those that start and stop or repeat.
Comment on attachment 382425 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382425&action=review >> Source/WebInspectorUI/ChangeLog:204 >> + Make the importing of timeline records `async` so we can attempt to rehydrate the DOM nodes > > Can you make this change separately in trunk in case it causes problems? I'd prefer to keep this in the same patch, as media records are basically useless without some information about the element that they're related to. If we don't have the rehydration step, we wouldn't have any useful information to show, and that makes any exported media recordings basically useless. >> Source/JavaScriptCore/inspector/protocol/Animation.json:13 >> + "id": "AnimationProgress", > > What about renaming to AnimationState? I like it! >> Source/JavaScriptCore/inspector/protocol/Animation.json:25 >> + { "name": "transitionProperty", "type": "string", "optional": true } > > It's worth clarifying when these properties are expected to be used or not. I'm assuming that Web Animations would have a different set of properties than CSS animations and transitions. > > Do you know whether this implementation is suitable for Web Animations, or will that require additional refactoring? This should also be suitable for Web Animations, in that _neither_ `animationName` nor `transitionProperty` would be provided. >> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:1389 >> + if (!domNode.isMediaElement()) > > Is this a behavior change? Not entirely. The only reason we wanted to do this before was so that if an ancestor of a media element goes full screen, we'd get notified about it and include a record for it. Previously, we would actually generate two records, one for the ancestor and one for the media element (which we synthesized in the frontend). Now we only have one record, and adjust the title of the full screen segment to reflect the originator of the full screen, rather than have two records. >> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:154 >> + if (this.isMediaElement()) > > I'm not sure this change buys us anything. What if we want to listen to other active DOM elements like form elements? Then we can add an `|| this.isFormElement()`. Given that `_shouldListenForEventListeners` was only ever called once, and how we want to expose `isMediaElement()` for other usage, I think this is fine. We can cross that bridge when/if we come to it. >> Source/WebInspectorUI/UserInterface/Models/MediaInstrument.js:54 >> + if (!initiatedByBackend) { > > Please flip the outer conditional into an early return. No need to nest this. This is the style that the rest of the instruments use. I'd rather be consistent (same logic as `initializeTarget` in most of the managers). >> Source/WebInspectorUI/UserInterface/Models/MediaTimeline.js:32 >> + return this._trackingAnimationIdRecordMap.get(trackingAnimationId) || null; > > As I'm working my way into the second half of the patch, the 'tracking' prefix is starting to become annoying. I think animationIdRecordMap (and similar) would be clear enough. The reason I want to do this is because I'm envisioning the Animation domain having two completely separate areas: - tracking, which is used exclusively for timeline recordings - enabled, which would be for non-recording related things (e.g. adjust global playback speed, or restart a particular animation) As such, we'd want different identifiers for each area so that they can be independently turned on/off (e.g. we should be able to disable the "enabled" area when the Canvas/Graphics tab is closed (which is what Canvas does now), meaning that we need a separate list of IDs so that disabling the Animation domain doesn't nuke all of our previously established state). I'd prefer to keep the word "tracking" in the name as it makes it clear what situation these values will exist (e.g. only during a timeline recording). >> Source/WebInspectorUI/UserInterface/Models/MediaTimelineRecord.js:104 >> + domNodeCSSPath: this._domNode instanceof WI.DOMNode ? WI.cssPath(this._domNode, {full: true}) : this._domNode.cssPath, > > This line is very confusing. If it's a DOMNode, *don't* use the DOMNode implementation of cssPath. If it is not a DOMNode, use the DOMNode implementation of cssPath. Perhaps both cases can be folded into WI.cssPath So in the "else" of the ternary, `_domNode` is actually just a POD object that has a `displayName` and `cssPath` property. During rehydration, if we aren't able to find a node that matches the `cssPath` we'd previously exported, we fall back to just using the string value and "carrying it forward" if we re-export. The same is true for the `displayName`, as if we can't find the node during rehydration, we don't have anything to create a name from. Actually, we should probably capture the `cssPath` as soon as the record is created so that if the node moves around, we don't lose its original position. >> Source/WebInspectorUI/UserInterface/Views/DOMTreeElementPathComponent.js:88 >> + static iconClassNameForNode(domNode) > > This is a nice cleanup, but it doesn't seem related to the rest of the patch. Please split it out. It is related, as this is how we get the icons for the various DOMNode types in the element/source column of the various data grids. >> Source/WebInspectorUI/UserInterface/Views/MediaTimelineDataGridNode.js:45 >> + this._cachedData.source = this.record.domNode; // Timeline Overview > > Huh? Since the overview mixes together a bunch of different record types, we only show a sort of "least common denominator" of columns (name, source, graph). In that view, rather than show a source code location, the "source" of the media event is actually the DOM node the record is associated with, so we show that instead.
Created attachment 382525 [details] Patch
Created attachment 382544 [details] Patch Rebase
Created attachment 382559 [details] Patch Fix tests after "progress" => "state" rename.
Comment on attachment 382559 [details] Patch Thank you for the clarifications, I am satisfied for this patch. r+
Created attachment 382615 [details] Patch Fix API test
Created attachment 382617 [details] Patch Rebase, as well as remvoving some unnecessary changes leftover from previous designs
Comment on attachment 382617 [details] Patch Attachment 382617 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/13202220 New failing tests: mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-ftl-eager-no-cjit-validate-phases
Created attachment 382649 [details] Patch
Comment on attachment 382649 [details] Patch Clearing flags on attachment: 382649 Committed r251959: <https://trac.webkit.org/changeset/251959>
All reviewed patches have been landed. Closing bug.