163407 2016-10-13 15:16:53 -0700 Web Inspector: Call RuntimeAgent.disableControlFlowProfiler when Code Coverage Profiler is turned off 2016-11-10 13:55:35 -0800 1 1 1 Unclassified WebKit Web Inspector WebKit Nightly Build All All RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=163408 InRadar P2 Normal --- 1 nvasilyev nvasilyev bburg commit-queue joepeck mattbaker nvasilyev saam timothy webkit-bug-importer oldest_to_newest 1239974 0 nvasilyev 2016-10-13 15:16:53 -0700 Disabling Code Coverage profiler by clicking [C] button should signal the backend to stop collecting the data by calling RuntimeAgent.disableControlFlowProfiler method. RuntimeAgent.disableControlFlowProfiler is currently unused. 1239975 1 webkit-bug-importer 2016-10-13 15:17:09 -0700 <rdar://problem/28764230> 1249875 2 294390 nvasilyev 2016-11-10 11:28:02 -0800 Created attachment 294390 Patch 1249877 3 294390 joepeck 2016-11-10 11:30:56 -0800 Comment on attachment 294390 Patch It is unfortunate that we don't have any tests for these methods. Given the Type Profiler chance is probably going to be identical, is there any reason this is split into two bugs? 1249883 4 nvasilyev 2016-11-10 11:39:55 -0800 > Given the Type Profiler chance is probably going to be identical, is there > any reason this is split into two bugs? Yes, I have concerns about Type Profiler https://bugs.webkit.org/show_bug.cgi?id=163408#c2 1249895 5 nvasilyev 2016-11-10 11:53:54 -0800 (In reply to comment #3) > Comment on attachment 294390 [details] > Patch > > It is unfortunate that we don't have any tests for these methods. Should we test if RuntimeAgent.enableControlFlowProfiler() and RuntimeAgent.disableControlFlowProfiler() gets called on clicking the [C] icon? I haven't written any UI tests that use view wrappers, but I expect it to be feasible. 1249896 6 joepeck 2016-11-10 11:56:24 -0800 (In reply to comment #5) > (In reply to comment #3) > > Comment on attachment 294390 [details] > > Patch > > > > It is unfortunate that we don't have any tests for these methods. > > Should we test if RuntimeAgent.enableControlFlowProfiler() and > RuntimeAgent.disableControlFlowProfiler() gets called on clicking the [C] > icon? We have no UI tests right now. I mean we don't even have tests for these protocol methods to ensure the backend does what we expect. 1249952 7 294390 commit-queue 2016-11-10 13:34:06 -0800 Comment on attachment 294390 Patch Clearing flags on attachment: 294390 Committed r208561: <http://trac.webkit.org/changeset/208561> 1249953 8 commit-queue 2016-11-10 13:34:10 -0800 All reviewed patches have been landed. Closing bug. 1249962 9 saam 2016-11-10 13:48:16 -0800 (In reply to comment #0) > Disabling Code Coverage profiler by clicking [C] button should signal the > backend to stop collecting the data by calling > RuntimeAgent.disableControlFlowProfiler method. > > RuntimeAgent.disableControlFlowProfiler is currently unused. The downside of doing this is that if you ever want to toggle the button, you will loose information. What I did with the type profiler is ensure that the button was just a local toggle for the UI, not the actual JSC type profiler. That way, if a user wants to show and then hide and then show the UI, the user wouldn't loose type information. What's the motivation for not following this design here? (Note that I still think we should turn the profilers off when recording a timeline) 1249964 10 nvasilyev 2016-11-10 13:55:35 -0800 (In reply to comment #9) > (In reply to comment #0) > > Disabling Code Coverage profiler by clicking [C] button should signal the > > backend to stop collecting the data by calling > > RuntimeAgent.disableControlFlowProfiler method. > > > > RuntimeAgent.disableControlFlowProfiler is currently unused. > > The downside of doing this is that if you ever want to toggle the button, > you will loose information. What I did with the type profiler is ensure that > the button was just a local toggle for the UI, not the actual JSC type > profiler. That way, if a user wants to show and then hide and then show the > UI, the user wouldn't loose type information. > > What's the motivation for not following this design here? > > (Note that I still think we should turn the profilers off when recording a > timeline) Here's a use case: 1. Turn Code Coverage off and on. 2. Perform some action on a web page. 3. See what got executed. Previously, we wouldn't be able to tell if the code was executed before performing that action or not. Now, turning off and on allows us to start with a clean slate. 294390 2016-11-10 11:28:02 -0800 2016-11-10 13:34:06 -0800 Patch patch.txt text/plain 1625 nvasilyev ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cgYi9Tb3VyY2UvV2Vi SW5zcGVjdG9yVUkvQ2hhbmdlTG9nCmluZGV4IDYwODM4ZTAuLmFmOTQwMjIgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkluc3BlY3Rv clVJL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2IEBACisyMDE2LTExLTEwICBOaWtpdGEgVmFzaWx5 ZXYgIDxudmFzaWx5ZXZAYXBwbGUuY29tPgorCisgICAgICAgIFdlYiBJbnNwZWN0b3I6IENhbGwg UnVudGltZUFnZW50LmRpc2FibGVDb250cm9sRmxvd1Byb2ZpbGVyIHdoZW4gQ29kZSBDb3ZlcmFn ZSBQcm9maWxlciBpcyB0dXJuZWQgb2ZmCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD0xNjM0MDcKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzI4NzY0MjMw PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFR1cm5p bmcgQ29kZSBDb3ZlcmFnZSBvZmYgYnkgY2xpY2tpbmcgW0NdIGljb24gbm93IGRpc2FibGVzIGl0 cyBiYWNrZW5kIGJ5IGNhbGxpbmcgUnVudGltZUFnZW50LmRpc2FibGVDb250cm9sRmxvd1Byb2Zp bGVyKCkuCisKKyAgICAgICAgKiBVc2VySW50ZXJmYWNlL1ZpZXdzL1NvdXJjZUNvZGVUZXh0RWRp dG9yLmpzOgorICAgICAgICAoV2ViSW5zcGVjdG9yLlNvdXJjZUNvZGVUZXh0RWRpdG9yLnByb3Rv dHlwZS5zZXQgX2Jhc2ljQmxvY2tBbm5vdGF0b3JFbmFibGVkKToKKwogMjAxNi0xMS0xMCAgQWFy b24gQ2h1ICA8YWFyb25fY2h1QGFwcGxlLmNvbT4KIAogICAgICAgICBXZWIgSW5zcGVjdG9yOiBB WEk6IGNsYXJpZnkgYnV0dG9uIHJvbGVzIChlLmcuIHRvZ2dsZSBvciBwb3B1cCBidXR0b24pCmRp ZmYgLS1naXQgYS9Tb3VyY2UvV2ViSW5zcGVjdG9yVUkvVXNlckludGVyZmFjZS9WaWV3cy9Tb3Vy Y2VDb2RlVGV4dEVkaXRvci5qcyBiL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2VySW50ZXJmYWNl L1ZpZXdzL1NvdXJjZUNvZGVUZXh0RWRpdG9yLmpzCmluZGV4IDA2NWI4NTEuLjBjNDI0NWIgMTAw NjQ0Ci0tLSBhL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2VySW50ZXJmYWNlL1ZpZXdzL1NvdXJj ZUNvZGVUZXh0RWRpdG9yLmpzCisrKyBiL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2VySW50ZXJm YWNlL1ZpZXdzL1NvdXJjZUNvZGVUZXh0RWRpdG9yLmpzCkBAIC0xOTA5LDYgKzE5MDksNyBAQCBX ZWJJbnNwZWN0b3IuU291cmNlQ29kZVRleHRFZGl0b3IgPSBjbGFzcyBTb3VyY2VDb2RlVGV4dEVk aXRvciBleHRlbmRzIFdlYkluc3BlYwogICAgICAgICAgICAgaWYgKCF0aGlzLl9jb250cm9sRmxv d1Njcm9sbEhhbmRsZXIpCiAgICAgICAgICAgICAgICAgdGhpcy5fZW5hYmxlU2Nyb2xsRXZlbnRz Rm9yQ29udHJvbEZsb3dBbm5vdGF0b3IoKTsKICAgICAgICAgfSBlbHNlIHsKKyAgICAgICAgICAg IFJ1bnRpbWVBZ2VudC5kaXNhYmxlQ29udHJvbEZsb3dQcm9maWxlcigpOwogICAgICAgICAgICAg dGhpcy5fYmFzaWNCbG9ja0Fubm90YXRvci5jbGVhcigpOwogCiAgICAgICAgICAgICBpZiAodGhp cy5fY29udHJvbEZsb3dTY3JvbGxIYW5kbGVyKQo=