This patch has two bug fixes for 57: * wasm binary debugging * watch expressions (expanding properties when the debuggee is active)

Comment on attachment 8908751 [details] [diff] [review] 9-15-1.patch Review of attachment 8908751 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/preferences/debugger.js @@ -11,5 @@ > pref("devtools.debugger.remote-timeout", 20000); > pref("devtools.debugger.pause-on-exceptions", false); > pref("devtools.debugger.ignore-caught-exceptions", false); > pref("devtools.debugger.source-maps-enabled", true); > -pref("devtools.debugger.client-source-maps-enabled", true); I still see references to the old pref in: - devtools/client/debugger/new/integration-tests.js - devtools/client/framework/toolbox-options.xhtml - devtools/client/framework/toolbox-process-window.js All need to be migrated to the new pref. @@ +13,5 @@ > pref("devtools.debugger.ignore-caught-exceptions", false); > pref("devtools.debugger.source-maps-enabled", true); > + > +// Enable client-side mapping service for source maps > +pref("devtools.source-map.client-service.enabled", true); As discussed, let's keep this in devtools.js as it is not debugger specific. ::: devtools/client/preferences/devtools.js @@ -305,5 @@ > // Enable the new webconsole frontend > pref("devtools.webconsole.new-frontend-enabled", true); > > -// Enable client-side mapping service for source maps > -pref("devtools.source-map.client-service.enabled", true); This pref is still used in: devtools/client/debugger/new/debugger.js devtools/client/framework/source-map-url-service.js devtools/client/framework/toolbox.js devtools/client/preferences/debugger.js devtools/client/shared/components/test/mochitest/test_frame_02.html devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_sourcemap_css.js devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_sourcemap_invalid.js devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_sourcemap_nosource.js

Ignore the last part of the review, I thought the pref "devtools.source-map.client-service.enabled" was being removed, while it was just moved to another file!

Comment on attachment 8908757 [details] [diff] [review] 9-15-2.patch Review of attachment 8908757 [details] [diff] [review]: ----------------------------------------------------------------- You probably missed this comment from my previous review. ::: devtools/client/preferences/debugger.js @@ -11,5 @@ > pref("devtools.debugger.remote-timeout", 20000); > pref("devtools.debugger.pause-on-exceptions", false); > pref("devtools.debugger.ignore-caught-exceptions", false); > pref("devtools.debugger.source-maps-enabled", true); > -pref("devtools.debugger.client-source-maps-enabled", true); I still see references to the old pref in: - devtools/client/debugger/new/integration-tests.js - devtools/client/framework/toolbox-options.xhtml - devtools/client/framework/toolbox-process-window.js

(In reply to Julian Descottes [:jdescottes] from comment #7) > I still see references to the old pref in: > - devtools/client/debugger/new/integration-tests.js Ugh, I was not aware of this, sorry. > - devtools/client/framework/toolbox-options.xhtml > - devtools/client/framework/toolbox-process-window.js These are fixed in https://bugzilla.mozilla.org/show_bug.cgi?id=1371849

> I still see references to the old pref in integration-tests.js I'll nuke the file.

(In reply to Tom Tromey :tromey from comment #8) > (In reply to Julian Descottes [:jdescottes] from comment #7) > > > I still see references to the old pref in: > > - devtools/client/debugger/new/integration-tests.js > > Ugh, I was not aware of this, sorry. > > > - devtools/client/framework/toolbox-options.xhtml > > - devtools/client/framework/toolbox-process-window.js > > These are fixed in https://bugzilla.mozilla.org/show_bug.cgi?id=1371849 I don't see a fix for devtools/client/framework/toolbox-process-window.js in the patches attached to this bug.

(In reply to Julian Descottes [:jdescottes] from comment #10) > I don't see a fix for devtools/client/framework/toolbox-process-window.js in > the patches attached to this bug. Sorry, that's correct. This code is harmless to leave in and I could roll the removal into that patch.

(In reply to Tom Tromey :tromey from comment #12) > (In reply to Julian Descottes [:jdescottes] from comment #10) > > > I don't see a fix for devtools/client/framework/toolbox-process-window.js in > > the patches attached to this bug. > > Sorry, that's correct. > This code is harmless to leave in and I could roll the removal into that > patch. The code can stay, but I'm just wondering if we don't need to force the new preference to false for the browser toolbox profile? The current code reads: > // Bug 1225160 - Using source maps with browser debugging can lead to a crash > Services.prefs.setBoolPref("devtools.debugger.source-maps-enabled", false); If "devtools.source-map.client-service.enabled" is true, we won't have similar issues?

(In reply to Julian Descottes [:jdescottes] from comment #14) > The current code reads: > > > // Bug 1225160 - Using source maps with browser debugging can lead to a crash > > Services.prefs.setBoolPref("devtools.debugger.source-maps-enabled", false); > > If "devtools.source-map.client-service.enabled" is true, we won't have > similar issues? This crash only occurs when using the server-side source map service. "devtools.debugger.source-maps-enabled" is the old debugger's pref. We are mostly concerned with "devtools.debugger.client-source-maps-enabled". So, everything there is fine.

Sorry, I had the two old prefs mixed up again :( Thanks for the clarification Tom.

try with patch rebased on top of Bug 1371849: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd42d7f0dad33275e94dc7e4d8573b6e1db4daa6

new try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cea0d89da8e7e2a4d9329ae3e0b3a4751ec5412c

Comment on attachment 8908855 [details] [diff] [review] 9-15-5.patch Review of attachment 8908855 [details] [diff] [review]: ----------------------------------------------------------------- Not sure about the changes in devtools/client/debugger/new/test/mochitest/browser.ini, skipifs have been shuffled around. Failures on devtools/client/debugger/new/test/mochitest/browser_dbg-pause-exceptions.js (see https://treeherder.mozilla.org/#/jobs?repo=try&revision=cea0d89da8e7e2a4d9329ae3e0b3a4751ec5412c)

Needs rebase now. conflicts with debugger.js after bug Bug 1400854 landed.