[webkit-reviews] review granted: [Bug 205434] Web Inspector: add instrumentation for showing existing Web Animations : [Attachment 386879] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has granted Devin Rousso <drousso at apple.com>'s
request for review:
Bug 205434: Web Inspector: add instrumentation for showing existing Web
Animations
https://bugs.webkit.org/show_bug.cgi?id=205434

Attachment 386879: Patch

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




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