880930 - JS debugger: RootActor needs its own definition of windowMediator

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P1)

Description

[Jim Blandy :jimb]

At the moment, RootActor's preNest and postNest hooks assume that the 'windowMediator' global is defined --- but that's only the case when it is being used in the same global as webbrowser.js, which is not the case in a content child process.

root.js needs its own definition for windowMediator.

Comment 1

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

Attachment: m-c patch - v1

Please excuse if I have any misunderstandings here, but I don't quite see the need for windowMediator to be a global variable. Can't we just use Services.wm here?

This patch fixes. I wasn't all too sure about the use of Cu vs. Components.utils. Some files define Cu explicitly, others don't. Please let me know whats best here.

Comment 2

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

Attachment: c-c patch - v1

Comment 3

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

Comment on attachment 803394 [details] [diff] [review]
m-c patch - v1

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

The patch needs rebasing and I have a few comments below, but most importantly this needs to be tested on a b2g device. I'm seeing some weirdness in today's build on my nexus-s that prevent me from testing this, so I'd like Alex to make sure that this works for him.

r=me if Alex gives an f+.

::: toolkit/devtools/server/actors/gcli.js
@@ +9,4 @@
>  let { classes: Cc, interfaces: Ci, utils: Cu } = Components;
>  
>  let { XPCOMUtils } = Cu.import("resource://gre/modules/XPCOMUtils.jsm", {});
> +Cu.import("resource://gre/modules/Services.jsm");

For performance and consistency better use:
XPCOMUtils.defineLazyModuleGetter(this, "Services", "resource://gre/modules/Services.jsm");

@@ +47,4 @@
>  };
>  
>  GcliActor.prototype.execute = function(request) {
> +  let chromeWindow = Services.wm.getMostRecentWindow("navigator:browser");

Use DebuggerServer.chromeWindowType instead of navigator:browser.

::: toolkit/devtools/server/actors/root.js
@@ +8,4 @@
>  
>  /* Root actor for the remote debugging protocol. */
>  
> +Components.utils.import("resource://gre/modules/Services.jsm");

Cu should be available here as well.

::: toolkit/devtools/server/actors/webbrowser.js
@@ +194,4 @@
>  };
>  
>  BrowserTabList.prototype.getList = function() {
> +  let topXULWindow = Services.wm.getMostRecentWindow("navigator:browser");

Rebase this hunk and make sure you use DebuggerServer.chromeWindowType instead of navigator:browser.

Comment 4

[Alexandre Poirot [:ochameau]]

Comment on attachment 803394 [details] [diff] [review]
m-c patch - v1

works fine!

Comment 5

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

Attachment: m-c patch - v2

For root.js you mention to use Cu.import, for the actor you say XPCOMUtils.defineLazyModuleGetter. Generally, in which cases would I use what? Or should I be using defineLazyModuleGetter always?

Comment 6

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

Comment on attachment 807997 [details] [diff] [review]
m-c patch - v2

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

(In reply to Philipp Kewisch [:Fallen] from comment #5)
> For root.js you mention to use Cu.import, for the actor you say
> XPCOMUtils.defineLazyModuleGetter. Generally, in which cases would I use
> what? Or should I be using defineLazyModuleGetter always?

In general they are equivalent, but a lazy getter is preferred if the imported code is not expected to be executed right away. Hopefully the existing code has incorporated this assumption, so following its example is a good rule of thumb.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 803395 [details] [diff] [review]
c-c patch - v1

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

LGTM. Thanks!

Comment 8

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

https://hg.mozilla.org/comm-central/rev/69f939e4cded
https://hg.mozilla.org/integration/fx-team/rev/0cd9c60fd674

Comment 9

[Girish Sharma [:Optimizer]]

This is not resolved fixed yet. (until it merges to m-c)

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0cd9c60fd674