Summary: | Web Inspector: display fullscreen enter/exit events in Timelines and Network node waterfalls | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||||||||||||||||
Component: | Web Inspector | Assignee: | 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
Devin Rousso
2018-09-21 19:20:58 PDT
Created attachment 351813 [details]
[Patch] WIP
Created attachment 351861 [details]
[Patch] WIP
Created attachment 352025 [details]
Patch
Rebase and tests
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 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
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 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
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 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
Created attachment 352062 [details]
Patch
Created attachment 352066 [details]
Patch
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 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". Created attachment 352134 [details]
Patch
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 on attachment 352134 [details] Patch Clearing flags on attachment: 352134 Committed r237431: <https://trac.webkit.org/changeset/237431> All reviewed patches have been landed. Closing bug. |