Web Inspector: create special Network waterfall for media events

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

Instead of leaving the waterfall column blank for node entries in `WI.NetworkTableContentView`, we should provide a timeline of events related to that particular node.  This would help provide a more visual way of correlating network requests with events. (e.g. tying `stalled` media events with longer than expected byte-range requests).

[Radar WebKit Bug Importer]

<rdar://problem/44626605>

[Devin Rousso]

Created attachment 350168 [details]
[Patch] WIP

[Devin Rousso]

Created attachment 350793 [details]
[Patch] WIP

[Devin Rousso]

Created attachment 350795 [details]
[Patch] WIP

[Devin Rousso]

Created attachment 350797 [details]
[Image] After Patch is applied (Network Table)

[Devin Rousso]

Created attachment 350798 [details]
[Image] After Patch is applied (DOM Node Selected)

[Devin Rousso]

Created attachment 351811 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 351811 [details]
Patch

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

Overall this looks good! I'd like to see an image of the network table popover for the media elements.

• How does this look for a video that has been playing for 30s?
• Can we get a screenshot where the items in the Table sidebar are indented more like TreeElements. Namely:
    1. All rows have space for a disclosure triangle
    2. Rows that are children of a media element are indented more

Going to r- just to see an updated patch.

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:2141
> +    auto createEventListener = [&] (const AtomicString& eventName) {
> +        node.addEventListener(eventName, EventFiredCallback::create(*this), false);
> +    };
> +
> +    if (is<HTMLMediaElement>(node)) {
> +        createEventListener(eventNames().abortEvent);
> +        createEventListener(eventNames().canplayEvent);

This is creating an EventFiredCallback for each event name. Can just one object be created for all of them?

In web content we can do:

    element.addEventListener("mousedown", this);
    element.addEventListener("click", this);

So I'm wondering if here you can do:

    auto listener = EventFiredCallback::create(*this);

    if (is<HTMLMediaElement>(node)) {
        node.addEventListener(eventNames().abortEvent, listener, false);
        node.addEventListener(eventNames().canplayEvent, listener, false);
        ...
    }

To avoid creating 20 listeners.

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:467
> +Object.defineProperty(Array.prototype, "adjacentForEach",

Hmm, this doesn't strike me as something worth adding to the Array prototype. It just invokes the callback with (currentValue, nextValue) up to (nextToLastValue, lastValue). Very peculiar. Why not just inline this?

> Source/WebInspectorUI/UserInterface/Views/DOMEventsBreakdownView.js:49
> +        let headElement = tableElement.appendChild(document.createElement("thead"));

Make this `position: sticky`? In which case this might need a border-bottom instead of tbody having a border-top.

> Source/WebInspectorUI/UserInterface/Views/DOMEventsBreakdownView.js:70
> +    layout()
> +    {
> +        super.layout();
> +
> +        this._tableBodyElement.removeChildren();

Either this should be updating every time a new event happens for this video, or there should be a refresh button to get any new events that have happened since opening the view.

It seems the embedder (DOMNodeEventsContentView) automatically updates this as events happen. This layout seems like it can be pretty expensive (layout all rows every rAF with events) We should consider virtualizing, perhaps even using WI.Table. Imagine a video element that has been playing for a long time and has accumulated a lot of events.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeEventsContentView.js:33
> +        const representedObject = null;
> +        super(representedObject);

Should we just pass the DOM Node up or is that a bad idea?

> Source/WebInspectorUI/UserInterface/Views/DOMNodeEventsContentView.js:55
> +        this._domNode.addEventListener(WI.DOMNode.Event.DidFireEvent, this._handleDOMNodeDidFireEvent, this);

This could happen when shown()/hidden(). And shown() could trigger a needsLayout(). I'm not sure if that is much better than the current.

> Source/WebInspectorUI/UserInterface/Views/NetworkDOMNodeDetailView.js:2
> + * Copyright (C) 2017 Apple Inc. All rights reserved.

2018.

> Source/WebInspectorUI/UserInterface/Views/NetworkResourceDetailView.js:-51
> -    get resource() { return this._resource; }

This strikes me as risky / annoying. Typically I tend to declare a better member name for the representedObject in the constructor anyways:

    this._resource = this.representedObject;

This makes it easier / more obvious in this code what we're working with. Not a hard requirement, but the class is named Network*Resource*DetailValue

> Source/WebInspectorUI/UserInterface/Views/NetworkResourceDetailView.js:93
> +        return super.detailNavigationItems.concat([
> +            new WI.RadioButtonNavigationItem("preview", WI.UIString("Preview")),
> +            new WI.RadioButtonNavigationItem("headers", WI.UIString("Headers")),
> +            new WI.RadioButtonNavigationItem("cookies", WI.UIString("Cookies")),
> +            new WI.RadioButtonNavigationItem("sizes", WI.UIString("Sizes")),
> +            new WI.RadioButtonNavigationItem("timing", WI.UIString("Timing")),
> +        ]);

Hmm, I don't like that these are in the getter. Ideally the list of navigation items can be initialized once in the constructor or initial layout as they don't change. That would avoid this temporary array, the getter, and the weird feeling super call.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:662
> +            let domEventElementSize = parseInt(window.getComputedStyle(this._table.element).getPropertyValue("--node-waterfall-dom-event-size"));

Does this `getComputedStyle` mean that each time we add a media element row we force a layout?

Even better, this is just a constant. Make it a constant with a comment to keep in sync with CSS.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1847
> -            return WI.Rect.rectFromClientRect(mouseBlock.getBoundingClientRect());
> +            return WI.Rect.rectFromClientRect(targetElement.getBoundingClientRect());

Do you recall why you made this change? I think the previous code was re-getting the row in case the Table blew away and created a new element. I'm not sure the new code would work in that case.

> LayoutTests/http/tests/inspector/dom/didFireEvent.html:44
> +        WI.domManager.querySelector(documentNode.id, "video#video", (videoNodeId) => {

Nit: Just `#video` is enough to uniquely identify the node.

[Devin Rousso]

Comment on attachment 351811 [details]
Patch

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

(In reply to Joseph Pecoraro from comment #8)
> • Can we get a screenshot where the items in the Table sidebar are indented
> more like TreeElements. Namely:
>     1. All rows have space for a disclosure triangle
>     2. Rows that are children of a media element are indented more
Followup: <https://webkit.org/b/190388>

>> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:2141
>> +        createEventListener(eventNames().canplayEvent);
> 
> This is creating an EventFiredCallback for each event name. Can just one object be created for all of them?
> 
> In web content we can do:
> 
>     element.addEventListener("mousedown", this);
>     element.addEventListener("click", this);
> 
> So I'm wondering if here you can do:
> 
>     auto listener = EventFiredCallback::create(*this);
> 
>     if (is<HTMLMediaElement>(node)) {
>         node.addEventListener(eventNames().abortEvent, listener, false);
>         node.addEventListener(eventNames().canplayEvent, listener, false);
>         ...
>     }
> 
> To avoid creating 20 listeners.

🤦

>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:467
>> +Object.defineProperty(Array.prototype, "adjacentForEach",
> 
> Hmm, this doesn't strike me as something worth adding to the Array prototype. It just invokes the callback with (currentValue, nextValue) up to (nextToLastValue, lastValue). Very peculiar. Why not just inline this?

I would tend to agree that having it be "local" makes a bit more sense, but I also have found in the past that this inevitably leads to duplication of code, and tends to result in another patch that centralizes all the call-sites.  I personally think the "cost" of adding it to Utilities is worth avoiding that scenario (plus it's general enough that it could be used elsewhere).

>> Source/WebInspectorUI/UserInterface/Views/DOMEventsBreakdownView.js:49
>> +        let headElement = tableElement.appendChild(document.createElement("thead"));
> 
> Make this `position: sticky`? In which case this might need a border-bottom instead of tbody having a border-top.

I'll see how that feels.

>> Source/WebInspectorUI/UserInterface/Views/DOMEventsBreakdownView.js:70
>> +        this._tableBodyElement.removeChildren();
> 
> Either this should be updating every time a new event happens for this video, or there should be a refresh button to get any new events that have happened since opening the view.
> 
> It seems the embedder (DOMNodeEventsContentView) automatically updates this as events happen. This layout seems like it can be pretty expensive (layout all rows every rAF with events) We should consider virtualizing, perhaps even using WI.Table. Imagine a video element that has been playing for a long time and has accumulated a lot of events.

I'll make this into a `WI.Table`.

>> Source/WebInspectorUI/UserInterface/Views/DOMNodeEventsContentView.js:33
>> +        super(representedObject);
> 
> Should we just pass the DOM Node up or is that a bad idea?

The views that `WI.Resource` creates in the Network tab follow the same pattern.  In this case, I don't think we want to pass it up as that implies that `WI.DOMNode` is something that can be displayed.  In reality, all we really want is the list of `domEvents`, but we also take the `WI.DOMNode` so that we can listen for `WI.DOMNode.Event.DidFireEvent`.

>> Source/WebInspectorUI/UserInterface/Views/DOMNodeEventsContentView.js:55
>> +        this._domNode.addEventListener(WI.DOMNode.Event.DidFireEvent, this._handleDOMNodeDidFireEvent, this);
> 
> This could happen when shown()/hidden(). And shown() could trigger a needsLayout(). I'm not sure if that is much better than the current.

Ditto (33).  This seems like it's a bit easier logically because then we don't need to do any sort of calculus as to what `domEvent`s were added while we were `hidden()`.

>> Source/WebInspectorUI/UserInterface/Views/NetworkResourceDetailView.js:-51
>> -    get resource() { return this._resource; }
> 
> This strikes me as risky / annoying. Typically I tend to declare a better member name for the representedObject in the constructor anyways:
> 
>     this._resource = this.representedObject;
> 
> This makes it easier / more obvious in this code what we're working with. Not a hard requirement, but the class is named Network*Resource*DetailValue

I would argue that the Network*Resource*DetailView makes it clear as to what type of `representedObject` we're dealing with.  I personally find it more confusing when there's two different keys that map to the same value, not to mention it may complicate logic if it happens that either needs to be updated since then the other will too.

>> Source/WebInspectorUI/UserInterface/Views/NetworkResourceDetailView.js:93
>> +        ]);
> 
> Hmm, I don't like that these are in the getter. Ideally the list of navigation items can be initialized once in the constructor or initial layout as they don't change. That would avoid this temporary array, the getter, and the weird feeling super call.

Agreed.  Will change.

>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:662
>> +            let domEventElementSize = parseInt(window.getComputedStyle(this._table.element).getPropertyValue("--node-waterfall-dom-event-size"));
> 
> Does this `getComputedStyle` mean that each time we add a media element row we force a layout?
> 
> Even better, this is just a constant. Make it a constant with a comment to keep in sync with CSS.

🤦 yup.  Will rework as a `const`.

>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1847
>> +            return WI.Rect.rectFromClientRect(targetElement.getBoundingClientRect());
> 
> Do you recall why you made this change? I think the previous code was re-getting the row in case the Table blew away and created a new element. I'm not sure the new code would work in that case.

I think it had to do with the fact that since events are fired somewhat frequently, it would cause the `WI.Popover` constantly shift, making it much harder to read.  Not entirely sure though.  Will investigate.

[Devin Rousso]

Comment on attachment 351811 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/DOMEventsBreakdownView.js:49
>>> +        let headElement = tableElement.appendChild(document.createElement("thead"));
>> 
>> Make this `position: sticky`? In which case this might need a border-bottom instead of tbody having a border-top.
> 
> I'll see how that feels.

`position: sticky` doesn't seem to play nicely with <thead>, at least not with a `border-bottom` 🤔

>>> Source/WebInspectorUI/UserInterface/Views/DOMEventsBreakdownView.js:70
>>> +        this._tableBodyElement.removeChildren();
>> 
>> Either this should be updating every time a new event happens for this video, or there should be a refresh button to get any new events that have happened since opening the view.
>> 
>> It seems the embedder (DOMNodeEventsContentView) automatically updates this as events happen. This layout seems like it can be pretty expensive (layout all rows every rAF with events) We should consider virtualizing, perhaps even using WI.Table. Imagine a video element that has been playing for a long time and has accumulated a lot of events.
> 
> I'll make this into a `WI.Table`.

On second thought, `WI.Table` adds a ton of stuff that I don't think we want right now, like sorting, column show/hide, and even the ability to select rows.  I will see about this more, and possibly address it in a followup.

For now, I'll shift the logic from regenerating on every `layout()` to only when new events are fired.

>>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1847
>>> +            return WI.Rect.rectFromClientRect(targetElement.getBoundingClientRect());
>> 
>> Do you recall why you made this change? I think the previous code was re-getting the row in case the Table blew away and created a new element. I'm not sure the new code would work in that case.
> 
> I think it had to do with the fact that since events are fired somewhat frequently, it would cause the `WI.Popover` constantly shift, making it much harder to read.  Not entirely sure though.  Will investigate.

I removed this because it isn't used unless the inspector window is resized.  Additionally, `domNode`s don't have a `.mouse-tracking` element, which means that it would have different effects depending on whether a resource/node was selected.  I'll add it back for resource entries.

[Devin Rousso]

Created attachment 351856 [details]
Patch

[BJ Burg]

(In reply to Devin Rousso from comment #10)
> Comment on attachment 351811 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=351811&action=review
> 
> >>> Source/WebInspectorUI/UserInterface/Views/DOMEventsBreakdownView.js:49
> >>> +        let headElement = tableElement.appendChild(document.createElement("thead"));
> >> 
> >> Make this `position: sticky`? In which case this might need a border-bottom instead of tbody having a border-top.
> > 
> > I'll see how that feels.
> 
> `position: sticky` doesn't seem to play nicely with <thead>, at least not
> with a `border-bottom` 🤔

Unfortunately, we've been down this path an the root cause is still not fixed. https://bugs.webkit.org/show_bug.cgi?id=128486

[Joseph Pecoraro]

Comment on attachment 351856 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:467
> +Object.defineProperty(Array.prototype, "adjacentForEach",

I still don't see value in this. If I read this I'd think it would run callback with both the adjacent elements, something like (before, current, next)

    ( undefi, arr[0], arr[1] )
    ( arr[0], arr[1], arr[2] )
    ( arr[1], arr[2], arr[3] )
    ( arr[2], arr[3], undefi )

The fact that it doesn't do what I expect is reason to not make it a top level function like this. Another reason is that it is a single for loop.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1845
> +            if (!entry.resource)
> +                return WI.Rect.rectFromClientRect(targetElement.getBoundingClientRect());

I still feel like this will fall over in some way but I'm not exactly sure how to test. Maybe some kind of table update that happens while the popover is showing. (An incomplete request becoming complete?)

> LayoutTests/http/tests/inspector/dom/didFireEvent.html:18
> +    let videoNode = null;

Style: I normally drop the assignment on these "values to be populated"

    let videoNode;

[Devin Rousso]

Created attachment 352002 [details]
Patch

[Devin Rousso]

Created attachment 352006 [details]
Patch

Fix test

[WebKit Commit Bot]

Comment on attachment 352006 [details]
Patch

Clearing flags on attachment: 352006

Committed r237028: <https://trac.webkit.org/changeset/237028>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.