[webkit-reviews] review denied: [Bug 203651] Web Inspector: Timelines: add a timeline that shows information about any recorded CSS animation/transition : [Attachment 382425] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has denied Devin Rousso <drousso at apple.com>'s
request for review:
Bug 203651: Web Inspector: Timelines: add a timeline that shows information
about any recorded CSS animation/transition
https://bugs.webkit.org/show_bug.cgi?id=203651

Attachment 382425: Patch

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




--- Comment #12 from Brian Burg <bburg at apple.com> ---
Comment on attachment 382425
  --> https://bugs.webkit.org/attachment.cgi?id=382425
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.