.
Created attachment 374892 [details] Patch
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Comment on attachment 374892 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374892&action=review This looks correct on the frontend. I have a few questions on the backend changes. > Source/JavaScriptCore/inspector/agents/InspectorHeapAgent.cpp:86 > + m_tracking = false; We already returned above if we are tracking so this seems wrong. > Source/WebCore/inspector/agents/InspectorMemoryAgent.cpp:84 > + m_tracking = false; > + > + ResourceUsageThread::removeObserver(this); This doesn't seem balanced. start/stopTracking handle the observer. If disable doesn't do anything while tracking then this seems unnecessary. > Source/WebCore/inspector/agents/InspectorMemoryAgent.cpp:-101 > - if (!m_enabled) > - return; Nice. Yeah this point shouldn't even be reached if we're enabled. > Source/WebCore/inspector/agents/InspectorTimelineAgent.cpp:-97 > - m_instrumentingAgents.setPersistentInspectorTimelineAgent(this); This change is confusing. The original idea behind a `persistent` agent was that it would always be around in InstrumentingAgents even if the frontend had not enabled (and I believe even if a frontend wasn't connected...) Maybe the `persistent` part is not needed anymore and this would be just like the other instrumenting agent pointers. > Source/WebInspectorUI/UserInterface/Controllers/HeapManager.js:26 > WI.HeapManager = class HeapManager extends WI.Object This can now have a warning comment at the top about multi-target support. We'll eventually need to get a snapshot from each target, and operate on HeapSnapshots per-target. > Source/WebInspectorUI/UserInterface/Controllers/HeapManager.js:76 > + HeapAgent.snapshot((error, timestamp, snapshotStringData) => { This for example either would need a target or would snapshot all targets in a multi-target world. > Source/WebInspectorUI/UserInterface/Views/HeapSnapshotInstanceDataGridNode.js:269 > + const objectGroup = undefined; > + WI.heapManager.getRemoteObject(this._node, objectGroup, (error, remoteObjectPayload) => { Interesting... this should probably have an object group, but it is the global object so maybe we don't care since it will likely be kept alive forever. Though global objects in isolated worlds are maybe getting leaked from this... only while Web Inspector is open.
Comment on attachment 374892 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374892&action=review >> Source/JavaScriptCore/inspector/agents/InspectorHeapAgent.cpp:86 >> + m_tracking = false; > > We already returned above if we are tracking so this seems wrong. I actually don't think we should have the earlier return (I probably forgot to remove this from one of my earlier iterations). If we `disable` while actively tracking, that SHOULD stop tracking. `disable` should take precedence over everything. >> Source/WebCore/inspector/agents/InspectorMemoryAgent.cpp:84 >> + ResourceUsageThread::removeObserver(this); > > This doesn't seem balanced. start/stopTracking handle the observer. If disable doesn't do anything while tracking then this seems unnecessary. See previous response. >> Source/WebCore/inspector/agents/InspectorTimelineAgent.cpp:-97 >> - m_instrumentingAgents.setPersistentInspectorTimelineAgent(this); > > This change is confusing. The original idea behind a `persistent` agent was that it would always be around in InstrumentingAgents even if the frontend had not enabled (and I believe even if a frontend wasn't connected...) > > Maybe the `persistent` part is not needed anymore and this would be just like the other instrumenting agent pointers. The only reason we had `persistent` was to support auto-capture whenever the main frame navigates and `console.profile`/`console.profileEnd`. If the Timelines tab isn't enabled, I don't think those should be either. I do think it could use a better name, like `inspectorTimelineAgent` and `activelyTrackingInspectorTimelineAgent`.
Created attachment 375336 [details] Patch
Created attachment 375455 [details] Patch
Comment on attachment 375455 [details] Patch r=me
Comment on attachment 375455 [details] Patch Clearing flags on attachment: 375455 Committed r248286: <https://trac.webkit.org/changeset/248286>
All reviewed patches have been landed. Closing bug.
<rdar://problem/53968929>