Bug 216416 – Web Inspector: Stop Recording in Timelines tab doesn't work reliably

• Component: Web Inspector
• Status: RESOLVED FIXED

[Patrick Angle]

Clicking "Stop Recording" in the Frames section of Timelines doesn't seem to work on large websites like nfl.com.

<rdar://51643727>

[Patrick Angle]

Created attachment 408557 [details]
Patch

[Devin Rousso]

Comment on attachment 408557 [details]
Patch

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

can you please upload a screenshot?

i'd like to see what it looks like before I r+, but i think this is probably fine :)

> Source/WebInspectorUI/UserInterface/Views/IndeterminateProgressSpinnerNavigationItem.css:26
> +.navigation-bar .item.indeterminateProgressSpinner {

Style: we normally kebab-case CSS, so `indeterminate-progress-spinner`

> Source/WebInspectorUI/UserInterface/Views/IndeterminateProgressSpinnerNavigationItem.css:27
> +    width: 26px;

Is this based on `.navigation-bar .item.button.image-only`?  Perhaps we should pull that out into a variable so we can use it here without having to hardcode the value?

> Source/WebInspectorUI/UserInterface/Views/IndeterminateProgressSpinnerNavigationItem.js:32
> +        console.assert(identifier);

NIT: we normally put assertions before their use, so please move this above `super(identifier);`

> Source/WebInspectorUI/UserInterface/Views/IndeterminateProgressSpinnerNavigationItem.js:34
> +        this._spinner = new WI.IndeterminateProgressSpinner();

Style: you can drop the `()` when constructing with no arguments

> Source/WebInspectorUI/UserInterface/Views/IndeterminateProgressSpinnerNavigationItem.js:36
> +        this._element.classList.add("indeterminateProgressSpinner");

Style: `_element` is a "private" member of a parent class, so please use the public getter `this.element`

> Source/WebInspectorUI/UserInterface/Views/IndeterminateProgressSpinnerNavigationItem.js:37
> +        this._element.append(this._spinner.element);

NIT: `appendChild`

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:466
> +        if (WI.timelineManager.capturingState === WI.TimelineManager.CapturingState.Starting || WI.timelineManager.capturingState === WI.TimelineManager.CapturingState.Active) {

NIT: You could nest this inside the `if` below to avoid a second check in the fail case.  It also reads a bit nicer IMO as then it's more of a "make sure not to do anything while stopping, and also only do this other thing while starting/active".

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:468
> +            // If we are Starting or Active we care about the currentTime, otherwise we do not want to update the
> +            // current time or else the interface will continue to scroll past the stop point.

NIT: part of this is redundant in the surrounding code, so I'd simplify this as just `// Do not update the current time when stopping or inactive or else the interface will continue to scroll past the stop point.`

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:475
> +            // Similar to the currentTime above, but we update the end time also when Inactive, as we may need to show
> +            // records from after we entered the stopping state and so we will do so when the state leaves Stopping.

ditto (:467)

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:861
> +        if (WI.timelineManager.capturingState === WI.TimelineManager.CapturingState.Starting || WI.timelineManager.capturingState === WI.TimelineManager.CapturingState.Active) {

ditto (:466)

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:863
> +            // If we are Starting or Active we care about the currentTime, otherwise we do not want to update the
> +            // current time or else the interface will continue to scroll past the stop point.

ditto (:467)

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:869
> +            // Similar to the currentTime above, but we update the end time also when Inactive, as we may need to show
> +            // records from after we entered the stopping state and so we will do so when the state leaves Stopping.

ditto (:467)

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingProgressView.js:35
> +        statusGroup.classList.add("status");

NIT: for DOM nodes that are recently created, you can use `.className = "..."`
```
    statusGroup.className = "status";
```

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingProgressView.js:38
> +        this._statusElement = document.createElement("span");
> +        statusGroup.append(this._statusElement);

You can do this on one line:
```
    this._statusElement = statusGroup.appendChild(document.createElement("span"));
```

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingProgressView.js:43
> +        this.element.append(statusGroup);

ditto (:37)
```
    let statusGroup = this.element.appendChild(document.createElement("div"));
```

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingProgressView.js:63
> +        if (x) {

NIT: please move this to the end of the function so that if future code in `_updateState` wants to use `this._visible` (or the `classList`) it's been properly set

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingProgressView.js:67
> +            WI.timelineManager.removeEventListener(null, null, this);

I think we should start avoiding uses of this, as it's kinda a "hammer" in that it just removes _all_ event listeners from the target that have `this`.  Unless we're disposing of the view, we should try to be more specific where we can.
```
    WI.timelineManager.removeEventListener(WI.TimelineManager.Event.CapturingStateChanged, this._handleTimelineCapturingStateChanged, this);
```

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingProgressView.js:88
> +            // When inactive this view should be hidden by its parent, so keep the state the same to avoid possibly
> +            // flickering to a different state just before the parent hides us.

Can we `console.assert(!this._visible)`, or more likely can we `console.assert(false)` since we should never hit this as we remove the event listener when not `visible`?

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingProgressView.js:93
> +    _setStateRecording()

since this is only really called once and is quite simple, I'd just inline it in `_updateState`

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingProgressView.js:99
> +    _setStateStopping()

ditto (:93)

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingProgressView.js:101
> +        this._statusElement.textContent = WI.UIString("Stopping Recording Timeline Data", "Stopping Recording Timeline Data @ Timeline Recording Progress", "Message for progress of stopping a timeline recording.");

"Stopping Recording Timeline Data" is kinda awkward to say/read.  Also, we normally use "Recording" as a noun in this context, e.g. "Clear Timeline Recording".

How about "Stopping Timeline Recording" or even just "Stopping"?

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingProgressView.js:105
> +    _handleTimelineCapturingStateChanged()

NIT: This isn't required or in a style guide or anything, but I usually try to list all parameters expected to ever be passed to a function so that it's clear how it may end up being used (not to mention it makes future diffs cleaner).  In this case, `_handleTimelineCapturingStateChanged` is an event listener so it probably should have an `event` parameter.  I also like prefixing the name with `_handle` so thanks for doing that too :)

[Patrick Angle]

Comment on attachment 408557 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/IndeterminateProgressSpinnerNavigationItem.js:32
>> +        console.assert(identifier);
> 
> NIT: we normally put assertions before their use, so please move this above `super(identifier);`

Whoops. It looks like this was wrong in TextNavigationItem as well, which is what I modeled this after. I'll drive by and fix it over there as well.

>> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingProgressView.js:88
>> +            // flickering to a different state just before the parent hides us.
> 
> Can we `console.assert(!this._visible)`, or more likely can we `console.assert(false)` since we should never hit this as we remove the event listener when not `visible`?

Visibility is handled by the parent, so it's possible for this view to receive a `CapturingState.Inactive` event before or after the parent does, so the parent may not have set visibility by the time we are here, so I'm not sure asserting visibility is correct in this case.

[Patrick Angle]

Created attachment 408566 [details]
Patch

[Patrick Angle]

Created attachment 408567 [details]
Frames view with patch

[Patrick Angle]

Created attachment 408568 [details]
Events view with patch

[Devin Rousso]

Comment on attachment 408566 [details]
Patch

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

r=me, nice work!

> Source/WebInspectorUI/UserInterface/Views/IndeterminateProgressSpinnerNavigationItem.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/IndeterminateProgressSpinnerNavigationItem.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`

[Patrick Angle]

Created attachment 408569 [details]
Patch

[Patrick Angle]

Comment on attachment 408566 [details]
Patch

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

>> 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.

Nope, don't need it. Opened a bug to remove it: https://bugs.webkit.org/show_bug.cgi?id=216423

[Patrick Angle]

Created attachment 408732 [details]
Patch

[Devin Rousso]

Comment on attachment 408732 [details]
Patch

r=me, nice!

[EWS]

Committed r267038: <https://trac.webkit.org/changeset/267038>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408732 [details].