Last Comment Bug 690713 - [highlighter] move DOM related methods from TreePanel.jsm to a new jsm
: [highlighter] move DOM related methods from TreePanel.jsm to a new jsm
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: Firefox 10
Assigned To: Paul Rouget [:paul]
:
Mentors:
Depends on:
Blocks: 672006
  Show dependency treegraph
 
Reported: 2011-09-30 03:39 PDT by Paul Rouget [:paul]
Modified: 2011-10-11 13:00 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (9.97 KB, patch)
2011-09-30 05:03 PDT, Paul Rouget [:paul]
rcampbell: review+
Details | Diff | Review
patch v1.1 (10.60 KB, patch)
2011-10-04 08:58 PDT, Paul Rouget [:paul]
rcampbell: review+
Details | Diff | Review
patch v2 (8.22 KB, patch)
2011-10-10 05:16 PDT, Paul Rouget [:paul]
mihai.sucan: review+
Details | Diff | Review
patch v2.1 (8.42 KB, patch)
2011-10-11 01:23 PDT, Paul Rouget [:paul]
mihai.sucan: review+
Details | Diff | Review

Description Paul Rouget [:paul] 2011-09-30 03:39:13 PDT
Methods like getParentObject are now isolated in TreePanel.jsm, but will be needed for the breadcrumbs display. We should extract them and move them to a new module (Utils.jsm).
Comment 1 Paul Rouget [:paul] 2011-09-30 05:03:11 PDT
Created attachment 563711 [details] [diff] [review]
patch v1
Comment 2 Rob Campbell [:rc] (:robcee) 2011-10-03 13:12:49 PDT
Comment on attachment 563711 [details] [diff] [review]
patch v1

I was at first not sure we needed a constructor and prototype-based object for this, since I didn't think there was going to be any state stored in the object. But then I saw the treeWalker references.

I'm worried about leaks. This should have a destructor that handles cleanup of the treeWalker and any other references contained in here. R+ with those.
Comment 3 Paul Rouget [:paul] 2011-10-04 08:58:53 PDT
Created attachment 564565 [details] [diff] [review]
patch v1.1

Addressed rob's comment.
Comment 4 Rob Campbell [:rc] (:robcee) 2011-10-04 12:42:54 PDT
Comment on attachment 564565 [details] [diff] [review]
patch v1.1

let's give this a run through try. Should be fine.
Comment 5 Mihai Sucan [:msucan] 2011-10-05 10:45:11 PDT
Thoughts:

- this is firebug legacy code that shouldn't be ported so much into our codebase. The breadcrumbs code use of the code should be avoided and it's a corner case need, anyway.

- the JSM is named "DOMUtils", there's nsIDOMWindowUtils as well, which causes some confusion.

- note that when we add JSMs, others will be able to reuse our code. This is not some valuable and reusable API we are providing to the Firefox codebase. This is just some niche code from Firebug, under-documented.

- doesn't this need SR? See http://www.mozilla.org/hacking/reviewers.html

I would suggest moving this code into the TreePanel.jsm, and add an EXPORT for the new symbol. This allows you to reuse the methods in the breadcrumbs code, and avoids the problem of adding a new JSM to the Firefox codebase.
Comment 6 Rob Campbell [:rc] (:robcee) 2011-10-06 03:18:59 PDT
(In reply to Mihai Sucan [:msucan] from comment #5)
> Thoughts:
...
> I would suggest moving this code into the TreePanel.jsm, and add an EXPORT
> for the new symbol. This allows you to reuse the methods in the breadcrumbs
> code, and avoids the problem of adding a new JSM to the Firefox codebase.

Mihai brought this up yesterday and he's convinced me this is the right way to go. Leave it in TreePanel.jsm but export it.
Comment 7 Paul Rouget [:paul] 2011-10-06 06:21:02 PDT
ok
Comment 8 Paul Rouget [:paul] 2011-10-10 05:16:40 PDT
Created attachment 565906 [details] [diff] [review]
patch v2
Comment 9 Paul Rouget [:paul] 2011-10-10 05:22:53 PDT
Comment on attachment 565906 [details] [diff] [review]
patch v2

Cancelling review, still some work to do.
Comment 10 Paul Rouget [:paul] 2011-10-10 05:24:37 PDT
Comment on attachment 565906 [details] [diff] [review]
patch v2

Nope, it should be good actually.
Comment 11 Mihai Sucan [:msucan] 2011-10-10 11:34:26 PDT
Comment on attachment 565906 [details] [diff] [review]
patch v2

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

Patch looks fine. Thank you! A couple of nits below.

::: browser/devtools/highlighter/TreePanel.jsm
@@ +713,5 @@
>    }
>  };
>  
> +
> +function DOMHelpers(window) {

s/window/aWindow/

Please also add a comment that describes the constructor.

@@ +718,5 @@
> +  this.window = window;
> +};
> +
> +DOMHelpers.prototype = {
> +  getParentObject: function Utils_getParentObject(node)

s/Utils_/DOMHelpers_/

(throughout the file)
Comment 12 Paul Rouget [:paul] 2011-10-11 01:23:01 PDT
Created attachment 566144 [details] [diff] [review]
patch v2.1

Mihai, better?
Comment 13 Mihai Sucan [:msucan] 2011-10-11 01:25:44 PDT
Comment on attachment 566144 [details] [diff] [review]
patch v2.1

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

Yes, thank you!

(you don't need to ask again for review once you get r+ as long as you only make the changes the reviewer asked for)
Comment 14 Rob Campbell [:rc] (:robcee) 2011-10-11 06:25:23 PDT
https://hg.mozilla.org/integration/fx-team/rev/dbfe5c55ee81
Comment 15 Rob Campbell [:rc] (:robcee) 2011-10-11 13:00:58 PDT
https://hg.mozilla.org/mozilla-central/rev/dbfe5c55ee81

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