The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 10

Status

()

Firefox
Developer Tools
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: paul, Assigned: paul)

Tracking

Trunk
Firefox 10
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
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).
(Assignee)

Updated

6 years ago
Blocks: 672006
(Assignee)

Comment 1

6 years ago
Created attachment 563711 [details] [diff] [review]
patch v1
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+
(Assignee)

Comment 3

6 years ago
Created attachment 564565 [details] [diff] [review]
patch v1.1

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

Comment 7

6 years ago
ok
(Assignee)

Comment 8

6 years ago
Created attachment 565906 [details] [diff] [review]
patch v2
Attachment #564565 - Attachment is obsolete: true
Attachment #565906 - Flags: review?(mihai.sucan)
(Assignee)

Comment 9

6 years ago
Comment on attachment 565906 [details] [diff] [review]
patch v2

Cancelling review, still some work to do.
Attachment #565906 - Flags: review?(mihai.sucan)
(Assignee)

Comment 10

6 years ago
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+
(Assignee)

Comment 12

6 years ago
Created attachment 566144 [details] [diff] [review]
patch v2.1

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+
(Assignee)

Updated

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
You need to log in before you can comment on or make changes to this bug.