This bug is for tracking all the work needed to remove the old debugger in favor of this new debugger. There are a few specific modes like Browser Toolbox debugging and add-on debugging which we should investigate what is required to support. In bug 1300861 we enabled the new debugger in Nightly only; it was not ready for production and was missing certain features. In bug 1294139 the new debugger is being turned on for all channels (MVP); it is still missing a few features but is generally ready for broader use.

Jason, Do you have an update about getting rid of the old debugger? Any plan in mind? There is no blocker for this bug, but plenty on bug 1294139. Should we fix some of its blockers before proceeding? Do you know about people still using the old one?

Harald, Any opinion? Do you know about any telemetry data that could help us take a decison?

I will check with the telemetry team on if we report the changed pref back to telemetry. I have not seen any regression as dependencies on bug 1294139 are not regressions, so feel confident that we can move forward with this bug. We are planning to reach out internally to make everybody aware and to capture any concerns with removing the code.

Given that we have no blockers filed on the old debugger and with 58 improving the biggest performance issues, this LGTM. Jason reported from talking to platform engineers that there are happy with the new debugger and we have no reports of engineers threatening to stay behind on the old debugger.

Taking it on to send out an announcement, will update the bug with the results.

Any news on this Harald? Can we close this? I'll at least downgrade the priority since all the work is essentially done.

Mentions of users turning the new debugger interface off to get event breakpoints https://github.com/devtools-html/debugger.html/issues/4750#issuecomment-345787371 https://news.ycombinator.com/item?id=15796379 Looks like we should wait for https://github.com/devtools-html/debugger.html/issues/4750 to land, otherwise we break workflows for platform devs.

Note that once removed, we will be able to remove the two followin JSM, as only old debugger uses them: devtools/client/shared/widgets/SimpleListWidget.jsm devtools/client/shared/widgets/BreadcrumbsWidget.jsm

Honza, Harald, can we get some prioritization done on this? Our 2018 planning does contain "debugger parity" in there. In particular we said we should try to get break on DOM/XHR/... done this year. I believe this is also a parity item (with both chrome and our old debugger) that would fit nicely in our 2018 plan. So far, the idea was that after we'd done some of the stackframe componentization work, we'd be able to take this on. Is this your understanding too? Do we have a firm plan for acting on this plan before end of year?

:jlaster, can you confirm that bug 1470037 is the only test migration and that it is blocking the removal?

Attaching a work-in-progress patch. This patch removes the old debugger, promotes the new debugger, removes the pref and all usages of the prefs, and a few other things. It's probably too aggressive for a single patch but I wanted to post it here anyway as a WIP.

1314057-remove-framework: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d900d925fd336e2f5712d90af3580e818448b33

"1314057-remove-framework" is very simple in that it removes the "new-debugger-frontend" pref when it's set to true, because that's the default.

1314057-remove-framework-old-debugger: Fixes framework tests which required the old debugger

1314057-webconsole-1: https://treeherder.mozilla.org/#/jobs?repo=try&revision=762a5b47a016b9a46973c0227e7b20274a57881c

Comment on attachment 9007400 [details] [diff] [review] 1314057-remove-framework-old-debugger.patch Review of attachment 9007400 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. One small thing, is that I'd like to see us re-use some helpers ::: devtools/client/framework/test/browser_toolbox_view_source_01.js @@ +11,5 @@ > var URL = `${URL_ROOT}doc_viewsource.html`; > var JS_URL = `${URL_ROOT}code_math.js`; > > +function assertSelectedLocation(debuggerPanel, line, column) { > + const location = debuggerPanel._selectors.getSelectedLocation(debuggerPanel._getState()); can we save this helper in a head.js file in this directory? It would be nice to share some of these utilities.

Comment on attachment 9008155 [details] [diff] [review] 1314057-webconsole-1.patch Review of attachment 9008155 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. One minor nit, otherwise great. ::: devtools/client/webconsole/test/mochitest/browser_webconsole_object_inspector_while_debugging_and_inspecting.js @@ +67,3 @@ > > + info("Waiting for debugger to pause"); > + const dbg = createDebuggerContext(toolbox); minor detail, but it would be nice to declare one dbg per test. could we set this variable at the top of the test and pass dbg into `pauseDebugger(dbg)`

1314057-canvsdebugger: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfdce10e0f33e583108c44caa9527dd8c8f343a6

Comment on attachment 9008510 [details] [diff] [review] 1314057-canvsdebugger.patch Review of attachment 9008510 [details] [diff] [review]: ----------------------------------------------------------------- Lets try and move these assertions to a shared helper. One reason that is helpful is as at somepoint we will want to change the panel, and i'd like to minimize the number of dependencies on fields like _store. This is another reason for using createDebuggerContext

Please check in patches 1-4 and leave the bug open.

Comment on attachment 9009286 [details] [diff] [review] 1314057-5.patch Review of attachment 9009286 [details] [diff] [review]: ----------------------------------------------------------------- Nice work!

Pushed by btara@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f34f4f048480 Remove new-debugger-frontend flag from framework devtool r=jlast https://hg.mozilla.org/integration/mozilla-inbound/rev/b797675da9c1 Remove new-debugger-frontend flag from framework devtool when not defaulted to true r=jlast https://hg.mozilla.org/integration/mozilla-inbound/rev/b356beaa690f Remove unnecessary debugger pref declarations, fix browser_webconsole_object_inspector_while_debugging_and_inspecting test r=jlast https://hg.mozilla.org/integration/mozilla-inbound/rev/f992a076c6f2 Update canvasdebugger to use the new debugger r=jlast https://hg.mozilla.org/integration/mozilla-inbound/rev/1d626fdff4f3 Remove new-debugger-frontend usage from webconsole tests r=jlast

Backed out 5 changesets (Bug 1314057) for browser_webconsole_split.js failures. Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=0fed430fe00c2c984a6a2c6cb4be30f62fe53baa Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/be14b8d9e4cc3acc1dc642115e818d6a806fd71f Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=199432261&repo=mozilla-inbound&lineNumber=3782 [task 2018-09-15T00:21:33.649Z] 00:21:33 INFO - TEST-START | devtools/client/webconsole/test/mochitest/browser_webconsole_split.js [task 2018-09-15T00:21:33.650Z] 00:21:33 INFO - TEST-INFO | started process screentopng [task 2018-09-15T00:21:34.043Z] 00:21:34 INFO - TEST-INFO | screentopng: exit 0 [task 2018-09-15T00:21:34.045Z] 00:21:34 INFO - TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/mochitest/browser_webconsole_split.js | Exception thrown - at chrome://mochitests/content/browser/devtools/client/webconsole/test/mochitest/browser_webconsole_split.js:1 - SyntaxError: redeclaration of non-configurable global property Toolbox [task 2018-09-15T00:21:34.047Z] 00:21:34 INFO - GECKO(3448) | MEMORY STAT | vsize 2487MB | residentFast 491MB | heapAllocated 217MB [task 2018-09-15T00:21:34.048Z] 00:21:34 INFO - TEST-OK | devtools/client/webconsole/test/mochitest/browser_webconsole_split.js | took 47ms

Please checkin 1-5 and leave the bug open.

Pushed by nbeleuzu@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/059a19d11454 Remove new-debugger-frontend flag from framework devtool. r=jlast https://hg.mozilla.org/integration/mozilla-inbound/rev/1b274560201f Remove new-debugger-frontend flag from framework devtool when not defaulted to true. r=jlast https://hg.mozilla.org/integration/mozilla-inbound/rev/7691cbfeac6a Remove unnecessary debugger pref declarations, fix browser_webconsole_object_inspector_while_debugging_and_inspecting test. r=jlast https://hg.mozilla.org/integration/mozilla-inbound/rev/cdb30006eb31 Update canvasdebugger to use the new debugger. r=jlast https://hg.mozilla.org/integration/mozilla-inbound/rev/9bb3fd62313e Remove new-debugger-frontend usage from webconsole tests. r=jlast

Backed out for devtools failures on browser_browser_toolbox_debugger.js Backout link: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed,busted,exception,runnable Push link: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed,busted,exception,runnable Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=199611466&repo=mozilla-inbound&lineNumber=2287

There were also [TV3] failures on browser_webconsole_optimized_out_vars.js Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=199611270&repo=mozilla-inbound&lineNumber=5730

Backout by nbeleuzu@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/02d8dc7e145e Backed out 5 changesets for devtools failures on browser_browser_toolbox_debugger.js . CLOSED TREE

Pushed by btara@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5f6bd58bf00c Remove new-debugger-frontend flag from framework devtool r=davidwalsh https://hg.mozilla.org/integration/mozilla-inbound/rev/2ba943d7458d Remove new-debugger-frontend flag from framework devtool when not defaulted to true r=davidwalsh https://hg.mozilla.org/integration/mozilla-inbound/rev/3bacabb31961 Remove unnecessary debugger pref declarations, fix browser_webconsole_object_inspector_while_debugging_and_inspecting test r=davidwalsh https://hg.mozilla.org/integration/mozilla-inbound/rev/a026c2283c4e Update canvasdebugger to use the new debugger r=davidwalsh https://hg.mozilla.org/integration/mozilla-inbound/rev/b69b81f1aaf5 Remove new-debugger-frontend usage from webconsole tests r=davidwalsh

https://hg.mozilla.org/mozilla-central/rev/5f6bd58bf00c https://hg.mozilla.org/mozilla-central/rev/2ba943d7458d https://hg.mozilla.org/mozilla-central/rev/3bacabb31961 https://hg.mozilla.org/mozilla-central/rev/a026c2283c4e https://hg.mozilla.org/mozilla-central/rev/b69b81f1aaf5

Please checkin patch 6. Thank you!

Please checkin patch 7 as well. Thank you!

removing checkin-needed until all patches are reviewed. Is that okay with you David?

Sorry :apavel! All are now reviewed but only 6 and 7 must go in. Thank you!

Ah, no worries, landing them now. Thanks for clarifying this!

Pushed by apavel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c166d6c8d2a9 Remove pretty-print and blackboxing tests no longer relevant to new debugger r=jlast https://hg.mozilla.org/integration/mozilla-inbound/rev/bad602c832be Remove old debugger's breakpoints-button, parser, and scripts-switching tests r=jlast

https://hg.mozilla.org/mozilla-central/rev/c166d6c8d2a9 https://hg.mozilla.org/mozilla-central/rev/bad602c832be

Comment on attachment 9012188 [details] [diff] [review] 1314057-08.patch Review of attachment 9012188 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/debugger/test/mochitest/browser_dbg_listaddons.js @@ -1,1 @@ > -/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */ You should not remove this test, but instead move it somewhere else. See bug 1470037 for more info. I tried to verify that in your previous patches, but please review the list in this bug to ensure these tests still exist.

Thank you for the reminder!

Please don't forget to remove the old debugger images and related CSS as well: devtools/client/themes/images/debugger-step-in.svg devtools/client/themes/images/debugger-step-out.svg devtools/client/themes/images/debugger-step-over.png devtools/client/themes/images/debugger-step-over.svg devtools/client/themes/images/debugger-step-over@2x.png devtools/client/themes/images/debugger-toggleBreakpoints.svg devtools/client/themes/images/breakpoint.svg devtools/client/themes/debugger.css devtools/client/sourceeditor/codemirror/old-debugger.css Note that some of the images are still used in the canvas debugger, but you can change the image paths in the canvas debugger to point to the new debugger images, which allows you to get rid of the old images.

Thank you for letting me know Tim! Can I ask how you spotted this so that I can better identify those assets next time?

(In reply to David Walsh :davidwalsh from comment #60) > Thank you for letting me know Tim! Can I ask how you spotted this so that I > can better identify those assets next time? You can manually scan those 2 directories for theme code: devtools/client/themes/ devtools/client/themes/images/ Searching the each of the filenames on https://searchfox.org can also reveal unused code related to the file. Aside, I also just noticed that the old debugger is the last consumer of the grouping and checkbox grouping feature of SideMenuWidget.jsm (notably in the sources and events view), so the JS code (from SideMenuWidget.jsm and related tests) and related CSS code [1] can be removed as well. There might be more stuff from shared UI widgets [2] to remove, I think it's worth giving a look at the old debugger UI, and checking if any usages of those widgets have some UI pattern that's only found in the old debugger. [1]: https://searchfox.org/mozilla-central/search?q=.side-menu-widget-group&case=false&regexp=false&path= [2]: https://searchfox.org/mozilla-central/source/devtools/client/shared/widgets

Comment on attachment 9009498 [details] [diff] [review] 1314057-1.patch Just setting the checkin+ flag to indicate that this has already been checked-in.

Comment on attachment 9013119 [details] [diff] [review] 1314057-11.patch Review of attachment 9013119 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/debugger/test/mochitest/browser_dbg_function-display-name.js @@ -5,5 @@ > - > -/** > - * Tests that anonymous functions appear in the stack frame list with either > - * their displayName property or a SpiderMonkey-inferred name. > - */ we don't need this test because it is tested on the engine level.

Please check-in 8-11. Thank you!

Pushed by nbeleuzu@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0e50b65dc5e6 Remove search and breadcrumb tests for old debugger. r=jlast https://hg.mozilla.org/integration/mozilla-inbound/rev/4bff01e1bc91 Remove unnecessary puase tests for debugger removal. r=jlast https://hg.mozilla.org/integration/mozilla-inbound/rev/0ae9f4ff48d9 Remove stack tests which we have equivalents for in new debugger. r=jlast https://hg.mozilla.org/integration/mozilla-inbound/rev/1e22371f8221 Remove tests not relevant to the new debugger. r=jlast

https://hg.mozilla.org/mozilla-central/rev/0e50b65dc5e6 https://hg.mozilla.org/mozilla-central/rev/4bff01e1bc91 https://hg.mozilla.org/mozilla-central/rev/0ae9f4ff48d9 https://hg.mozilla.org/mozilla-central/rev/1e22371f8221

Please checkin patches 12 and 13 -- thank you!

Pushed by dluca@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e503bf1a29c5 Remove old debugger tests for breakpoints where already covered by new debugger r=jlast https://hg.mozilla.org/integration/mozilla-inbound/rev/61875633b2c2 Remove var viewer tests; remove sources tests r=jlast

https://hg.mozilla.org/mozilla-central/rev/e503bf1a29c5 https://hg.mozilla.org/mozilla-central/rev/61875633b2c2

https://hg.mozilla.org/mozilla-central/rev/e503bf1a29c5 https://hg.mozilla.org/mozilla-central/rev/61875633b2c2

Comment on attachment 9014144 [details] [diff] [review] 1314057-14.patch Review of attachment 9014144 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, but i think the test should be in a different directory such as a client/shared or toolbox directory

Comment on attachment 9014222 [details] [diff] [review] 1314057-15.patch Review of attachment 9014222 [details] [diff] [review]: ----------------------------------------------------------------- looks good. but should be in a different directory

Comment on attachment 9014480 [details] [diff] [review] 1314057-17.patch Review of attachment 9014480 [details] [diff] [review]: ----------------------------------------------------------------- i think it should be in a different directory

Here's a try run for 14-17: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6388f9feede865aea95caf664b1437b9d4cb9cba&selectedJob=204098762 Green!

Please checkin patches 14-17. Thank you!

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/241f876d557f Move globalactor and global method override tests. r=loganfsmyth https://hg.mozilla.org/integration/mozilla-inbound/rev/fa0d46e77437 Move listtabs and listaddons tests to shared. r=loganfsmyth https://hg.mozilla.org/integration/mozilla-inbound/rev/90480b4b4c43 Remove duplicated test between old and new debugger. r=loganfsmyth https://hg.mozilla.org/integration/mozilla-inbound/rev/f46426835026 Move old debugger's multiple-windows, tab and target actors tests. r=loganfsmyth

https://hg.mozilla.org/mozilla-central/rev/241f876d557f https://hg.mozilla.org/mozilla-central/rev/fa0d46e77437 https://hg.mozilla.org/mozilla-central/rev/90480b4b4c43 https://hg.mozilla.org/mozilla-central/rev/f46426835026

Try run for 18: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6037786138bc9c29c1b2d9041eae0835cbc82102

Backout by dluca@mozilla.com: https://hg.mozilla.org/mozilla-central/rev/f5681e1f56e6 Backed out 4 changesets for devtool failures. a=backout

Backed out 4 changesets (bug 1314057) for devtool failures. a=backout Log: https://treeherder.mozilla.org/logviewer.html#?job_id=204437862&repo=mozilla-inbound&lineNumber=1775 INFO - TEST-PASS | devtools/client/shared/test/browser_dbg_listaddons.js | Root actor should identify itself as a browser. - 06:53:43 INFO - Installing addon: Z:\task_1539153781\build\tests\mochitest\browser\devtools\client\shared\test\addon1.xpi 06:53:43 INFO - Get addon actor for ID: jid1-oBAwBoE5rSecNg@jetpack 06:53:43 INFO - Console message: 1539154378445 addons.xpi WARN Error loading bootstrap.js for jid1-oBAwBoE5rSecNg@jetpack: ReferenceError: __SCRIPT_URI_SPEC__ is not defined(resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///Z:/task_1539153781/build/tests/mochitest/browser/devtools/client/shared/test/addon1.xpi!/bootstrap.js:7:7) JS Stack trace: @bootstrap.js:7:7 06:53:43 INFO - loadBootstrapScope@XPIProvider.jsm:1677:9 06:53:43 INFO - callBootstrapMethod@XPIProvider.jsm:1535:9 06:53:43 INFO - async*_install@XPIProvider.jsm:1790:7 06:53:43 INFO - async*install@XPIProvider.jsm:1783:12 06:53:43 INFO - installTemporaryAddon@XPIInstall.jsm:3957:17 06:53:43 INFO - async*XPIProvider[meth]@XPIProvider.jsm:2740:28 06:53:43 INFO - installTemporaryAddon@AddonManager.jsm:2054:33 06:53:43 INFO - installTemporaryAddon@AddonManager.jsm:3330:12 06:53:43 INFO - addTemporaryAddon@browser_dbg_listaddons.js:129:10 06:53:43 INFO - testFirstAddon@browser_dbg_listaddons.js:55:10 06:53:43 INFO - process@Promise-backend.js:928:23 06:53:43 INFO - walkerLoop@Promise-backend.js:812:29 06:53:43 INFO - Promise*scheduleWalkerLoop@Promise-backend.js:745:11 06:53:43 INFO - schedulePromise@Promise-backend.js:776:7 06:53:43 INFO - Promise.prototype.then@Promise-backend.js:461:5 06:53:43 INFO - test/<@browser_dbg_listaddons.js:37:8 06:53:43 INFO - resolve@deprecated-sync-thenables.js:38:40 06:53:43 INFO - then@deprecated-sync-thenables.js:18:43 06:53:43 INFO - resolve@deprecated-sync-thenables.js:73:11 06:53:43 INFO - connect/<@debugger-client.js:185:7 06:53:43 INFO - l@event-source.js:59:11 06:53:43 INFO - eventSource/proto.emit@event-source.js:124:9 06:53:43 INFO - DebuggerClient/<@debugger-client.js:71:5 06:53:43 INFO - emit@event-emitter.js:178:15 06:53:43 INFO - emit@event-emitter.js:255:5 06:53:43 INFO - emitReply@debugger-client.js:886:31 06:53:43 INFO - onPacket@debugger-client.js:891:9 06:53:43 INFO - send/<@local-transport.js:64:11 06:53:43 INFO - exports.makeInfallible/<@ThreadSafeDevToolsUtils.js:109:14 06:53:43 INFO - DevToolsUtils.executeSoon*exports.executeSoon@DevToolsUtils.js:57:21 06:53:43 INFO - send@local-transport.js:58:7 06:53:43 INFO - _onConnection@main.js:890:7 06:53:43 INFO - connectPipe@main.js:274:24 06:53:43 INFO - test@browser_dbg_listaddons.js:30:21 06:53:43 INFO - Tester_execTest@browser-test.js:1124:9 06:53:43 INFO - nextTest/<@browser-test.js:986:9 06:53:43 INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@SimpleTest.js:795:59 06:53:43 INFO - Console message: 1539154378446 addons.xpi WARN Add-on jid1-oBAwBoE5rSecNg@jetpack is missing bootstrap method install 06:53:43 INFO - Console message: 1539154378447 addons.xpi WARN Add-on jid1-oBAwBoE5rSecNg@jetpack is missing bootstrap method startup 06:53:43 INFO - Buffered messages finished 06:53:43 INFO - TEST-UNEXPECTED-FAIL | devtools/client/shared/test/browser_dbg_listaddons.js | Test timed out - 06:53:43 INFO - GECKO(6316) | MEMORY STAT | vsize 1739MB | vsizeMaxContiguous 131487358MB | residentFast 246MB | heapAllocated 88MB 06:53:43 INFO - TEST-OK | devtools/client/shared/test/browser_dbg_listaddons.js | took 45051ms 06:53:43 INFO - checking window state 06:53:43 INFO - TEST-START | devtools/client/shared/test/browser_dbg_listtabs-01.js 06:53:43 INFO - GECKO(6316) | MEMORY STAT | vsize 1739MB | vsizeMaxContiguous 131487358MB | residentFast 253MB | heapAllocated 97MB 06:53:43 INFO - TEST-OK | devtools/client/shared/test/browser_dbg_listtabs-01.js | took 334ms 06:53:43 INFO - checking window state 06:53:43 INFO - TEST-START | devtools/client/shared/test/browser_dbg_listtabs-03.js 06:53:44 INFO - GECKO(6316) | MEMORY STAT | vsize 1739MB | vsizeMaxContiguous 131487358MB | residentFast 255MB | heapAllocated 99MB 06:53:44 INFO - TEST-OK | devtools/client/shared/test/browser_dbg_listtabs-03.js | took 184ms 06:53:44 INFO - checking window state 06:53:44 INFO - TEST-START | devtools/client/shared/test/browser_dbg_multiple-windows.js 06:53:44 INFO - GECKO(6316) | MEMORY STAT | vsize 1745MB | vsizeMaxContiguous 131487358MB | residentFast 267MB | heapAllocated 108MB 06:53:44 INFO - TEST-OK | devtools/client/shared/test/browser_dbg_multiple-windows.js | took 529ms 06:53:44 INFO - checking window state 06:53:44 INFO - TEST-START | devtools/client/shared/test/browser_dbg_navigation.js 06:53:44 INFO - GECKO(6316) | MEMORY STAT | vsize 1739MB | vsizeMaxContiguous 131487358MB | residentFast 264MB | heapAllocated 110MB 06:53:44 INFO - TEST-OK | devtools/client/shared/test/browser_dbg_navigation.js | took 149ms 06:53:44 INFO - checking window state 06:53:44 INFO - TEST-START | devtools/client/shared/test/browser_dbg_target-scoped-actor-01.js 06:53:44 INFO - GECKO(6316) | MEMORY STAT | vsize 1739MB | vsizeMaxContiguous 131487358MB | residentFast 265MB | heapAllocated 112MB 06:53:44 INFO - TEST-OK | devtools/client/shared/test/browser_dbg_target-scoped-actor-01.js | took 118ms 06:53:44 INFO - checking window state 06:53:44 INFO - TEST-START | devtools/client/shared/test/browser_dbg_target-scoped-actor-02.js 06:53:45 INFO - GECKO(6316) | MEMORY STAT | vsize 1739MB | vsizeMaxContiguous 131487358MB | residentFast 266MB | heapAllocated 113MB 06:53:45 INFO - TEST-OK | devtools/client/shared/test/browser_dbg_target-scoped-actor-02.js | took 118ms 06:53:45 INFO - checking window state 06:53:45 INFO - TEST-START | devtools/client/shared/test/browser_devices.js Backout: https://hg.mozilla.org/mozilla-central/rev/f5681e1f56e6d77989c3f63f9a017f5c94bd74af

Please check in 14-17 please! Thank you!

Please checking 18 as well. Thank you!

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/73a17eb4a47c Move globalactor and global method override tests. r=loganfsmyth https://hg.mozilla.org/integration/mozilla-inbound/rev/abfa105d7bc3 Move listtabs and listaddons tests to shared. r=loganfsmyth https://hg.mozilla.org/integration/mozilla-inbound/rev/c4e7e3734161 Remove duplicated test between old and new debugger. r=loganfsmyth https://hg.mozilla.org/integration/mozilla-inbound/rev/e3373dae68a5 Move old debugger's multiple-windows, tab and target actors tests. r=loganfsmyth https://hg.mozilla.org/integration/mozilla-inbound/rev/e7efb5d6d6fd Move worker actor and console tests to shared. r=loganfsmyth

https://hg.mozilla.org/mozilla-central/rev/73a17eb4a47c https://hg.mozilla.org/mozilla-central/rev/abfa105d7bc3 https://hg.mozilla.org/mozilla-central/rev/c4e7e3734161 https://hg.mozilla.org/mozilla-central/rev/e3373dae68a5 https://hg.mozilla.org/mozilla-central/rev/e7efb5d6d6fd

Could you have a look here :ochameau? 95% of this huge-looking patch is just deletion of files in debugger/ and then moving up the debugger/new files, but I'm still seeing errors like this: Error running mach: ['mochitest', 'browser_markup_shadowdom_open_debugger.js'] The error occurred in code that was called by the mach command. This is either a bug in the called code itself or in the way that mach is calling it. You should consider filing a bug for this issue. If filing a bug, please include the full output of mach, including this error message. The details of the failure are as follows: Exception: A cross-directory support file path noted in a test manifest does not appear in any other manifest. "devtools/client/debugger/test/mochitest/helpers.js" must appear in another test manifest to specify an install for "!/devtools/client/debugger/test/mochitest/helpers.js". File "/Users/davidwalsh/Projects/mozilla-inbound/testing/mochitest/mach_commands.py", line 370, in run_mochitest_general driver.install_tests(tests) File "/Users/davidwalsh/Projects/mozilla-inbound/python/mozbuild/mozbuild/controller/building.py", line 1358, in install_tests '_tests', test_objs) File "/Users/davidwalsh/Projects/mozilla-inbound/python/mozbuild/mozbuild/testing.py", line 263, in install_test_files manifest = _make_install_manifest(topsrcdir, topobjdir, test_objs) File "/Users/davidwalsh/Projects/mozilla-inbound/python/mozbuild/mozbuild/testing.py", line 251, in _make_install_manifest _resolve_installs(install_info.deferred_installs, topobjdir, manifest) File "/Users/davidwalsh/Projects/mozilla-inbound/python/mozbuild/mozbuild/testing.py", line 193, in _resolve_installs 'for "!/%s".' % (path, path))

Comment on attachment 9016885 [details] [diff] [review] 1314057-19.patch Could you retry on a rebased tree? Unless there is another patch to apply before this one? It seems to be working for me locally... $ wget https://bug1314057.bmoattachments.org/attachment.cgi?id=9016885 -O p $ git apply p $ ./mach build $ ./mach mochitest browser_markup_shadowdom_open_debugger.js

Extended try for 19: https://treeherder.mozilla.org/#/jobs?repo=try&revision=60077d65cec80283e880d1c53aac5557c03cf9ca&selectedJob=205642836

Comment on attachment 9017464 [details] [diff] [review] 1314057-19.patch Review of attachment 9017464 [details] [diff] [review]: ----------------------------------------------------------------- Looks good overall! My main concern here is that I see some tests being deleted that are still relevants. Then it would be great if we could save changelog of files that are having a name collision... Otherwise, with that fixed, it should be good to go! Note that, if that can help moving forward, you may do that in two steps: * remove the old debugger You may split this one in two: * remove everything but the couple of mochitests and necessary helpers. * And later on, another one to remove/move them. * move client/debugger/new to client/debugger/ Final tip, you are working in a unique bug and with FF65 coming on monday it may start being quite messy to land patches in one bug that may end up in two distinct releases... You might want to start working on distinct/individual bugs. ::: .eslintignore @@ +120,4 @@ > devtools/client/debugger/** > > # Ignore devtools imported repositories > +devtools/client/debugger/** Hum. I think we would want to followup on this as it doesn't look quite right to have the whole debugger source being ignored. I imagine it doesn't make sense to lint the transpiled files, but we should already be able to lint the mochitests at least. ::: devtools/client/debugger/moz.build @@ +19,4 @@ > ] > > with Files('**'): > + BUG_COMPONENT = ('DevTools', 'Debugger') \ No newline at end of file Please update the indentation of this file to have 4 space indentation instead of 2. That's the typical indentation for moz.build files. ::: devtools/client/debugger/panel.js @@ +114,4 @@ > } > }; > + > +exports.DebuggerPanel = DebuggerPanel; I don't know if you tried doing that, but it would be great to keep file history by: * removing the old file (hg rm) * moving the new one here Then hopefully we keep track of all the changelog. As here it looks like you are squashing the two files altogether whereas you are not modifying anything. ::: devtools/client/debugger/test/mochitest/browser.ini @@ +772,5 @@ > +skip-if = os != "mac" || debug || !nightly_build > +[browser_dbg_rr_replay-03.js] > +skip-if = os != "mac" || debug || !nightly_build > +[browser_dbg_rr_console_warp-01.js] > +skip-if = os != "mac" || debug || !nightly_build Same comment here about file history. ::: devtools/client/debugger/test/mochitest/browser_dbg_debugger-statement.js @@ -81,5 @@ > -} > - > -registerCleanupFunction(function () { > - gClient = null; > -}); Has this test being copied somewhere else? If not, it should as it doesn't test anything related to old debugger UI. Instead it covers the ThreadClient behavior only. To distinguish tests that should be kept it is quite simple. We should keep tests that do not test the old debugger UI. One easy filter is to look for the ones that do not use `initDebugger`... But using initDebugger doesn't necessarely mean testing the UI. Some tests are using it, without using the returned "panel" object and so aren't testing the UI. ::: devtools/client/debugger/test/mochitest/browser_dbg_event-listeners-01.js @@ -128,5 @@ > - Assert.ok(types.includes("change"), "Found the change handler."); > - Assert.ok(types.includes("keyup"), "Found the keyup handler."); > - > - await aThreadClient.resume(); > -} Same comment about this test. ::: devtools/client/debugger/test/mochitest/browser_dbg_event-listeners-02.js @@ -107,5 @@ > - "Capturing property has the right value."); > - } > - > - await aThreadClient.resume(); > -} Same comment about this test. ::: devtools/client/debugger/test/mochitest/browser_dbg_event-listeners-03.js @@ -77,5 @@ > -} > - > -registerCleanupFunction(function () { > - gClient = null; > -}); Same comment about this test. ::: devtools/client/debugger/test/mochitest/browser_dbg_listworkers.js @@ -55,5 @@ > - > - yield close(client); > - finish(); > - }); > -} Same comment about this test. ::: devtools/client/debugger/test/mochitest/browser_dbg_promises-allocation-stack.js @@ -83,5 @@ > - }); > - }); > - > - yield front.detach(); > -} Same comment about all the tests browser_dbg_promises*.js. ::: devtools/client/debugger/test/mochitest/browser_dbg_terminate-on-tab-close.js @@ -33,5 @@ > - }); > - > - callInTab(gTab, "debuggerThenThrow"); > - }); > -} Same comment about this test. ::: devtools/client/debugger/test/mochitest/head.js @@ -1307,5 @@ > - let debuggerPanel = toolbox.getCurrentPanel(); > - let gDebugger = debuggerPanel.panelWin; > - > - return {client, tab, tabClient, workerClient, toolbox, gDebugger}; > -} Same comment about file history. ::: taskcluster/taskgraph/test/automationrelevance.json @@ -18,4 @@ > "branch": "default" > }, > "files": [ > - "devtools/client/debugger/new/index.html" I'm not sure it is worth updating these two files from taskgraph's tests. It just archived some old changeset metadata and is asserting against it. Let the test live in the past and forget about it! ::: testing/runtimes/mochitest-devtools-chrome.runtimes.json @@ -41,5 @@ > - "devtools/client/debugger/new/test/mochitest/browser_dbg-sourcemaps.js": 13900, > - "devtools/client/debugger/new/test/mochitest/browser_dbg-sourcemaps2.js": 6133, > - "devtools/client/debugger/new/test/mochitest/browser_dbg-sources.js": 7369, > - "devtools/client/debugger/new/test/mochitest/browser_dbg-tabs.js": 13291, > - "devtools/client/debugger/new/test/mochitest/browser_dbg_keyboard-shortcuts.js": 9276, I don't think you should update these two JSON files, it looks like something generated automatically.

:jdescottes; with regard to patch 19, the goal was to remove the old debugger assets while leaving the new debugger in its current location. There are test to migrate but :honza had suggested we skip them for now and migrate them in the next patch. https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff4328982474f7bc67c596eec740f28623f0d993&selectedJob=206642144

Comment on attachment 9018673 [details] [diff] [review] 1314057-19.patch Review of attachment 9018673 [details] [diff] [review]: ----------------------------------------------------------------- Thanks David, assuming you agreed on what to do about the tests with Alex/Honza I don't have anything but small nits here. I am curious about which tests are left to migrate. Is there another bug already logged with the list of test we want to try to migrate? I randomly looked at browser_dbg_worker-window.js and browser_dbg_addon-console.js which don't seem covered by other tests, but are deleted with this patch. Candidates for follow up bugs: - we have many strings in devtools/client/locales/en-US/debugger.properties which were only used by the old debugger and should be removed - the file sourceeditor/debugger.js is no longer imported anywhere and can also be removed - there is a file called "old-debugger.css" which is still being imported by codemirror. We check if it can be removed. - this mention of the old debugger can be removed: https://searchfox.org/mozilla-central/rev/0ec9f5972ef3e4a479720630ad7285a03776cdfc/devtools/client/webconsole/test/mochitest/head.js#575-578 - do a last cleanup for mentions of "new debugger" ::: devtools/client/debugger/moz.build @@ +7,4 @@ > 'new' > ] > > DevToolsModules( We can remove this since this is empty now. ::: devtools/client/debugger/test/mochitest/head.js @@ +30,4 @@ > promise = require("devtools/shared/deprecated-sync-thenables"); > > const EXAMPLE_URL = "http://example.com/browser/devtools/client/debugger/test/mochitest/"; > +//const FRAME_SCRIPT_URL = getRootDirectory(gTestPath) + "code_frame-script.js"; We should remove the commented out line ::: devtools/client/definitions.js @@ +154,4 @@ > } > }; > > +Tools.jsdebugger.url = "chrome://devtools/content/debugger/new/index.html"; This should be in the default definition of Tools.jsdebugger now. We should also remove the loader.lazyGetter(this, "DebuggerPanel", () => require("devtools/client/debugger/panel").DebuggerPanel); in this file. ::: devtools/client/framework/attach-thread.js @@ +43,5 @@ > // frontend. This is because it does sourcemapping on the > // client-side, so the server should not do it. > + const useSourceMaps = false; > + const autoBlackBox = false; > + const ignoreFrameEnvironment = true; nit: those variables are not strictly necessary (we just need threadOptions below) and the comment could be rephrased since we are always using the new debugger now.

Updated version 19 with :jdescottes' suggested nits: ./mach try -b do -p linux64,macosx64,win32,win64 -u mochitest-clipboard-e10s,mochitest-e10s-dt -t none --rebuild 4

Removing checkin-needed to restore a test file. New patch coming shortly.

Updated to restore two tests mentioned by :jdescottes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8620ce60ef7650edae3b3704561a136bd3bc0cf

New patch 19 fixes small lint issue.

Needs rebasing. Also, does this bug still need the leave-open keyword set?

Rebased, and please keep open.

Please checkin 19. Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7b9e1e1bdd1ab877ab0683c6561d0da6ac94a8d

Now it has conflicts on autoland with bug 1490491 :(

Rebased on autoland; try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1cf6f6406507dbec9047333ae26bd36d16e07815

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/9fb4fd49a818 Remove old debugger assets. r=jdescottes