| Summary: | Web Inspector: Start moving toward better multi-target support | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||
| Component: | Web Inspector | Assignee: | Joseph Pecoraro <joepeck> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | commit-queue, hi, inspector-bugzilla-changes, joepeck, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Attachments: |
|
||||||||
Created attachment 354057 [details]
[PATCH] Proposed Fix
I assume this won't apply, but I'll get it up for now.
Comment on attachment 354057 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=354057&action=review > Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:692 > + if (!InspectorBackend.domains.Debugger.setPauseOnAssertions) { Why not use `target.DebuggerAgent` here? > Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:65 > + if (target.NetworkAgent.setResourceCachingDisabled && WI.settings.resourceCachingDisabled && WI.settings.resourceCachingDisabled.value) The second check for `WI.settings.resourceCachingDisabled` is unnecessary as it always exists > Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:66 > + target.NetworkAgent.setResourceCachingDisabled(true); You could just call this function with the `WI.Setting`'s value instead of using `true`: if (target.NetworkAgent.setResourceCachingDisabled) target.NetworkAgent.setResourceCachingDisabled(WI.settings.resourceCachingDisabled.value); > Source/WebInspectorUI/UserInterface/Controllers/RuntimeManager.js:44 > + if (target.RuntimeAgent.enableTypeProfiler && WI.settings.showJavaScriptTypeInformation && WI.settings.showJavaScriptTypeInformation.value) Ditto (NetworkManager.js:65) > Source/WebInspectorUI/UserInterface/Controllers/RuntimeManager.js:48 > + if (target.RuntimeAgent.enableControlFlowProfiler && WI.settings.enableControlFlowProfiler && WI.settings.enableControlFlowProfiler.value) Ditto (NetworkManager.js:65) > Source/WebInspectorUI/UserInterface/Controllers/RuntimeManager.js:135 > + let executionContextId = this._activeExecutionContext.id; Considering that this is only used in one place, I'd be fine just inlining it there > Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:726 > + if (!window.TimelineAgent) Shouldn't this follow the `domains` approach? if (!InspectorBackend.domains.Timeline) > Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:1095 > + target.TimelineAgent.setInstruments([...instrumentSet]); `Array.from` should be faster/simpler than constructing an array using spread <https://jsperf.com/set-iterator-vs-foreach/4> > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:178 > + if (!InspectorBackend.domains.DOM.hasEvent("pseudoElementAdded")) While we're doing this change, is there no other protocol check we can perform for iOS9? Something in one of the "always available" domains? Perhaps that `RuntimeAgent.enableTypeProfiler` exists? > Source/WebInspectorUI/UserInterface/Protocol/Target.js:75 > + if (this.PageAgent.setShowPaintRects && WI.settings.showPaintRects.value) Ditto (NetworkManager.js:65) > Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:176 > + result.release(); Shouldn't we still release in the case that the `result.type !== "function"`? > Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:104 > + if (window.PageAgent && PageAgent.setShowRulers) Ditto (TimelineManager.js:726) if (InspectorBackend.domains.Page.setShowRulers) > Source/WebInspectorUI/UserInterface/Views/LogContentView.js:131 > + if (window.HeapAgent && HeapAgent.gc) Ditto (TimelineManager.js:726) if (InspectorBackend.domains.Heap.gc) Comment on attachment 354057 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=354057&action=review >> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:692 >> + if (!InspectorBackend.domains.Debugger.setPauseOnAssertions) { > > Why not use `target.DebuggerAgent` here? This is using `setPauseOnAssertions` as an agnostic feature check for "we are after iOS 10 or not". I think its more of an agnostic feature check then a per-target check. But I'll still switch to the target version here. >> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:66 >> + target.NetworkAgent.setResourceCachingDisabled(true); > > You could just call this function with the `WI.Setting`'s value instead of using `true`: > > if (target.NetworkAgent.setResourceCachingDisabled) > target.NetworkAgent.setResourceCachingDisabled(WI.settings.resourceCachingDisabled.value); Done. Some of these we probably just wanted to avoid a message to the backend if we know we were matching the default value, like sending false when the backend should be false. >> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:726 >> + if (!window.TimelineAgent) > > Shouldn't this follow the `domains` approach? > > if (!InspectorBackend.domains.Timeline) Ultimately for multi-target support this code will need to attempt to enable auto capture on one or more targets. So this will have to be revisited later. So I used `window.TargetAgent` to make it clear that we should revisit it. >> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:1095 >> + target.TimelineAgent.setInstruments([...instrumentSet]); > > `Array.from` should be faster/simpler than constructing an array using spread > <https://jsperf.com/set-iterator-vs-foreach/4> Done. >> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:178 >> + if (!InspectorBackend.domains.DOM.hasEvent("pseudoElementAdded")) > > While we're doing this change, is there no other protocol check we can perform for iOS9? Something in one of the "always available" domains? Perhaps that `RuntimeAgent.enableTypeProfiler` exists? This is a difference between 9.0 and 9.3 and later... So our options are limited (none in the big 3 Runtime/Debugger/Console). I'm going to keep this because it was the most accurate at the time. >> Source/WebInspectorUI/UserInterface/Protocol/Target.js:75 >> + if (this.PageAgent.setShowPaintRects && WI.settings.showPaintRects.value) > > Ditto (NetworkManager.js:65) I think here we are avoiding sending `false` to the backend when we know the backend should already be starting out `false`. >> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:176 >> + result.release(); > > Shouldn't we still release in the case that the `result.type !== "function"`? Agreed, I cleaned this up to: remoteObject.getProperty("constructor", (error, result, wasThrown) => { if (error) return; if (result.type === "function") remoteObject.target.DebuggerAgent.getFunctionDetails(result.objectId, didGetFunctionDetails); result.release(); }); >> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:104 >> + if (window.PageAgent && PageAgent.setShowRulers) > > Ditto (TimelineManager.js:726) > > if (InspectorBackend.domains.Page.setShowRulers) Same thing, regarding multi-target support. >> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:131 >> + if (window.HeapAgent && HeapAgent.gc) > > Ditto (TimelineManager.js:726) > > if (InspectorBackend.domains.Heap.gc) Same thing regarding multi-target support. Created attachment 354261 [details]
[PATCH] Proposed Fix
Comment on attachment 354261 [details]
[PATCH] Proposed Fix
r=me
Comment on attachment 354261 [details] [PATCH] Proposed Fix Rejecting attachment 354261 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 354261, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Source/WebInspectorUI/ChangeLog is not at the top of the file. Full output: https://webkit-queues.webkit.org/results/9920061 |
Start moving toward better multi-target support A world where we do: if (PageAgent.foo) PageAgent.foo(...); Is ambiguous in a multi-target world: • Which target is the `PageAgent.foo` command going to go to? • Which target is the `PageAgent.foo` feature check happening on? Is that important? Ultimately, the behavior that we would like to have is to always do: • target.PageAgent Then it is clear exactly which target we are operating on. Another possibility is that we don't care or have a Target, we just want to "feature check" (common for enums as well) if (FooAgent.hasEvent("someEvent")) ... support something else ... In these cases, we should have an agnostic way to just get the complete list of whatever the backend supports: InspectorBackend.domains.Foo.hasEvent("someEvent") The key part being: InspectorBackend.domains.<DomainName> --- For now we have hundreds of places in our code that are using unprefixed or window prefixed agents. As we bring up multi-target support we should start eliminating these. Currently the behavior is they use the "Main Target Connection". That ends up being fine right now as the main target is limited to being a Page or ServiceWorker debuggable. But as we move toward supporting multiple such targets each unprefixed access to "PageAgent", "DOMAgent", "NetworkAgent", etc will need to be explicit per-target.