Closed Bug 880930 Opened 7 years ago Closed 7 years ago

JS debugger: RootActor needs its own definition of windowMediator

Categories

(DevTools :: Debugger, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 27

People

(Reporter: jimb, Assigned: Fallen)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Blocks: 878958
Priority: -- → P1
Attached patch m-c patch - v1 (obsolete) — Splinter Review
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.
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Attachment #803394 - Flags: review?(past)
Attached patch c-c patch - v1Splinter Review
Attachment #803395 - Flags: review?(mconley)
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.
Attachment #803394 - Flags: review?(past)
Attachment #803394 - Flags: review+
Attachment #803394 - Flags: feedback?(poirot.alex)
Comment on attachment 803394 [details] [diff] [review]
m-c patch - v1

works fine!
Attachment #803394 - Flags: feedback?(poirot.alex) → feedback+
Attached patch m-c patch - v2Splinter Review
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?
Attachment #803394 - Attachment is obsolete: true
Attachment #807997 - Flags: review+
Attachment #807997 - Flags: feedback?(past)
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.
Attachment #807997 - Flags: feedback?(past)
Comment on attachment 803395 [details] [diff] [review]
c-c patch - v1

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

LGTM. Thanks!
Attachment #803395 - Flags: review?(mconley) → review+
This is not resolved fixed yet. (until it merges to m-c)
Status: RESOLVED → UNCONFIRMED
Ever confirmed: false
Resolution: FIXED → ---
Whiteboard: [fixed-in-fx-team]
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
https://hg.mozilla.org/mozilla-central/rev/0cd9c60fd674
Status: ASSIGNED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
Depends on: 919683
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.