[webkit-reviews] review denied: [Bug 189773] Web Inspector: create special Network waterfall for media events : [Attachment 351811] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<drousso at apple.com>'s request for review:
Bug 189773: Web Inspector: create special Network waterfall for media events
https://bugs.webkit.org/show_bug.cgi?id=189773

Attachment 351811: Patch

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




--- Comment #8 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 351811
  --> https://bugs.webkit.org/attachment.cgi?id=351811
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.