Summary: | Web Inspector: Add `CSS.setLayoutContextTypeChangedMode` for getting information about all layout contexts | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Patrick Angle <pangle> | ||||||||
Component: | Web Inspector | Assignee: | Patrick Angle <pangle> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bburg, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 226661 | ||||||||||
Attachments: |
|
Created attachment 419615 [details]
Patch v1.0
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features. Comment on attachment 419615 [details] Patch v1.0 View in context: https://bugs.webkit.org/attachment.cgi?id=419615&action=review r=me, nice tests :) > Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:940 > + for (auto* child = node.firstChild(); child; child = child->nextSibling()) Are we sure that there's not a more efficient or less intense way to find all the grid contexts other than to walk the entire DOM tree? :( Maybe we could use `childrenOfType<Element>(node)` instead so that we only even attempt to instrument elements (since `Node` cannot be a grid context)? > Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:959 > + for (auto document : domAgent->documents()) NIT: `auto*` > Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:86 > + WI.assumingMainTarget().CSSAgent.setLayoutContextTypeChangedMode(WI.CSSManager.LayoutContextTypeChangedMode.All); While it is a valid assumption (for now) that only the main target has a `CSS` agent, in order to make this code more future-friendly we should iterate `WI.targets`. Also, I'd move this to a `WI.CSSManager.prototype.set layoutContextTypeChangedMode` so that protocol stuff is kept outside of view classes :) CSSManager.js ``` set layoutContextTypeChangedMode(layoutContextTypeChangedMode) { for (let target of WI.targets) { // COMPATIBILITY (iOS 14.5): CSS.setLayoutContextTypeChangedMode did not exist if (target.hasCommand("CSS.setLayoutContextTypeChangedMode")) target.CSSagent.setLayoutContextTypeChangedMode(layoutContextTypeChangedMode); } } ``` LayoutDetailsSidebarPanel.js ``` attached() { super.attached(); WI.Frame.addEventListener(WI.Frame.Event.MainResourceDidChange, this._mainResourceDidChange, this); WI.cssManager.layoutContextTypeChangedMode = WI.CSSManager.LayoutContextTypeChangedMode.All); } ``` > Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:93 > + WI.assumingMainTarget().CSSAgent.setLayoutContextTypeChangedMode(WI.CSSManager.LayoutContextTypeChangedMode.Observed); ditto (:86) > LayoutTests/inspector/css/setLayoutContextTypeChangedMode-expected.txt:11 > +PASS: 0 grid node should be instrumented. > +PASS: 1 grid nodes should be instrumented. > +PASS: 0 grid nodes should be instrumented. > +PASS: 1 grid nodes should be instrumented. > +PASS: 0 grid nodes should be instrumented. > +PASS: 1 grid nodes should be instrumented. these should be "0 grid nodes" (plural) and "1 grid node" (singular) and "2 grid nodes" (plural) 😅 (I feel bad for being this pedantic, but it just sticks out so much to my eyes 🤣) > LayoutTests/inspector/css/setLayoutContextTypeChangedMode.html:22 > + // TODO: [PCA] Use Razvan's impl in WI.DOMManager instead of redeclaring it. oops? > LayoutTests/inspector/css/setLayoutContextTypeChangedMode.html:37 > + documentNode.querySelector("#queryGrid"), Just to be extra sure/careful, can we pull this into a `let queryGridNode = ...` and then do `queryGridNode.awaitEvent(WI.DOMNode.Event.LayoutContextTypeChanged)` so that we know for a fact it's the right node being instrumented/updated? > LayoutTests/inspector/css/setLayoutContextTypeChangedMode.html:43 > + changeElementDisplayValue("queryGrid", "block"), NIT: to assist someone trying to read the output of the test, I'd suggest putting some `InspectorTest.log("Changing '#queryGrid' to 'display: block;'...");` (we usually use "..." to represent something that's about to happen whereas "." represents something that has happened) in between each of these checks :) > LayoutTests/inspector/css/setLayoutContextTypeChangedMode.html:75 > + WI.assumingMainTarget().CSSAgent.setLayoutContextTypeChangedMode(WI.CSSManager.LayoutContextTypeChangedMode.All), ditto (LayoutDetailsSidebarPanel.js:86) > LayoutTests/inspector/css/setLayoutContextTypeChangedMode.html:81 > + await WI.assumingMainTarget().CSSAgent.setLayoutContextTypeChangedMode(WI.CSSManager.LayoutContextTypeChangedMode.Observed); ditto (LayoutDetailsSidebarPanel.js:86) > LayoutTests/inspector/css/setLayoutContextTypeChangedMode.html:130 > + WI.domManager.requestDocument().then((doc) => { NIT: now that `WI.DOMManager.prototype.requestDocument` supports returning (and caching) a `Promise`, we should move away from this pattern and instead have each test that needs the `documentNode` fetch it itself Created attachment 419658 [details]
Patch v1.1 - Review Notes, More Verbose Test
Created attachment 419666 [details]
Patch v1.2 - Added reviewer to changelogs
Committed r272566: <https://commits.webkit.org/r272566> All reviewed patches have been landed. Closing bug and clearing flags on attachment 419666 [details]. |
From Devin's Review Comment on attachment 419315 [details]: ``` As a potential optimization, maybe you could add a `CSS.setGlobalLayoutContextTypeInstrumentationEnabled` that's toggled on inside `WI.LayoutDetailsSidebarPanel.prototype.attached` and off inside `WI.LayoutDetailsSidebarPanel.prototype.detached` which controls whether the `Node` is always instrumented (when toggled on) or if only previously instrumented `Node` are allowed (when toggled off). This means that until the developer shows the Layout panel, we'd only be getting `CSS.nodeLayoutContextTypeChanged` events for nodes that have been shown in the Elements Tab, which is likely _far_ more performant than if we'd instrumented every potentially viable node. ```