[webkit-reviews] review granted: [Bug 216416] Web Inspector: Stop Recording in Timelines tab doesn't work reliably : [Attachment 408566] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Patrick Angle <pangle at apple.com>'s
request for review:
Bug 216416: Web Inspector: Stop Recording in Timelines tab doesn't work
reliably
https://bugs.webkit.org/show_bug.cgi?id=216416

Attachment 408566: Patch

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




--- Comment #7 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 408566
  --> https://bugs.webkit.org/attachment.cgi?id=408566
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408566&action=review

r=me, nice work!

>
Source/WebInspectorUI/UserInterface/Views/IndeterminateProgressSpinnerNavigatio
nItem.js:37
> +	   this.element.appendChild(this._spinner.element);

NIT: I'd move this closer to where `this._spinner` is created.	I tend to like
to keep operations related to a given node/view as closely packed together as
possible so that it's really easy to find and understand the full scope of how
things are created and configured in the DOM :)

>
Source/WebInspectorUI/UserInterface/Views/IndeterminateProgressSpinnerNavigatio
nItem.js:39
> +	   this.tooltip = tooltip;

Should we assert that `tooltip` is provided, or at least provide an `?? ""` so
that the `title` doesn't get set to `"undefined"`?

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:472
> +		   this._currentTime = currentTime;

If we're no longer updating the `currentTime`, do we still need the "stop
requested" marker (grey circle)?  If not, we should remove it (and any
dedicated/associated logic) in a followup.

> Source/WebInspectorUI/UserInterface/Views/Variables.css:217
> +    --navigation-item-image-only-width: 26px;

this should probably include `button` in it somewhere, e.g.
`--image-button-navigation-item-width`