562168 - Move inspector.js into a jsm

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[Rob Campbell [:rc] (:robcee)]

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.>);
});

Comment 1

[Rob Campbell [:rc] (:robcee)]

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.

Comment 3

[Rob Campbell [:rc] (:robcee)]

Attachment: jsm wip

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 4

[Dão Gottwald [:dao]]

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

Comment 5

[Rob Campbell [:rc] (:robcee)]

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

Comment 6

[Mihai Sucan [:msucan]]

As agreed with Robert, I am going to take this bug. Will get to update the patch some time this week.

Comment 7

[Mihai Sucan [:msucan]]

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.

Comment 8

[Mihai Sucan [:msucan]]

Attachment: proposed patch

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

Comment 9

[Mihai Sucan [:msucan]]

Attachment: rebased patch

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!

Comment 10

[Mihai Sucan [:msucan]]

Attachment: another rebase

Rebased on top of the latest attachment 561225 [details] [diff] [review] from bug 687854.

Comment 11

[Rob Campbell [:rc] (:robcee)]

Attachment: yet another rebase

just rebasing on latest patches and checkins.

reviewing...

Comment 12

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 561713 [details] [diff] [review]
yet another rebase

r+ with gavin's modifications applied.

Comment 13

[Rob Campbell [:rc] (:robcee)]

fixed by bug 687854.