159014 2016-06-21 21:27:00 -0700 Web Inspector: don't start auto capturing if the Inspector window is not visible 2016-06-23 11:25:25 -0700 1 1 1 Unclassified WebKit Web Inspector WebKit Nightly Build All All RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=159042 InRadar P2 Normal --- 1 bburg bburg bburg joepeck mattbaker nvasilyev timothy webkit-bug-importer oldest_to_newest 1204412 0 bburg 2016-06-21 21:27:00 -0700 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. 1204413 1 bburg 2016-06-21 21:27:14 -0700 <rdar://problem/26931269> 1204422 2 bburg 2016-06-21 22:09:42 -0700 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} 1204424 3 281815 bburg 2016-06-21 22:15:28 -0700 Created attachment 281815 Proposed Fix 1204605 4 281815 joepeck 2016-06-22 11:54:45 -0700 Comment on attachment 281815 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(). 1204678 5 bburg 2016-06-22 14:45:34 -0700 (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 1204688 6 bburg 2016-06-22 15:05:54 -0700 Committed r202352: <http://trac.webkit.org/changeset/202352> 1204690 7 bburg 2016-06-22 15:19:35 -0700 Committed r202353: <http://trac.webkit.org/changeset/202353> 281815 2016-06-21 22:15:28 -0700 2016-06-22 11:54:45 -0700 Proposed Fix bug-159014-20160621221520.patch text/plain 4566 bburg U3VidmVyc2lvbiBSZXZpc2lvbjogMjAyMTk4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViSW5zcGVj dG9yVUkvQ2hhbmdlTG9nIGIvU291cmNlL1dlYkluc3BlY3RvclVJL0NoYW5nZUxvZwppbmRleCBm ZDliOTc4NTM3OTA1ZTNkOGM5ODViM2Q2YjczMGIzMmViZWNmNjI0Li5kNjdmZjQ4M2NkZjMzODI1 NTg5ZThmZDUxMzJlYWI4ZGQ4YzQwYjczIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViSW5zcGVjdG9y VUkvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwyOSBAQAorMjAxNi0wNi0yMSAgQnJpYW4gQnVyZyAgPGJidXJnQGFwcGxlLmNvbT4KKwor ICAgICAgICBXZWIgSW5zcGVjdG9yOiBkb24ndCBzdGFydCBhdXRvIGNhcHR1cmluZyBpZiB0aGUg SW5zcGVjdG9yIHdpbmRvdyBpcyBub3QgdmlzaWJsZQorICAgICAgICBodHRwczovL2J1Z3Mud2Vi a2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTU5MDE0CisgICAgICAgIDxyZGFyOi8vcHJvYmxlbS8y NjkzMTI2OT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAg ICBUaW1lbGluZVRhYkNvbnRlbnRWaWV3IHNob3VsZCBub3QgdGVsbCB0aGUgdGltZWxpbmUgbWFu YWdlciB0byBlbmFibGUKKyAgICAgICAgYXV0by1jYXB0dXJpbmcgdW5sZXNzIHRoZSBXZWIgSW5z cGVjdG9yIFVJIGlzIHZpc2libGUuIElmIGl0IGlzIHByZWxvYWRlZAorICAgICAgICBidXQgbm90 IHNob3duIHRvIHRoZSB1c2VyLCB0aGVuIGF1dG8tY2FwdHVyaW5nIG1heSBpbmFkdmVydGVudGx5 IGRpc2FibGUKKyAgICAgICAgdGhlIGRlYnVnZ2VyLCBjYXVzaW5nIGl0IHRvIG1pc3MgYGRlYnVn Z2VyYCBzdGF0ZW1lbnRzIGFuZCBub3QgYnJpbmcgdGhlCisgICAgICAgIGluc3BlY3RvciB0byBm cm9udC4KKworICAgICAgICAqIFVzZXJJbnRlcmZhY2UvQ29udHJvbGxlcnMvVGltZWxpbmVNYW5h Z2VyLmpzOgorICAgICAgICAoV2ViSW5zcGVjdG9yLlRpbWVsaW5lTWFuYWdlci5wcm90b3R5cGUu c2V0IGF1dG9DYXB0dXJlT25QYWdlTG9hZCk6CisgICAgICAgIEJhaWwgb3V0IGlmIG5vdGhpbmcg Y2hhbmdlZC4gQ29lcmNlIHRvIGEgYm9vbGVhbiBzaW5jZSB0aGUgYmFja2VuZCByZXF1aXJlcyBh IGJvb2xlYW4uCisKKyAgICAgICAgKiBVc2VySW50ZXJmYWNlL1ZpZXdzL1RpbWVsaW5lVGFiQ29u dGVudFZpZXcuanM6CisgICAgICAgIChXZWJJbnNwZWN0b3IuVGltZWxpbmVUYWJDb250ZW50Vmll dyk6IExpc3RlbiBmb3IgVUkgdmlzaWJpbGl0eSBjaGFuZ2VzLgorICAgICAgICAoV2ViSW5zcGVj dG9yLlRpbWVsaW5lVGFiQ29udGVudFZpZXcucHJvdG90eXBlLnNob3duKToKKyAgICAgICAgRW5h YmxlIGF1dG8tY2FwdHVyaW5nIGlmIHRoZSBVSSBpcyB2aXNpYmxlLgorCisgICAgICAgIChXZWJJ bnNwZWN0b3IuVGltZWxpbmVUYWJDb250ZW50Vmlldy5wcm90b3R5cGUuX2luc3BlY3RvclZpc2li aWxpdHlDaGFuZ2VkKToKKyAgICAgICAgVXBkYXRlIHRoZSBhdXRvLWNhcHR1cmluZyBzZXR0aW5n IGlmIHRoZSBVSSBiZWNhbWUgdmlzaWJsZSBvciBub3QgdmlzaWJsZS4KKwogMjAxNi0wNi0xNyAg Q29tbWl0IFF1ZXVlICA8Y29tbWl0LXF1ZXVlQHdlYmtpdC5vcmc+CiAKICAgICAgICAgVW5yZXZp ZXdlZCwgcm9sbGluZyBvdXQgcjIwMjE1Mi4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJJbnNwZWN0 b3JVSS9Vc2VySW50ZXJmYWNlL0NvbnRyb2xsZXJzL1RpbWVsaW5lTWFuYWdlci5qcyBiL1NvdXJj ZS9XZWJJbnNwZWN0b3JVSS9Vc2VySW50ZXJmYWNlL0NvbnRyb2xsZXJzL1RpbWVsaW5lTWFuYWdl ci5qcwppbmRleCA0MjBjZGQxZWNkY2YzZjgyMmYyZGFlZTM5ZTNiZTAzZWM5ZWNkOTA3Li44Y2Q4 ZWM0NWNkZDgzN2EzM2EwZDRhYTE3ZjgwYmI4N2QzNWVhZTA0IDEwMDY0NAotLS0gYS9Tb3VyY2Uv V2ViSW5zcGVjdG9yVUkvVXNlckludGVyZmFjZS9Db250cm9sbGVycy9UaW1lbGluZU1hbmFnZXIu anMKKysrIGIvU291cmNlL1dlYkluc3BlY3RvclVJL1VzZXJJbnRlcmZhY2UvQ29udHJvbGxlcnMv VGltZWxpbmVNYW5hZ2VyLmpzCkBAIC0xMzUsMTAgKzEzNSwxNSBAQCBXZWJJbnNwZWN0b3IuVGlt ZWxpbmVNYW5hZ2VyID0gY2xhc3MgVGltZWxpbmVNYW5hZ2VyIGV4dGVuZHMgV2ViSW5zcGVjdG9y Lk9iamVjdAogCiAgICAgc2V0IGF1dG9DYXB0dXJlT25QYWdlTG9hZChhdXRvQ2FwdHVyZSkKICAg ICB7CisgICAgICAgIGF1dG9DYXB0dXJlID0gISFhdXRvQ2FwdHVyZTsKKworICAgICAgICBpZiAo dGhpcy5fYXV0b0NhcHR1cmVPblBhZ2VMb2FkID09PSBhdXRvQ2FwdHVyZSkKKyAgICAgICAgICAg IHJldHVybjsKKwogICAgICAgICB0aGlzLl9hdXRvQ2FwdHVyZU9uUGFnZUxvYWQgPSBhdXRvQ2Fw dHVyZTsKIAogICAgICAgICBpZiAod2luZG93LlRpbWVsaW5lQWdlbnQgJiYgVGltZWxpbmVBZ2Vu dC5zZXRBdXRvQ2FwdHVyZUVuYWJsZWQpCi0gICAgICAgICAgICBUaW1lbGluZUFnZW50LnNldEF1 dG9DYXB0dXJlRW5hYmxlZChhdXRvQ2FwdHVyZSk7CisgICAgICAgICAgICBUaW1lbGluZUFnZW50 LnNldEF1dG9DYXB0dXJlRW5hYmxlZCh0aGlzLl9hdXRvQ2FwdHVyZU9uUGFnZUxvYWQpOwogICAg IH0KIAogICAgIGdldCBlbmFibGVkVGltZWxpbmVUeXBlcygpCmRpZmYgLS1naXQgYS9Tb3VyY2Uv V2ViSW5zcGVjdG9yVUkvVXNlckludGVyZmFjZS9WaWV3cy9UaW1lbGluZVRhYkNvbnRlbnRWaWV3 LmpzIGIvU291cmNlL1dlYkluc3BlY3RvclVJL1VzZXJJbnRlcmZhY2UvVmlld3MvVGltZWxpbmVU YWJDb250ZW50Vmlldy5qcwppbmRleCAwOTM4OWFmNWRmOGNjNGE1YmRmZmY5NmJkNWQzYThmY2Mx NjE2NjUyLi41MTY4YjExM2NiNjE1OWQ1MTc4ZTkyZTFkZTZjMDhkYWQ2NTUwOGJmIDEwMDY0NAot LS0gYS9Tb3VyY2UvV2ViSW5zcGVjdG9yVUkvVXNlckludGVyZmFjZS9WaWV3cy9UaW1lbGluZVRh YkNvbnRlbnRWaWV3LmpzCisrKyBiL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2VySW50ZXJmYWNl L1ZpZXdzL1RpbWVsaW5lVGFiQ29udGVudFZpZXcuanMKQEAgLTcwLDYgKzcwLDggQEAgV2ViSW5z cGVjdG9yLlRpbWVsaW5lVGFiQ29udGVudFZpZXcgPSBjbGFzcyBUaW1lbGluZVRhYkNvbnRlbnRW aWV3IGV4dGVuZHMgV2ViSW4KICAgICAgICAgV2ViSW5zcGVjdG9yLnRpbWVsaW5lTWFuYWdlci5h ZGRFdmVudExpc3RlbmVyKFdlYkluc3BlY3Rvci5UaW1lbGluZU1hbmFnZXIuRXZlbnQuQ2FwdHVy aW5nU3RhcnRlZCwgdGhpcy5fY2FwdHVyaW5nU3RhcnRlZE9yU3RvcHBlZCwgdGhpcyk7CiAgICAg ICAgIFdlYkluc3BlY3Rvci50aW1lbGluZU1hbmFnZXIuYWRkRXZlbnRMaXN0ZW5lcihXZWJJbnNw ZWN0b3IuVGltZWxpbmVNYW5hZ2VyLkV2ZW50LkNhcHR1cmluZ1N0b3BwZWQsIHRoaXMuX2NhcHR1 cmluZ1N0YXJ0ZWRPclN0b3BwZWQsIHRoaXMpOwogCisgICAgICAgIFdlYkluc3BlY3Rvci5ub3Rp ZmljYXRpb25zLmFkZEV2ZW50TGlzdGVuZXIoV2ViSW5zcGVjdG9yLk5vdGlmaWNhdGlvbi5WaXNp YmlsaXR5U3RhdGVEaWRDaGFuZ2UsIHRoaXMuX2luc3BlY3RvclZpc2liaWxpdHlDaGFuZ2VkLCB0 aGlzKTsKKwogICAgICAgICB0aGlzLl9kaXNwbGF5ZWRSZWNvcmRpbmcgPSBudWxsOwogICAgICAg ICB0aGlzLl9kaXNwbGF5ZWRDb250ZW50VmlldyA9IG51bGw7CiAgICAgICAgIHRoaXMuX3ZpZXdN b2RlID0gbnVsbDsKQEAgLTI3Nyw3ICsyNzksOCBAQCBXZWJJbnNwZWN0b3IuVGltZWxpbmVUYWJD b250ZW50VmlldyA9IGNsYXNzIFRpbWVsaW5lVGFiQ29udGVudFZpZXcgZXh0ZW5kcyBXZWJJbgog ICAgICAgICB0aGlzLl90b2dnbGVSZWNvcmRpbmdTaG9ydGN1dC5kaXNhYmxlZCA9IGZhbHNlOwog ICAgICAgICB0aGlzLl90b2dnbGVOZXdSZWNvcmRpbmdTaG9ydGN1dC5kaXNhYmxlZCA9IGZhbHNl OwogCi0gICAgICAgIFdlYkluc3BlY3Rvci50aW1lbGluZU1hbmFnZXIuYXV0b0NhcHR1cmVPblBh Z2VMb2FkID0gdHJ1ZTsKKyAgICAgICAgaWYgKFdlYkluc3BlY3Rvci52aXNpYmxlKQorICAgICAg ICAgICAgV2ViSW5zcGVjdG9yLnRpbWVsaW5lTWFuYWdlci5hdXRvQ2FwdHVyZU9uUGFnZUxvYWQg PSB0cnVlOwogICAgIH0KIAogICAgIGhpZGRlbigpCkBAIC0zNjIsNiArMzY1LDExIEBAIFdlYklu c3BlY3Rvci5UaW1lbGluZVRhYkNvbnRlbnRWaWV3ID0gY2xhc3MgVGltZWxpbmVUYWJDb250ZW50 VmlldyBleHRlbmRzIFdlYkluCiAgICAgICAgIHRoaXMuX3JlY29yZEJ1dHRvbi50b2dnbGVkID0g aXNDYXB0dXJpbmc7CiAgICAgfQogCisgICAgX2luc3BlY3RvclZpc2liaWxpdHlDaGFuZ2VkKGV2 ZW50KQorICAgIHsKKyAgICAgICAgV2ViSW5zcGVjdG9yLnRpbWVsaW5lTWFuYWdlci5hdXRvQ2Fw dHVyZU9uUGFnZUxvYWQgPSAhIXRoaXMudmlzaWJsZSAmJiAhIVdlYkluc3BlY3Rvci52aXNpYmxl OworICAgIH0KKwogICAgIF90b2dnbGVSZWNvcmRpbmdPblNwYWNlYmFyKGV2ZW50KQogICAgIHsK ICAgICAgICAgaWYgKFdlYkluc3BlY3Rvci5pc0V2ZW50VGFyZ2V0QW5FZGl0YWJsZUZpZWxkKGV2 ZW50KSkK