692405 - Convert the debugger UI to a module

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

The current debugger UI is implemented as a XUL overlay. We should make it a module and hook on the initialization sequence the same way HUDService.jsm or inspector.jsm do it.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

At the same time we should get rid of the content directory and move all source code up to browser/devtools/debugger. On a similar note, robcee has suggested that there is no need for test/browser/ and we could put all browser mochitests in test/.

Comment 2

[Victor Porof [:vporof][:vp]]

Attachment: v1

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 572830 [details] [diff] [review]
v1

Review of attachment 572830 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't look too much into the patch, because I had issues applying it. Let's get these fixed and we'll be back in business!

I got a conflict probably because you haven't updated my l10n patch with a typo fix I did yesterday. Fixing that manually doesn't let the patch apply cleanly. The best course of action would be to pop everything up to (and including) the l10n patch, delete the l10n patch, import the new version from the bug and then push the other patches on top of it.

I also got many leftovers in browser/devtools/debugger/content/ and browser/devtools/debugger/test/browser/. Did you only do hg mv for a few of the files and hg cp for the others? The patch does not contain the "deleted file mode XXX" lines for most of them.

And one last thing that caught my eye: you have removed trailing whitespace from browser.js. Although this is in accordance to the style guide, I think there is a good chance that we'll get yelled at from browser peers for touching unrelated code and mixing whitespace fixes with important changes. I have removed a similar setting I had locally in vim to avoid getting bitten by this.

Comment 4

[Victor Porof [:vporof][:vp]]

Attachment: v2

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 573325 [details] [diff] [review]
v2

Review of attachment 573325 [details] [diff] [review]:
-----------------------------------------------------------------

Patch looks good. r- mainly for the forgotten paths in the test files and the lost version history. As discussed in IRC, we should use hg mv instead of hg add + hg rm.

::: browser/devtools/debugger/DebuggerUI.jsm
@@ +226,5 @@
> +  getDebugger: function DebuggerUI_getDebugger(aTab) {
> +    return aTab._scriptDebugger;
> +  },
> +
> +  getPreferences: function DebuggerUI_getPreferences() {

I think a getter would be better here. Probably for getDebuger as well, although DebuggerUI.debugger does seem kinda weird...

@@ +334,5 @@
> +
> +  /**
> +   * Gets the preferred height of the debugger pane.
> +   *
> +   * @return {Number} the preferred height

Nit: the curly braces here and below are inconsistent with the rest of the file. That's also how it's done in scratchpad and web console.

@@ +341,5 @@
> +    let prefs = Cc["@mozilla.org/preferences-service;1"].
> +      getService(Ci.nsIPrefService).
> +      getBranch(this._branch);
> +
> +    return prefs.getIntPref("ui.height");

This is unnecessarily verbose, just use Services.prefs.getIntPref.

@@ +348,5 @@
> +  /**
> +   * Sets the preferred height of the debugger pane.
> +   *
> +   * @param {Number} value
> +   *                 the new height

Nit: start the description right below the parameter type and use a proper sentence (capitalization, full stop).

@@ +355,5 @@
> +    let prefs = Cc["@mozilla.org/preferences-service;1"].
> +      getService(Ci.nsIPrefService).
> +      getBranch(this._branch);
> +
> +    prefs.setIntPref("ui.height", value);

Same as above: Services.prefs.setIntPref.

::: browser/devtools/debugger/debugger-view.js
@@ +89,5 @@
> +
> +  /**
> +   * Sets the current frames state based on the debugger active thread state.
> +   *
> +   * @param {String} aState

Heh, there is inconsistency in this file, too, but I caused it with the l10n patch. It would be good to maintain a consistent format across the codebase though.

::: browser/devtools/debugger/test/Makefile.in
@@ +40,4 @@
>  topsrcdir       = @top_srcdir@
>  srcdir          = @srcdir@
>  VPATH           = @srcdir@
> +relativesrcdir  = browser/devtools/debugger/test/browser

This should be browser/devtools/debugger/test now.

::: browser/devtools/debugger/test/browser_dbg_contextactor-01.js
@@ +11,5 @@
> +var gClient = null;
> +
> +function test()
> +{
> +  DebuggerServer.addActors("chrome://mochitests/content/browser/browser/devtools/debugger/test/browser/testactors.js");

This is still the old URL under test/browser.

::: browser/devtools/debugger/test/browser_dbg_contextactor-02.js
@@ +11,5 @@
> +var gClient = null;
> +
> +function test()
> +{
> +  DebuggerServer.addActors("chrome://mochitests/content/browser/browser/devtools/debugger/test/browser/testactors.js");

Old URL.

::: browser/devtools/debugger/test/browser_dbg_debuggerstatement.js
@@ +8,5 @@
> +
> +var gClient = null;
> +var gTab = null;
> +const DEBUGGER_TAB_URL = "http://example.com/browser/browser/devtools/" +
> +                         "debugger/test/browser/" +

Old URL.

::: browser/devtools/debugger/test/browser_dbg_propertyview-07.js
@@ +6,5 @@
> + * Make sure that the property view displays function parameters.
> + */
> +
> +const TAB_URL = "http://example.com/browser/browser/devtools/debugger/test/" +
> +                "browser/browser_dbg_frame-parameters.html";

Old URL.

::: browser/devtools/debugger/test/browser_dbg_propertyview-08.js
@@ +6,5 @@
> + * Make sure that the property view displays the properties of objects.
> + */
> +
> +const TAB_URL = "http://example.com/browser/browser/devtools/debugger/test/" +
> +                "browser/browser_dbg_frame-parameters.html";

Old URL.

::: browser/devtools/debugger/test/browser_dbg_script-switching.html
@@ +4,5 @@
> +		<title>Browser Debugger Script Switching Test</title>
> +    <!-- Any copyright is dedicated to the Public Domain.
> +         http://creativecommons.org/publicdomain/zero/1.0/ -->
> +    <script type="text/javascript" src="http://example.com/browser/browser/devtools/debugger/test/browser/test-script-switching-01.js"></script>
> +    <script type="text/javascript" src="http://example.com/browser/browser/devtools/debugger/test/browser/test-script-switching-02.js"></script>

Old URLs.

::: browser/devtools/debugger/test/browser_dbg_script-switching.js
@@ +6,5 @@
> + * Make sure that switching the displayed script in the UI works as advertised.
> + */
> +
> +const TAB_URL = "http://example.com/browser/browser/devtools/debugger/test/" +
> +                "browser/browser_dbg_script-switching.html";

Old URL.

::: browser/devtools/debugger/test/browser_dbg_tabactor-01.js
@@ +11,5 @@
> +var gClient = null;
> +
> +function test()
> +{
> +  DebuggerServer.addActors("chrome://mochitests/content/browser/browser/devtools/debugger/test/browser/testactors.js");

Old URL.

::: browser/devtools/debugger/test/browser_dbg_tabactor-02.js
@@ +11,5 @@
> +var gClient = null;
> +
> +function test()
> +{
> +  DebuggerServer.addActors("chrome://mochitests/content/browser/browser/devtools/debugger/test/browser/testactors.js");

Old URL.

::: browser/devtools/debugger/test/head.js
@@ +5,5 @@
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cu = Components.utils;
> +Cu.import("resource:///modules/dbg-server.jsm");
> +Cu.import("resource:///modules/dbg-client.jsm");

Hmm, I wonder if we should put these under a devtools path, too. There is an XXX comment in the Makefile suggesting that they might have to be chrome urls, but I don't fully understand the tradeoffs. This does not have to happen in this bug though.

@@ +12,5 @@
> +const TAB1_URL = "http://example.com/browser/browser/devtools/debugger/test/browser/browser_dbg_tab1.html";
> +
> +const TAB2_URL = "http://example.com/browser/browser/devtools/debugger/test/browser/browser_dbg_tab2.html";
> +
> +const STACK_URL = "http://example.com/browser/browser/devtools/debugger/test/browser/browser_dbg_stack.html";

All these URLs should be changed.

::: browser/devtools/jar.mn
@@ +10,5 @@
>      content/browser/orion.css                     (sourceeditor/orion/orion.css)
>      content/browser/orion-mozilla.css             (sourceeditor/orion/mozilla.css)
> +*   content/browser/debugger.xhtml                (debugger/debugger.xhtml)
> +*   content/browser/debugger.css                  (debugger/debugger.css)
> +*   content/browser/debugger.js                   (debugger/debugger.js)

The stars in the first column are not really needed, since we don't need any preprocessing for these files. I think we used to in some of them, but not any more. It just makes the build a bit slower.

::: browser/locales/en-US/chrome/browser/devtools/debugger.dtd
@@ +2,4 @@
>  
>  <!-- LOCALIZATION NOTE (applicationMenuLabel): This is the label for the
>    -  application menu item that opens the debugger UI. -->
> +<!ENTITY debuggerMenu.label          "Script Debugger">

Since you changed the entity name, fix the localization note above (applies throughout the file).

@@ +8,2 @@
>    -  debugger UI. Do not translate this one! -->
> +<!ENTITY debuggerMenu.accesskey      "I">

Since you are adding an access key, it would be nice to run the patch through try, or get a local full-test run if you are on Linux or Windows. The last time I added one, I broke a context menu test that checked that our access keys are unique in all configurations. I'm on a Mac and access keys are not visible here, unfortunately.

Comment 6

[Victor Porof [:vporof][:vp]]

Attachment: v3

Comment 7

[Victor Porof [:vporof][:vp]]

Attachment: v3.1

The preprocessing * in jar.mn is needed for debugger.xhtml

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 573769 [details] [diff] [review]
v3.1

Review of attachment 573769 [details] [diff] [review]:
-----------------------------------------------------------------

As discussed in IRC I think that if you renamed before editing debugger-view.js, debugger.xhtml and browser_dbg_panesize.js, they would show up properly in the diff, like the test Makefile. Using "hg log -f" on them only shows one revision. It's no big deal if you can't get it to work for some reason though.

Just fix the entity names in the comments. Excellent job!

::: browser/locales/en-US/chrome/browser/devtools/debugger.dtd
@@ +3,2 @@
>  
>  <!-- LOCALIZATION NOTE (applicationMenuLabel): This is the label for the

You forgot to update the entity name in the localization notes (debuggerMenu.label in this case). Applies here and elsewhere in this file.

Comment 9

[Victor Porof [:vporof][:vp]]

Attachment: v3.2

Fixed localization comments.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v4

Fixed a few more entity names in the localization comments, changed the command key to cmd-opt-S since I is going to be used by the inspector with bug 689924 and avoided using ctrl-alt on Windows and Linux as requested in bug 696759.

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/users/dcamp_campd.org/remote-debug/rev/b55ebf123cc4