Bug 191625 – Web Inspector: Timelines: add Media timeline

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

Add a new timeline to Timelines for media events/recordings/information.  Right now, this will include all of the event/fullscreen/low-power tracking done for <audio>/<video>/<picture> elements that is already visible in the Network tab.

[Devin Rousso]

Created attachment 354778 [details]
Patch

[Devin Rousso]

Created attachment 354779 [details]
[Image] After Patch is applied

[Devin Rousso]

Test page <https://devinrousso.com/demo/WebKit/stream_of_water.html>

[Nikita Vasilyev]

Comment on attachment 354778 [details]
Patch

Not a complete review.

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

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:729
> +        if (!isEmptyObject(data))
> +            domEvent.data = data;

Why is this condition needed?

> Source/WebInspectorUI/UserInterface/Models/MediaTimelineRecord.js:45
> +    get isLowPower() { return this._isLowPower; }

This seems to be unused but I suppose it's okay to have it for symmetry.

> Source/WebInspectorUI/UserInterface/Models/TimelineRange.js:33
> +        this._startTime = isNaN(startTime) ? NaN : startTime;
> +        this._endTime = isNaN(endTime) ? NaN : endTime;

How is this different from:

this._startTime = startTime;
this._endTime = endTime;

?

> Source/WebInspectorUI/UserInterface/Models/TimelineRange.js:53
> +        if (isNaN(this._startTime))
> +            this._startTime = NaN;

Again, why do we need to set it to NaN when it's already NaN?..

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.css:132
> +    background-color: rgb(148, 190, 164);
> +    border-color: rgb(101, 161, 134);

Nit: we prefer HSL over RGB for new code.

[Devin Rousso]

Comment on attachment 354778 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:729
>> +            domEvent.data = data;
> 
> Why is this condition needed?

It's mainly to prevent the key from being added when unnecessary.  It's not needed, per se, but it is nicer to look at when debugging.

>> Source/WebInspectorUI/UserInterface/Models/TimelineRange.js:33
>> +        this._endTime = isNaN(endTime) ? NaN : endTime;
> 
> How is this different from:
> 
> this._startTime = startTime;
> this._endTime = endTime;
> 
> ?

It's mainly to ensure that the held values are "nice" (e.g. not `undefined` or `null` or `"asd"`).  I keep forgetting that `isNaN(null) === false` anyways, so I'll change this a bit.

>> Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.css:132
>> +    border-color: rgb(101, 161, 134);
> 
> Nit: we prefer HSL over RGB for new code.

Ah whoops.  Copied these values from the images :P

[Devin Rousso]

(In reply to Devin Rousso from comment #3)
> Test page <https://devinrousso.com/demo/WebKit/stream_of_water.html>
Test page (Low Power) <https://media.w3.org/2010/05/sintel/trailer.mp4>

[Nikita Vasilyev]

Comment on attachment 354778 [details]
Patch

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

I couldn't test low power mode events, but the DOM events and full-screen events worked well!

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:1186
> +            ++this._lowPowerRange.details.count;

Nit: I actually find `foo++` to be more readable than `++foo`. Is it in our guidelines now to use `++foo`?

>>> Source/WebInspectorUI/UserInterface/Models/TimelineRange.js:33
>>> +        this._endTime = isNaN(endTime) ? NaN : endTime;
>> 
>> How is this different from:
>> 
>> this._startTime = startTime;
>> this._endTime = endTime;
>> 
>> ?
> 
> It's mainly to ensure that the held values are "nice" (e.g. not `undefined` or `null` or `"asd"`).  I keep forgetting that `isNaN(null) === false` anyways, so I'll change this a bit.

`isNaN(endTime) ? NaN` just feels wrong. If you end up using it, please leave a comment.

[Devin Rousso]

Created attachment 355150 [details]
Patch

[Matt Baker]

Comment on attachment 355150 [details]
Patch

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

Overall this looks pretty good. I would like to see an updated version of this path which eliminates the Timeline "area" ranges, for the following reasons:

- The gray treatment feels like something is disabled, and is hard to distinguish from the "unselected" portion of the timeline overview graph.
- The area covers other timeline overview graphs. This may have been intentional, but even though the full-screen event applies to the whole document it isn't relevant to all timelines.
- z-index issues: mousing over the range/resizing the window causes the portion covered by the horizontal scroll container to pop.

And it adds a lot of code to the patch. Let's do this as a follow-up/refinement, once we'e had a change to hammer out a solid design.

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:121
> +            let layoutInstrumentIndex = types.indexOf(WI.TimelineRecord.Type.Layout) + 1;

Rename `layoutInstrumentIndex` to `insertionIndex`, since it's not the index of the Layout instrument.

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:730
> +        this._addDOMEvent(domEvent);

Why was this refactored?

> Source/WebInspectorUI/UserInterface/Models/MediaTimelineRecord.js:2
> + * Copyright (C) 2013 Apple Inc. All rights reserved.

2018

> Source/WebInspectorUI/UserInterface/Models/MediaTimelineRecord.js:52
> +                return this._domEvent.data.enabled ? WI.UIString("Entered Full Screen Mode") : WI.UIString("Exited Full Screen Mode");

Should be "Full-Screen Mode"; Apple's developer documentation hyphenates full-screen.

> Source/WebInspectorUI/UserInterface/Models/MediaTimelineRecord.js:57
> +            return this._isLowPower ? WI.UIString("Entered Low Power Mode") : WI.UIString("Exited Low Power Mode");

Ditto, "Low-Power Mode".

> Source/WebInspectorUI/UserInterface/Models/MediaTimelineRecord.js:62
> +        return WI.UIString("DOM Event");

Are there events other than changes to full-screen and low-power mode state? If not this should assert(false) and return null.

> Source/WebInspectorUI/UserInterface/Models/TimelineRange.js:32
> +        this._startTime = startTime;

This class is also used for frame ranges, hence the units-agnostic approach. We should continue using `*Value` instead of `*Time`.

> Source/WebInspectorUI/UserInterface/Views/MediaTimelineOverviewGraph.css:2
> + * Copyright (C) 2014 Apple Inc. All rights reserved.

2018

> Source/WebInspectorUI/UserInterface/Views/MediaTimelineOverviewGraph.js:3
> + * Copyright (C) 2014, 2015 Apple Inc. All rights reserved.

2018

> Source/WebInspectorUI/UserInterface/Views/MediaTimelineOverviewGraph.js:78
> +            if (!timelineRecordBar)

Style: I realize this code is copy-pasted, but it just seems odd testing "not timelineRecordBar" instead of:

if (timelineRecordBar) {
    timelineRecordBar.renderMode = renderMode;
    timelineRecordBar.records = records;
} else
    timelineRecordBar = this._recordBars[recordBarIndex] = new WI.TimelineRecordBar(this, records, renderMode);

> Source/WebInspectorUI/UserInterface/Views/MediaTimelineView.js:2
> + * Copyright (C) 2014, 2015 Apple Inc. All rights reserved.

2018

> Source/WebInspectorUI/UserInterface/Views/TimelineOverview.js:695
> +                continue;

Nice.

[Devin Rousso]

Comment on attachment 355150 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:730
>> +        this._addDOMEvent(domEvent);
> 
> Why was this refactored?

This was mainly to prevent adding an `undefined` value to `domEvent` when no `data` is provided.  It isn't strictly necessary, so I'll remove it.

>> Source/WebInspectorUI/UserInterface/Models/MediaTimelineRecord.js:62
>> +        return WI.UIString("DOM Event");
> 
> Are there events other than changes to full-screen and low-power mode state? If not this should assert(false) and return null.

Non-fullscreen events will be shown in the media timeline (e.g. `play` and `pause`), but this point should also be unreachable so I'll add an assert.

>> Source/WebInspectorUI/UserInterface/Models/TimelineRange.js:32
>> +        this._startTime = startTime;
> 
> This class is also used for frame ranges, hence the units-agnostic approach. We should continue using `*Value` instead of `*Time`.

Really?  When I search for it all I see it being used for is the Timeline tab's path component selection UI.  Where else do you see this being used?

[Matt Baker]

Comment on attachment 355150 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Models/TimelineRange.js:32
>>> +        this._startTime = startTime;
>> 
>> This class is also used for frame ranges, hence the units-agnostic approach. We should continue using `*Value` instead of `*Time`.
> 
> Really?  When I search for it all I see it being used for is the Timeline tab's path component selection UI.  Where else do you see this being used?

That's the only place it's used, but these are the same path components that are used when selecting frame ranges in the Rendering Frames view.

[Devin Rousso]

Created attachment 355313 [details]
Patch

[Matt Baker]

(In reply to Devin Rousso from comment #12)
> Created attachment 355313 [details]
> Patch

This revision looks better, just a couple things I noticed (hence the r- for now):

- The Name column in the MediaTimelineView is missing icons.
- The same events in the timeline are labeled differently, depending on whether they are shown in the overview or Media timeline. For example,

Steps to Reproduce:
1. Open https://devinrousso.com/demo/WebKit/stream_of_water.html.
2. Start recording
3. Hit play on the video
4. Stop recording

=> Media Timeline shows names like "play", "playing"
=> Overview shows "Play Event Dispatched", "Playing Event Dispatched". Other timelines (JavaScript & Events, and Layout & Rendering) show the same name in both.

[Devin Rousso]

(In reply to Matt Baker from comment #13)
> - The same events in the timeline are labeled differently, depending on whether they are shown in the overview or Media timeline. For example,
So I'm not sure how we want to address this.  As mentioned offline, I realized today that we already do show events being fired, but only if the event already has a listener.  In the case of the media events, because our instrumentation uses an event listener, the script timeline thinks that there is an event listener for it in the page, and shows an event as such.  I think that we should land this as is for now, and make a followup such that we don't show a timeline event if the event listener was added via C++ (as in the case of media events).

As far as the naming goes, I personally think that it should match the event name used in JavaScript, as that's what developers are likely to search for.  The names used by the events timeline are localized, meaning that if an event name localizes to a completely different set of characters (which is probably pretty common), searching for it may be confusing (I'd imagine the developer to search for "play", and then either give up (thinking it didn't fire) or search for it in the localized language.  I'd prefer if we kept it as is in this patch (and maybe even changed the other patch to match).

[Devin Rousso]

Created attachment 355453 [details]
Patch

[Matt Baker]

Comment on attachment 355453 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 355453 [details]
Patch

Clearing flags on attachment: 355453

Committed r238484: <https://trac.webkit.org/changeset/238484>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46233165>