Closed Bug 1265854 Opened 4 years ago Closed 4 years ago

replace uses of nsIDOMNode constants

Categories

(DevTools :: Framework, enhancement, P1)

enhancement

Tracking

(firefox49 fixed)

RESOLVED FIXED
Firefox 49
Iteration:
49.3 - Jun 6
Tracking Status
firefox49 --- fixed

People

(Reporter: tromey, Assigned: jlong)

References

Details

(Whiteboard: [devtools-html])

Attachments

(1 file, 2 obsolete files)

In the inspector at least, nsIDOMNode seems to be used only for constants.
These uses need to be replaced for the devtools de-chrome-ification project.
devtools.html did this by:

    nsIDOMNode: HTMLElement,

so perhaps this idea can be reused.
Assignee: nobody → jlong
Status: NEW → ASSIGNED
Iteration: --- → 49.1 - May 9
Flags: qe-verify-
Priority: -- → P1
Iteration: 49.1 - May 9 → 49.2 - May 23
Attached patch 1265854.patch (obsolete) — Splinter Review
Attachment #8754879 - Flags: review?(ttromey)
Comment on attachment 8754879 [details] [diff] [review]
1265854.patch

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

Thank you for doing this.

I think this is all good, but I wonder if it would be simpler and more webbish to just define "Node" globally in Loader.jsm as nsIDOMNode; and then use Node.SOME_CONSTANT everywhere.  The developer-toolbar.js change gave me this idea.
Attachment #8754879 - Flags: review?(ttromey) → review+
That's not a bad idea, although we've been trying to minimize the loader magic. Ideally, everything would be loaded through BrowserLoader and we could just have direct access to those APIs. But I'll think about the loader approach.
Couldn't push to try before, just got it working: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4193be266b40 will still think about the loader idea
Iteration: 49.2 - May 23 → 49.3 - Jun 6
Attached patch 1265854.patch (obsolete) — Splinter Review
I'm hesistant to touch Loader.jsm. That's been a source pain several times. I'd rather not keep "faking" a web environment, and this approach is pretty simple so I think we should continue with it.

This patch fixes a few last things. If the try run is green I will land it.
Attached patch 1265854.patchSplinter Review
rebased
Attachment #8754879 - Attachment is obsolete: true
Attachment #8757502 - Attachment is obsolete: true
Meant to do this earlier; previous try run had a ton of unrelated (I hope) failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=236b3121826b
There's been a lot of unrelated errors on try. But I saw one bustage that was my fault. Going to do another run just in case: https://treeherder.mozilla.org/#/jobs?repo=try&revision=22088c0f66cd
Pushed by jlong@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/be6fddfc1390
replace uses of nsiDOMNode constants in devtools frontend r=tromey
https://hg.mozilla.org/mozilla-central/rev/be6fddfc1390
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Depends on: 1305416
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.