Bug 159014 – Web Inspector: don't start auto capturing if the Inspector window is not visible

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

After https://webkit.org/b/159004, we still sometimes can't hit a `debugger` statement from Automation.evaluateJavaScriptFunction. This happens if the Timelines tab was last visible in the previous Web Inspector session.

1. When Automation.inspectBrowsingContext runs, it opens up the Inspector, which restores the last shown tab.
2. TimelineTabContentView.shown() is called, and it set WebInspector.timelineManager.autoCaptureOnPageLoad = true
3. TimelineManager tells the backend to enable auto capturing
4. Shortly thereafter, the backend disables breakpoints and tells the frontend that auto capturing started.

I think it is easiest to alter TimelineTabContentView to not set autoCaptureOnPageLoad if WebInspector.visible is not true. Also it should listen to the VisibilityStateChanged notification and re-enable it when the page becomes visible.

[Blaze Burg]

<rdar://problem/26931269>

[Blaze Burg]

Here are some logs verifying the new behavior.

--

When restoring TimelineTabContentView in normal inspector:

TimelineTabContentView.js:282:20: CONSOLE LOG TimelineTabContentView.shown() this.visible:  true WebInspector.visible:  false
TimelineTabContentView.js:372:20: CONSOLE LOG inspectorVisibilityChanged this.visible:  true WebInspector.visible:  false
TimelineTabContentView.js:372:20: CONSOLE LOG inspectorVisibilityChanged this.visible:  true WebInspector.visible:  true
[native code]: CONSOLE LOG request: {"id":23,"method":"Timeline.setAutoCaptureEnabled","params":{"enabled":true}}

--

When restoring TimelineTabContentView with preloaded (not visible) inspector:

TimelineTabContentView.js:282:20: CONSOLE LOG TimelineTabContentView.shown() this.visible:  true WebInspector.visible:  false
TimelineTabContentView.js:372:20: CONSOLE LOG inspectorVisibilityChanged this.visible:  true WebInspector.visible:  false

[successfully paused at debugger statement]

--

When starting profiling via WK API / InspectorFrontendAPI:

[native code]: CONSOLE LOG request: {"id":25,"method":"Timeline.start"}
[native code]: CONSOLE LOG request: {"id":26,"method":"ScriptProfiler.startTracking","params":{"includeSamples":true}}
[native code]: CONSOLE LOG request: {"id":27,"method":"Memory.startTracking"}
/TimelineTabContentView.js:372:20: CONSOLE LOG inspectorVisibilityChanged this.visible:  undefined WebInspector.visible:  false

[Showing Web Inspector (non-Timeline tab restored)]

/TimelineTabContentView.js:372:20: CONSOLE LOG inspectorVisibilityChanged this.visible:  undefined WebInspector.visible:  true

[Switching to Timeline tab]

/TimelineTabContentView.js:282:20: CONSOLE LOG TimelineTabContentView.shown() this.visible:  true WebInspector.visible:  true
[native code]: CONSOLE LOG request: {"id":54,"method":"Timeline.setAutoCaptureEnabled","params":{"enabled":true}}
[native code]: CONSOLE LOG response: {"result":{},"id":54}

[Blaze Burg]

Created attachment 281815 [details]
Proposed Fix

[Joseph Pecoraro]

Comment on attachment 281815 [details]
Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=281815&action=review

r=me

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:138
> +        autoCapture = !!autoCapture;

I don't think this is useful.

> Source/WebInspectorUI/UserInterface/Views/TimelineTabContentView.js:73
> +        WebInspector.notifications.addEventListener(WebInspector.Notification.VisibilityStateDidChange, this._inspectorVisibilityChanged, this);

This, and all of these global event listeners (timelineManager) should be removed in a closed() method for this ContentView. Including a call to super.closed().

[Blaze Burg]

(In reply to comment #4)
> Comment on attachment 281815 [details]
> Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281815&action=review
> 
> r=me
> 
> > Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:138
> > +        autoCapture = !!autoCapture;
> 
> I don't think this is useful.

It was necessary at some point because ContentView.visible is undefined until it's true. We could coerce it outside of the manager class.

> 
> > Source/WebInspectorUI/UserInterface/Views/TimelineTabContentView.js:73
> > +        WebInspector.notifications.addEventListener(WebInspector.Notification.VisibilityStateDidChange, this._inspectorVisibilityChanged, this);
> 
> This, and all of these global event listeners (timelineManager) should be
> removed in a closed() method for this ContentView. Including a call to
> super.closed().

OK

[Blaze Burg]

Committed r202352: <http://trac.webkit.org/changeset/202352>

[Blaze Burg]

Committed r202353: <http://trac.webkit.org/changeset/202353>