1400352 - Update the debugger frontend (9/15)

Status: RESOLVED INVALID

(DevTools :: Debugger, enhancement, P3)

Comment 1

[Jason Laster [:jlast]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a9eda5d343a35e554d2b3d957306d04272bc44e

Comment 2

[Jason Laster [:jlast]]

Attachment: 9-15-1.patch

Comment 3

[Jason Laster [:jlast]]

This patch has two bug fixes for 57:

* wasm binary debugging
* watch expressions (expanding properties when the debuggee is active)

Comment 4

[Julian Descottes [:jdescottes]]

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

Comment 5

[Jason Laster [:jlast]]

Attachment: 9-15-2.patch

Comment 6

[Julian Descottes [:jdescottes]]

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 7

[Julian Descottes [:jdescottes]]

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

Comment 8

[Tom Tromey :tromey]

(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

Comment 9

[Jason Laster [:jlast]]

> I still see references to the old pref in integration-tests.js

I'll nuke the file.

Comment 10

[Julian Descottes [:jdescottes]]

(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.

Comment 11

[Jason Laster [:jlast]]

Attachment: 9-15-3.patch

Comment 12

[Tom Tromey :tromey]

(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.

Comment 13

[Jason Laster [:jlast]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=63475306741a3fcebf05e9401025c921fc575e7d

Comment 14

[Julian Descottes [:jdescottes]]

(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?

Comment 15

[Tom Tromey :tromey]

(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.

Comment 16

[Jason Laster [:jlast]]

Attachment: 9-15-4.patch

Comment 17

[Julian Descottes [:jdescottes]]

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

Comment 18

[Julian Descottes [:jdescottes]]

Attachment: Bug 1400352 - Update the debugger frontend (9/15).

Review commit: https://reviewboard.mozilla.org/r/180454/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/180454/

Comment 19

[Julian Descottes [:jdescottes]]

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

Comment 20

[Jason Laster [:jlast]]

Attachment: 9-15-5.patch

Comment 21

[Julian Descottes [:jdescottes]]

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

Comment 22

[Julian Descottes [:jdescottes]]

Comment on attachment 8908843 [details]
Bug 1400352 - Update the debugger frontend (9/15).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180454/diff/1-2/

Comment 23

[Julian Descottes [:jdescottes]]

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)

Comment 24

[Jason Laster [:jlast]]

Attachment: 9-15-6.patch

Comment 25

[Julian Descottes [:jdescottes]]

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

Comment 26

[Patrick Brosset <:pbro>]

This bug has not been updated in the last 6 months. Resetting the assignee field.
Please, feel free to pick it up again and add a comment outlining your plans for it if you do still intend to work on it.
This is just trying to clean our backlog of bugs and make bugs available for people.