Bug 191625 - Web Inspector: Timelines: add Media timeline
Summary: Web Inspector: Timelines: add Media timeline
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on: 189773 189874 190641
Blocks: 191635
  Show dependency treegraph
 
Reported: 2018-11-14 00:17 PST by Devin Rousso
Modified: 2018-11-25 14:12 PST (History)
6 users (show)

See Also:


Attachments
Patch (93.10 KB, patch)
2018-11-14 00:44 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (968.26 KB, image/png)
2018-11-14 00:44 PST, Devin Rousso
no flags Details
Patch (92.92 KB, patch)
2018-11-16 16:06 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (72.90 KB, patch)
2018-11-19 22:55 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (72.57 KB, patch)
2018-11-21 19:06 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2018-11-14 00:17:49 PST
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.
Comment 1 Devin Rousso 2018-11-14 00:44:05 PST
Created attachment 354778 [details]
Patch
Comment 2 Devin Rousso 2018-11-14 00:44:20 PST
Created attachment 354779 [details]
[Image] After Patch is applied
Comment 3 Devin Rousso 2018-11-14 13:52:58 PST
Test page <https://devinrousso.com/demo/WebKit/stream_of_water.html>
Comment 4 Nikita Vasilyev 2018-11-14 14:51:42 PST
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.
Comment 5 Devin Rousso 2018-11-14 15:01:04 PST
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
Comment 6 Devin Rousso 2018-11-14 16:24:07 PST
(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>
Comment 7 Nikita Vasilyev 2018-11-14 18:55:55 PST
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.
Comment 8 Devin Rousso 2018-11-16 16:06:53 PST
Created attachment 355150 [details]
Patch
Comment 9 Matt Baker 2018-11-19 14:52:13 PST
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.
Comment 10 Devin Rousso 2018-11-19 15:01:54 PST
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?
Comment 11 Matt Baker 2018-11-19 15:34:39 PST
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.
Comment 12 Devin Rousso 2018-11-19 22:55:40 PST
Created attachment 355313 [details]
Patch
Comment 13 Matt Baker 2018-11-21 18:05:03 PST
(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.
Comment 14 Devin Rousso 2018-11-21 19:03:45 PST
(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).
Comment 15 Devin Rousso 2018-11-21 19:06:51 PST
Created attachment 355453 [details]
Patch
Comment 16 Matt Baker 2018-11-25 13:23:30 PST
Comment on attachment 355453 [details]
Patch

r=me
Comment 17 WebKit Commit Bot 2018-11-25 14:11:31 PST
Comment on attachment 355453 [details]
Patch

Clearing flags on attachment: 355453

Committed r238484: <https://trac.webkit.org/changeset/238484>
Comment 18 WebKit Commit Bot 2018-11-25 14:11:33 PST
All reviewed patches have been landed.  Closing bug.
Comment 19 Radar WebKit Bug Importer 2018-11-25 14:12:31 PST
<rdar://problem/46233165>