Closed
Bug 690713
Opened 13 years ago
Closed 13 years ago
[highlighter] move DOM related methods from TreePanel.jsm to a new jsm
Categories
(DevTools :: General, defect)
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)
8.42 KB,
patch
|
msucan
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•13 years ago
|
||
Comment 2•13 years ago
|
||
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•13 years ago
|
||
Addressed rob's comment.
Attachment #563711 -
Attachment is obsolete: true
Attachment #564565 -
Flags: review?(rcampbell)
Comment 4•13 years ago
|
||
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+
Comment 5•13 years ago
|
||
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•13 years ago
|
||
(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•13 years ago
|
||
ok
Assignee | ||
Comment 8•13 years ago
|
||
Attachment #564565 -
Attachment is obsolete: true
Attachment #565906 -
Flags: review?(mihai.sucan)
Assignee | ||
Comment 9•13 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•13 years ago
|
||
Comment on attachment 565906 [details] [diff] [review] patch v2 Nope, it should be good actually.
Attachment #565906 -
Flags: review?(mihai.sucan)
Comment 11•13 years ago
|
||
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•13 years ago
|
||
Mihai, better?
Attachment #565906 -
Attachment is obsolete: true
Attachment #566144 -
Flags: review?(mihai.sucan)
Comment 13•13 years ago
|
||
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•13 years ago
|
Whiteboard: [land-in-fx-team]
Comment 14•13 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/dbfe5c55ee81
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
Comment 15•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/dbfe5c55ee81
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•