[webkit-reviews] review denied: [Bug 190641] Web Inspector: display low-power enter/exit events in Timelines and Network node waterfalls : [Attachment 352815] [Patch] WIP

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<drousso at apple.com>'s request for review:
Bug 190641: Web Inspector: display low-power enter/exit events in Timelines and
Network node waterfalls
https://bugs.webkit.org/show_bug.cgi?id=190641

Attachment 352815: [Patch] WIP

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




--- Comment #4 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 352815
  --> https://bugs.webkit.org/attachment.cgi?id=352815
[Patch] WIP

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

This is a WIP, but I'm going to r- because I want to remember to see an updated
version of the patch and, if possible, we should try to have some kind of test.

> Source/JavaScriptCore/inspector/protocol/DOM.json:677
> +		   { "name": "timestamp", "$ref": "Network.Timestamp",
"description": "Time when the <video> entered/exited low power mode" },

Should we include a walltime as well? If we ever wanted the UI to display a
wall time (perhaps in the Detail View) I think we'd want to do this.

Maybe "<video>" => "video element" in these comments. Since we seem to assume
`<code>` in other description comments. I mention this because we have <code>
inline comments in other protocol description strings. But I'd rather you keep
your comment and we eliminate / phase out the <code> since we aren't actually
displaying the description comments anywhere and if we did we'd probably do
something better.

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:336
> +    m_mediaMetrics.clear();

We should also stop the timer if it is running.

    m_mediaMetricsTimer.stop();

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:2443
> +    for (auto* mediaElement : HTMLMediaElement::allMediaElements()) {

Because we don't remove elements from m_mediaMetrics (until reset()) it's
possible that this code would make a mistake if an HTMLMediaElement gets
created, gets metrics, gets destroyed, and then a HTMLMediaElement gets created
at the same address. In that case the old element's m_mediaMetrics will be
compared to the new element's value. That is not as unlikely as it sounds, but
would be quite rare.

Given this loop iterates all of the existing media elements, we should consider
removing HTMLMediaElements from m_mediaMetrics if they didn't show up in
HTMLMediaElement::allMediaElements().

Probably something like:

    m_mediaMetrics.removeIf([&] (auto& entry) {
	return !HTMLMediaElement::allMediaElements().contains(entry.key);
    });

That would reduce the possibility of this being a bug down to a single frame
which I think is better, and avoids the possibility of unbounded growth in
`m_mediaMetrics`. Unless there really is a good opportunity to immediately
remove media elements from m_mediaMetrics, I think this is pretty good.

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:2467
> +	   auto renderedFrameCount = totalVideoFrames -
iterator->value.totalFrames;
> +	   if (renderedFrameCount <= 0)

Since these are both `unsigned` it seems the difference would probably also be
`unsigned` and so I don't think this could ever be less than zero.

I'd rather use explicit types, instead of `auto`, so that this would be more
obvious.

> Source/WebCore/inspector/agents/InspectorDOMAgent.h:304
> +    HashMap<HTMLMediaElement*, MediaMetrics> m_mediaMetrics;

There should probably be a comment that we shouldn't use the pointer in this
map, it is only used for matching. It could even be a `void*` if that makes it
a little more clear that we shouldn't use it.

> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:141
> +	   if (!node)dispatchEventToListenersreturn;

Heh, this is broken. Looks like it should just be a return.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.css:185
> +	/* 1/3 of the vertical space above any .dom-event node */
> +    --low-power-vertical-padding: calc((50% -
(var(--node-waterfall-dom-event-size) / 3)) / 2);

Do you have a screenshot of what this looks like?