Closed Bug 690713 Opened 10 years ago Closed 10 years ago

[highlighter] move DOM related methods from TreePanel.jsm to a new jsm

Categories

(DevTools :: General, defect)

x86
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 10

People

(Reporter: paul, Assigned: paul)

References

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(1 file, 3 obsolete files)

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).
Blocks: 672006
Attached patch patch v1 (obsolete) — Splinter Review
Assignee: nobody → paul
Status: NEW → ASSIGNED
Attachment #563711 - Flags: review?(rcampbell)
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.
Attachment #563711 - Flags: review?(rcampbell) → review+
Attached patch patch v1.1 (obsolete) — Splinter Review
Addressed rob's comment.
Attachment #563711 - Attachment is obsolete: true
Attachment #564565 - Flags: review?(rcampbell)
Comment on attachment 564565 [details] [diff] [review]
patch v1.1

let's give this a run through try. Should be fine.
Attachment #564565 - Flags: review?(rcampbell) → review+
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.
(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.
ok
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #564565 - Attachment is obsolete: true
Attachment #565906 - Flags: review?(mihai.sucan)
Comment on attachment 565906 [details] [diff] [review]
patch v2

Cancelling review, still some work to do.
Attachment #565906 - Flags: review?(mihai.sucan)
Comment on attachment 565906 [details] [diff] [review]
patch v2

Nope, it should be good actually.
Attachment #565906 - Flags: review?(mihai.sucan)
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)
Attachment #565906 - Flags: review?(mihai.sucan) → review+
Attached patch patch v2.1Splinter Review
Mihai, better?
Attachment #565906 - Attachment is obsolete: true
Attachment #566144 - Flags: review?(mihai.sucan)
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)
Attachment #566144 - Flags: review?(mihai.sucan) → review+
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/dbfe5c55ee81
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/dbfe5c55ee81
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.