[webkit-reviews] review denied: [Bug 200118] Web Inspector: Timelines: disable related agents when the tab is closed : [Attachment 374892] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<drousso at apple.com>'s request for review:
Bug 200118: Web Inspector: Timelines: disable related agents when the tab is
closed
https://bugs.webkit.org/show_bug.cgi?id=200118

Attachment 374892: Patch

https://bugs.webkit.org/attachment.cgi?id=374892&action=review




--- Comment #3 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 374892
  --> https://bugs.webkit.org/attachment.cgi?id=374892
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:2
69
> +	   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.