Bug 189874 – Web Inspector: display fullscreen enter/exit events in Timelines and Network node waterfalls

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

It is often useful to know when an element/page entered or existed fullscreen mode, especially if you can relate that event to nearby timeline/network activity.

[Radar WebKit Bug Importer]

<rdar://problem/44700000>

[Devin Rousso]

Created attachment 351813 [details]
[Patch] WIP

[Devin Rousso]

Created attachment 351861 [details]
[Patch] WIP

[Devin Rousso]

Created attachment 352025 [details]
Patch

Rebase and tests

[EWS Watchlist]

Comment on attachment 352025 [details]
Patch

Attachment 352025 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9530431

New failing tests:
http/tests/inspector/dom/didFireEvent.html

[EWS Watchlist]

Created attachment 352028 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 352025 [details]
Patch

Attachment 352025 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9530664

New failing tests:
http/tests/inspector/dom/didFireEvent.html

[EWS Watchlist]

Created attachment 352029 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 352025 [details]
Patch

Attachment 352025 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9531941

New failing tests:
http/tests/inspector/dom/didFireEvent.html

[EWS Watchlist]

Created attachment 352035 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Devin Rousso]

Created attachment 352062 [details]
Patch

[Devin Rousso]

Created attachment 352066 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 352066 [details]
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?

[Devin Rousso]

Comment on attachment 352066 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:144
>> +        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.

`_shouldListenForEventListeners` sounds like a good name.

>> 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++,
>     });

This is for the situation where an element and one of it's ancestors are listening for the same event:

#document (listening for "webkitfullscreenchange")
    <video> (listening for "webkitfullscreenchange")

If we cause <video> to go fullscreen, then both <video> and #document will get a "webkitfullscreenchange" event, but they will be slightly off in timing.  Furthermore, both events will be sent directly to <video>, since `EventFiredCallback` uses the event's absolute `target`, not it's `currentTarget` (this way we get the actual node that triggered the fullscreen, not the #document).

We don't want both events to appear for <video> (it would call `_addDOMEvent` for the #document event) since it really only received one of the events.  We also can't use the serial identifier since we are actually receiving two distinct event payloads.

I think a better solution may be to rework `EventFiredCallback` to keep track of the last event fired to ensure that it is never sent to the frontend more than once.

>> Source/WebInspectorUI/UserInterface/Views/DOMEventsBreakdownView.js:106
>> +        }, []);
> 
> This doesn't read any better than a for-loop to me. Is there any advantage to using reduce here?

No reason anymore.  I used to chain it with a `forEach`, but I have since reworked it.  I'll change it.

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

LOL I have no idea how this got here.  I might've accidentally added it when editing some of the blocking patches 😅

>> 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!

That was also probably my fault.  I hate english sometimes 😑

>> 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?

This was used when I was trying to figure out the debug WK1 issue.  I tried using a "mousedown" instead of "keydown".

[Devin Rousso]

Created attachment 352134 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 352134 [details]
Patch

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

New code looks good to me.

> Source/JavaScriptCore/inspector/protocol/DOM.json:670
> +                { "name": "data", "type": "object", "optional": true, "description": "Holds ancillary information about the event or its target." }

We'll want to update our ITMLKit friends.

[WebKit Commit Bot]

Comment on attachment 352134 [details]
Patch

Clearing flags on attachment: 352134

Committed r237431: <https://trac.webkit.org/changeset/237431>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.