Last Comment Bug 562168 - Move inspector.js into a jsm
: Move inspector.js into a jsm
Status: RESOLVED FIXED
[minotaur][best: 1h; likely: 3h; wors...
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: x86 Mac OS X
: P1 normal (vote)
: ---
Assigned To: Mihai Sucan [:msucan]
:
: J. Ryan Stinnett [:jryans] (use ni?)
Mentors:
: 669652 (view as bug list)
Depends on: 687854
Blocks: 681975
  Show dependency treegraph
 
Reported: 2010-04-27 14:12 PDT by Rob Campbell [:rc] (:robcee)
Modified: 2011-09-26 10:12 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
jsm wip (5.21 KB, patch)
2011-08-13 15:16 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
proposed patch (40.22 KB, patch)
2011-09-05 13:14 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
rebased patch (51.30 KB, patch)
2011-09-19 13:40 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
another rebase (43.94 KB, patch)
2011-09-21 09:21 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
yet another rebase (43.82 KB, patch)
2011-09-22 06:00 PDT, Rob Campbell [:rc] (:robcee)
rcampbell: review+
Details | Diff | Splinter Review

Description Rob Campbell [:rc] (:robcee) 2010-04-27 14:12:30 PDT
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) 2011-08-03 11:50:42 PDT
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 2 Rob Campbell [:rc] (:robcee) 2011-08-05 09:06:21 PDT
*** Bug 669652 has been marked as a duplicate of this bug. ***
Comment 3 Rob Campbell [:rc] (:robcee) 2011-08-13 15:16:11 PDT
Created attachment 552908 [details] [diff] [review]
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] 2011-08-14 07:34:51 PDT
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) 2011-08-15 09:18:36 PDT
(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] 2011-08-30 11:32:49 PDT
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] 2011-09-03 06:23:35 PDT
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] 2011-09-05 13:14:35 PDT
Created attachment 558329 [details] [diff] [review]
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] 2011-09-19 13:40:01 PDT
Created attachment 561006 [details] [diff] [review]
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] 2011-09-21 09:21:28 PDT
Created attachment 561496 [details] [diff] [review]
another rebase

Rebased on top of the latest attachment 561225 [details] [diff] [review] from bug 687854.
Comment 11 Rob Campbell [:rc] (:robcee) 2011-09-22 06:00:08 PDT
Created attachment 561713 [details] [diff] [review]
yet another rebase

just rebasing on latest patches and checkins.

reviewing...
Comment 12 Rob Campbell [:rc] (:robcee) 2011-09-23 09:54:54 PDT
Comment on attachment 561713 [details] [diff] [review]
yet another rebase

r+ with gavin's modifications applied.
Comment 13 Rob Campbell [:rc] (:robcee) 2011-09-26 10:12:06 PDT
fixed by bug 687854.

Note You need to log in before you can comment on or make changes to this bug.