Bug 189874

Summary: Web Inspector: display fullscreen enter/exit events in Timelines and Network node waterfalls
Product: WebKit Reporter: Devin Rousso <hi>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, dbates, esprehn+autocc, ews-watchlist, inspector-bugzilla-changes, joepeck, kangil.han, keith_miller, mark.lam, msaboff, rniwa, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 189773    
Bug Blocks: 190641, 191625    
Attachments:
Description Flags
[Patch] WIP
none
[Patch] WIP
none
Patch
none
Archive of layout-test-results from ews103 for mac-sierra
none
Archive of layout-test-results from ews116 for mac-sierra
none
Archive of layout-test-results from ews115 for mac-sierra
none
Patch
none
Patch
none
Patch none

Description Devin Rousso 2018-09-21 19:20:58 PDT
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.
Comment 1 Radar WebKit Bug Importer 2018-09-21 19:21:23 PDT
<rdar://problem/44700000>
Comment 2 Devin Rousso 2018-10-08 14:12:39 PDT
Created attachment 351813 [details]
[Patch] WIP
Comment 3 Devin Rousso 2018-10-08 23:01:33 PDT
Created attachment 351861 [details]
[Patch] WIP
Comment 4 Devin Rousso 2018-10-10 23:59:56 PDT
Created attachment 352025 [details]
Patch

Rebase and tests
Comment 5 EWS Watchlist 2018-10-11 01:05:27 PDT Comment hidden (obsolete)
Comment 6 EWS Watchlist 2018-10-11 01:05:29 PDT Comment hidden (obsolete)
Comment 7 EWS Watchlist 2018-10-11 02:05:44 PDT Comment hidden (obsolete)
Comment 8 EWS Watchlist 2018-10-11 02:05:46 PDT Comment hidden (obsolete)
Comment 9 EWS Watchlist 2018-10-11 04:02:31 PDT Comment hidden (obsolete)
Comment 10 EWS Watchlist 2018-10-11 04:02:33 PDT Comment hidden (obsolete)
Comment 11 Devin Rousso 2018-10-11 11:52:34 PDT
Created attachment 352062 [details]
Patch
Comment 12 Devin Rousso 2018-10-11 12:44:57 PDT
Created attachment 352066 [details]
Patch
Comment 13 Joseph Pecoraro 2018-10-11 19:00:35 PDT
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?
Comment 14 Devin Rousso 2018-10-11 19:20:17 PDT
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".
Comment 15 Devin Rousso 2018-10-11 21:28:36 PDT
Created attachment 352134 [details]
Patch
Comment 16 Joseph Pecoraro 2018-10-15 13:50:00 PDT
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.
Comment 17 WebKit Commit Bot 2018-10-25 15:59:37 PDT
Comment on attachment 352134 [details]
Patch

Clearing flags on attachment: 352134

Committed r237431: <https://trac.webkit.org/changeset/237431>
Comment 18 WebKit Commit Bot 2018-10-25 15:59:39 PDT
All reviewed patches have been landed.  Closing bug.