Bug 159014 - Web Inspector: don't start auto capturing if the Inspector window is not visible
Summary: Web Inspector: don't start auto capturing if the Inspector window is not visible
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Brian Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-06-21 21:27 PDT by Brian Burg
Modified: 2016-06-23 11:25 PDT (History)
6 users (show)

See Also:


Attachments
Proposed Fix (4.46 KB, patch)
2016-06-21 22:15 PDT, Brian Burg
joepeck: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2016-06-21 21:27:00 PDT
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.
Comment 1 Brian Burg 2016-06-21 21:27:14 PDT
<rdar://problem/26931269>
Comment 2 Brian Burg 2016-06-21 22:09:42 PDT
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}
Comment 3 Brian Burg 2016-06-21 22:15:28 PDT
Created attachment 281815 [details]
Proposed Fix
Comment 4 Joseph Pecoraro 2016-06-22 11:54:45 PDT
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().
Comment 5 Brian Burg 2016-06-22 14:45:34 PDT
(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
Comment 6 Brian Burg 2016-06-22 15:05:54 PDT
Committed r202352: <http://trac.webkit.org/changeset/202352>
Comment 7 Brian Burg 2016-06-22 15:19:35 PDT
Committed r202353: <http://trac.webkit.org/changeset/202353>