[webkit-reviews] review granted: [Bug 189874] Web Inspector: display fullscreen enter/exit events in Timelines and Network node waterfalls : [Attachment 352066] Patch

[bugzilla-daemon at webkit.org]

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

Attachment 352066: Patch

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




--- Comment #13 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 352066
  --> https://bugs.webkit.org/attachment.cgi?id=352066
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:144
> +	   let lowerCaseName = this.localName() ||
this.nodeName().toLowerCase();
> +	   if (lowerCaseName === "video" || lowerCaseName === "audio")

Should we make this a helper? `_isMediaElement` or
`_shouldListenForEventListeners`? It seems like something that might be easier
to find in the future.

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:747
> +	       && domEvent.timestamp - previousDOMEvent.timestamp < 0.001

Whats up with this? Should we just give each domEvent a sequential identifier,
and compare those? That seems like it would prevent this fuzzy matching.

Something like:

    this._addDOMEvent({
	eventName,
	timestamp: ...,
	data,
	__identifier: DOMManager.nextDOMEventSequentialIdentifier++,
    });

> Source/WebInspectorUI/UserInterface/Views/DOMEventsBreakdownView.js:106
> +	   let fullscreenDOMEvents =
WI.DOMNode.getFullscreenDOMEvents(this._domEvents);
> +	   let fullscreenRanges = fullscreenDOMEvents.reduce((accumulator,
current) => {
> +	       let {enabled} = current.data;
> +	       if (enabled || !accumulator.length) {
> +		   accumulator.push({
> +		       startTimestamp: enabled ? current.timestamp :
startTimestamp,
> +		   });
> +	       }
> +	       accumulator.lastValue.endTimestamp = (enabled && current ===
fullscreenDOMEvents.lastValue) ? endTimestamp : current.timestamp;
> +	       return accumulator;
> +	   }, []);

This doesn't read any better than a for-loop to me. Is there any advantage to
using reduce here?

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:685
> +		   for (let i = 0; i < fullscreenDOMEvents.length; i += 2) {

Lets assert that we have pairs. Something like:

    console.assert((fullscreenDOMEvents.length % 2) === 0);

If someone were to change getFullscreenDOMEvents to not elide common `enabled`
siblings then this would help catch that.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:691
> +		       let originator = fullscreenDOMEvents[i].originator;

If we hit the line 679 case above then range starts without a known originator
but ends with a known originator and we would miss that here. I'd suggest:

    let originator = fullscreenDOMEvents[i].originator || fullscreenDOMEvents[i
+ 1].originator;

> LayoutTests/ChangeLog:7
> +	   Reviewed by Joseph Pecoraro.

lol?

> LayoutTests/http/tests/inspector/dom/didFireEvent.html:43
> +

Style: Remove empty line.

> LayoutTests/http/tests/inspector/dom/didFireEvent.html:68
> +	       InspectorTest.pass(`Should recieve a "webkitfullscreenchange"
event.`);

Typo: recieve => receive

Aside: This typo exists in
LayoutTests/inspector/dom/getSupportedEventNames.html as well!

> LayoutTests/http/tests/inspector/dom/didFireEvent.html:140
> +    <button id="fullscreen"></button>

It looks like this button is not needed. Was it for local testing?