Closed Bug 562168 Opened 10 years ago Closed 8 years ago

Move inspector.js into a jsm

Categories

(DevTools :: General, defect, P1)

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: rcampbell, Assigned: msucan)

References

Details

(Whiteboard: [minotaur][best: 1h; likely: 3h; worst: 6h])

Attachments

(1 file, 4 obsolete files)

from (https://bugzilla.mozilla.org/show_bug.cgi?id=547453#c23)

Can we put the object definitions in a JavaScript module instead? We'd still
need to have object instances per-window (as lazy getters, perhaps?), and have
them keep references to the per-window elements they refer to, which would
require some refactoring, but it seems like that shouldn't be too hard to
manage?

Something like:
XPCOMUtils.defineLazyGetter(this, "InspectorUI", function () {
  let temp = {};
  Cu.import("resource:///modules/Inspector.jsm", temp);
  return new temp.InspectorUI(panelElement, treeElement, tabbrowser, <etc.>);
});
Summary: Move Inspector object initialization into module, refactor (reticle) → Move inspector.js into a jsm
Whiteboard: [minotaur][best: 1h; likely: 3h; worst: 6h]
Updating this bug to reflect reality a year and a half later.

We should move inspector.js into a jsm of it's own and lazily initialize it browser.js.

This should facilitate a move into browser/devtools.
Duplicate of this bug: 669652
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED
Attached patch jsm wip (obsolete) — Splinter Review
WIP patch. Invokable through a scratchpad, but not through browser UI.

Still need to import INSPECTOR_NOTIFICATIONS into TreePanel.jsm and create a loader function for the Inspect menu item. Minor changes.
Comment on attachment 552908 [details] [diff] [review]
jsm wip

>--- a/browser/base/content/inspector.js
>+++ b/browser/base/content/inspector.jsm

>+__defineGetter__("window", function() {
>+  delete this.window;
>+  let win = Services.wm.getMostRecentWindow("navigator:browser");
>+  return this.window = win;
>+});
>+
>+__defineGetter__("gBrowser", function() {
>+  delete this.gBrowser;
>+  let gBrowser = window.gBrowser;
>+  return this.gBrowser = gBrowser;
>+});
>+
>+__defineGetter__("document", function() {
>+  delete this.document;
>+  let doc = window.document;
>+  return this.document = doc;
>+});

This looks quite broken. Invoking the inspector a second time in a second window would let these properties still point to the first window. You're also leaking that window.

By the way, how about creating resource:///modules/devtools/?
(In reply to Dão Gottwald [:dao] from comment #4)
> Comment on attachment 552908 [details] [diff] [review]
> jsm wip
> 
> >--- a/browser/base/content/inspector.js
> >+++ b/browser/base/content/inspector.jsm
> 
> >+__defineGetter__("window", function() {
> >+  delete this.window;
> >+  let win = Services.wm.getMostRecentWindow("navigator:browser");
> >+  return this.window = win;
> >+});
> >+
> >+__defineGetter__("gBrowser", function() {
> >+  delete this.gBrowser;
> >+  let gBrowser = window.gBrowser;
> >+  return this.gBrowser = gBrowser;
> >+});
> >+
> >+__defineGetter__("document", function() {
> >+  delete this.document;
> >+  let doc = window.document;
> >+  return this.document = doc;
> >+});
> 
> This looks quite broken. Invoking the inspector a second time in a second
> window would let these properties still point to the first window. You're
> also leaking that window.

mm. good point. I was hoping I could keep InspectorUI as a singleton, but it looks like I may need to actually instantiate them per window. Should be easier to cleanup that way too.

> By the way, how about creating resource:///modules/devtools/?

yeah, Mike Ratcliffe mentioned this last week in response to your comment in one of his bugs. Probably a good idea and worth doing as we grow more and more of these.

thanks for the driveby!
Priority: -- → P1
Blocks: 681975
As agreed with Robert, I am going to take this bug. Will get to update the patch some time this week.
Assignee: rcampbell → mihai.sucan
Waiting for the updated patch for bug 650794. It makes sense to base on top of the work to split out the tree panel - that has r+ and it's *almost* ready.
Depends on: 650794
Attached patch proposed patch (obsolete) — Splinter Review
This is the proposed patch. This migrates the inspector.js code to .jsm. InspectorUI instances are created per window. All features work, all tests pass.

Please note that this patch depends on:
- v16 patch attachment 558309 [details] [diff] [review] from bug 650794,
- which in turn depends on attachment 555734 [details] [diff] [review] from bug 681653.

Looking forward for your review. Thank you!

(Another note: I haven't moved the JSM to any different location for now. Dão's suggestion to use resource://browser/devtools/ is a good one, and we should do that. Let's discuss it on IRC or somewhere.)
Attachment #552908 - Attachment is obsolete: true
Attachment #558329 - Flags: review?(rcampbell)
Attached patch rebased patch (obsolete) — Splinter Review
Rebased the patch and also moved the jsm and the inspector test files to browser/devtools/highlighter/.

Please note that the local patch queue is as follows:
bug 681653, bug 663831, bug 650794, bug 681651, bug 562168

Please let me know if I forgot other files to move to devtools.

Looking forward to your review! Thanks!
Attachment #558329 - Attachment is obsolete: true
Attachment #561006 - Flags: review?(rcampbell)
Attachment #558329 - Flags: review?(rcampbell)
Depends on: 681651
Attached patch another rebase (obsolete) — Splinter Review
Rebased on top of the latest attachment 561225 [details] [diff] [review] from bug 687854.
Attachment #561006 - Attachment is obsolete: true
Attachment #561496 - Flags: review?(rcampbell)
Attachment #561006 - Flags: review?(rcampbell)
Depends on: 687854
No longer depends on: 650794, 681651, reticle
just rebasing on latest patches and checkins.

reviewing...
Attachment #561496 - Attachment is obsolete: true
Attachment #561713 - Flags: review?(rcampbell)
Attachment #561496 - Flags: review?(rcampbell)
Comment on attachment 561713 [details] [diff] [review]
yet another rebase

r+ with gavin's modifications applied.
Attachment #561713 - Flags: review?(rcampbell) → review+
fixed by bug 687854.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.