Status

()

enhancement
P2
normal
RESOLVED FIXED
Last year
3 months ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

(Blocks 1 bug)

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(21 attachments)

3.18 KB, patch
qdot
: review+
Details | Diff | Splinter Review
36.01 KB, patch
surkov
: review+
smaug
: review+
Details | Diff | Splinter Review
38.55 KB, patch
qdot
: review+
Details | Diff | Splinter Review
18.00 KB, patch
qdot
: review+
Details | Diff | Splinter Review
20.77 KB, patch
qdot
: review+
Details | Diff | Splinter Review
14.10 KB, patch
qdot
: review+
Details | Diff | Splinter Review
16.71 KB, patch
qdot
: review+
Details | Diff | Splinter Review
23.04 KB, patch
qdot
: review+
Details | Diff | Splinter Review
3.80 KB, patch
snorp
: review+
Details | Diff | Splinter Review
24.29 KB, patch
qdot
: review+
Details | Diff | Splinter Review
4.18 KB, patch
qdot
: review+
Details | Diff | Splinter Review
11.05 KB, patch
qdot
: review+
Details | Diff | Splinter Review
20.70 KB, patch
mossop
: review+
Details | Diff | Splinter Review
105.64 KB, patch
qdot
: review+
Details | Diff | Splinter Review
3.88 KB, patch
surkov
: review+
Details | Diff | Splinter Review
11.53 KB, patch
mossop
: review+
Details | Diff | Splinter Review
117.64 KB, patch
jdescottes
: review+
Details | Diff | Splinter Review
8.03 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
7.41 KB, patch
qdot
: review+
Details | Diff | Splinter Review
13.56 KB, patch
qdot
: review+
Details | Diff | Splinter Review
48.16 KB, patch
qdot
: review+
Details | Diff | Splinter Review
No description provided.
Depends on: 1456703
Priority: -- → P2
Depends on: 1463981
Depends on: 1464478
Depends on: 1464519
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
This needs pretty careful review for the update service changes, though I think
they're pretty mechanical and safe.
Attachment #8980935 - Flags: review?(dtownsend)
Depends on: 1460735
Comment on attachment 8980924 [details] [diff] [review]
part 2.  Remove nsIDOMNode usage from accessible/

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

there's dom nsFocusManager change in the patch, which looks good with me, but not in a11y module. You might need somebody else to check it.
Attachment #8980924 - Flags: review?(surkov.alexander) → review+
Comment on attachment 8980937 [details] [diff] [review]
part 15.  Stop using nsIDOMNode in the a11y talos tests

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

we might not need all tree methods, if only ID version method is used
Attachment #8980937 - Flags: review?(surkov.alexander) → review+
Comment on attachment 8980924 [details] [diff] [review]
part 2.  Remove nsIDOMNode usage from accessible/

Probably a good idea...  Olli, can you review the nsFocusManager bit of this patch, please?
Attachment #8980924 - Flags: review?(bugs)
> we might not need all tree methods, if only ID version method is used

The others are basically helpers for the id version which call themselves recursively.
Attachment #8980924 - Flags: review?(bugs) → review+
Comment on attachment 8980939 [details] [diff] [review]
part 17.  Stop using nsIDOMNode in devtools/

I think someone more active in devtools should review this one.
Attachment #8980939 - Flags: review?(ttromey) → review?(jdescottes)
Comment on attachment 8980939 [details] [diff] [review]
part 17.  Stop using nsIDOMNode in devtools/

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

Looks good to me. Two nits (up to you), one file to remove from the diff.

::: devtools/client/shared/widgets/view-helpers.js
@@ +312,5 @@
>  
>  /**
>   * A generic Item is used to describe children present in a Widget.
>   *
> + * This is basically a very thin wrapper around an Node, with a few

nit: there are a few "an Node" in this patch, could we replace " an node" -> " a node" everywhere?

::: devtools/shared/builtin-modules.js
@@ +36,5 @@
>    Element,
>    Event,
>    FileReader,
>    FormData,
> +  Node,

nit: could we keep the list sorted and move this before TextDecoder. 
Same comment for the next list (even though "InspectorUtils" is in the wrong spot in the second list)

::: package-lock.json
@@ -1,1 @@
>  {

this file was probably updated as a side effect (after running eslint?) and should not be in this changeset
Attachment #8980939 - Flags: review?(jdescottes) → review+
> nit: there are a few "an Node" in this patch, could we replace " an node" -> " a node" everywhere?

Good catch!  I thought I checked those after doing the search+replace, but clearly not well enough.

> nit: could we keep the list sorted and move this before TextDecoder. 

Yes.  I tried to do that, and clearly failed....

> this file was probably updated as a side effect (after running eslint?)

Ugh, indeed.  I fixed that once, but eslint is a gift that just keeps on giving.  I filed bug 1465164 on that.  Excellent catch.
Attachment #8980935 - Flags: review?(dtownsend) → review+
Attachment #8980938 - Flags: review?(dtownsend) → review+
Attachment #8980923 - Flags: review?(kyle) → review+
Attachment #8980940 - Flags: review?(mrbkap) → review+
Attachment #8980925 - Flags: review?(kyle) → review+
Attachment #8980926 - Flags: review?(kyle) → review+
Attachment #8980927 - Flags: review?(kyle) → review+
Attachment #8980928 - Flags: review?(kyle) → review+
Attachment #8980929 - Flags: review?(kyle) → review+
Attachment #8980930 - Flags: review?(kyle) → review+
Attachment #8980932 - Flags: review?(kyle) → review+
Attachment #8980933 - Flags: review?(kyle) → review+
Attachment #8980934 - Flags: review?(kyle) → review+
Attachment #8980936 - Flags: review?(kyle) → review+
Attachment #8980941 - Flags: review?(kyle) → review+
Attachment #8980942 - Flags: review?(kyle) → review+
Comment on attachment 8980943 [details] [diff] [review]
part 21.  Remove nsIDOMNode

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

::: dom/base/test/test_bug352728.html
@@ +50,5 @@
>      var comment = document.createComment(aText);
>      var types = [ Comment, CharacterData, Node ];
>      checkTypes(comment, "comment", types);
>  
> +    var interfaces = [];

nit: here and below, this passes to a function that runs a for-loop on the interfaces array, so it's basically a noop now. Can be removed.

::: dom/base/test/test_bug352728.xhtml
@@ +74,5 @@
>      var comment = document.createComment(aText);
>      var types = [ Comment, CharacterData, Node ];
>      checkTypes(comment, "comment", types);
>  
> +    var interfaces = [];

nit: here and below, this passes to a function that runs a for-loop on the interfaces array, so it's basically a noop now. Can be removed.
Attachment #8980943 - Flags: review?(kyle) → review+

Comment 30

Last year
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c38176356708
part 1.  Make it possible to importGlobalProperties Node.  r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ccf1348f689
part 2.  Remove nsIDOMNode usage from accessible/.  r=surkov
https://hg.mozilla.org/integration/mozilla-inbound/rev/7161cb18122e
part 3.  Remove nsIDOMNode usage from widget/.  r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d81f28df4d2
part 4.  Remove nsIDOMNode usage from uriloader/.  r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/55a0726047bb
part 5.  Remove nsIDOMNode usage from netwerk/.  r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/9013597c936e
part 6.  Remove nsIDOMNode usage from docshell/.  r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/4eb337cfade1
part 7.  Remove nsIDOMNode usage from layout/inspector/.  r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e6146338754
part 8.  Remove nsIDOMNode usage from layout/.  r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/62ece2b265ab
part 9.  Mostly remove use of nsIDOMNode from mobile/.  r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/94ab3582901b
part 10.  Remove use of nsIDOMNode from remaining xpidl.  r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6e11e7c543c
part 11.  Remove use of nsIDOMNode from xpfe/.  r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/220898648f5f
part 12.  Remove use of nsIDOMNode from xpcom/.  r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b02b634a18a
part 13.  Remove use of nsIDOMNode from toolkit/.  r=mossop
https://hg.mozilla.org/integration/mozilla-inbound/rev/e46b5b302771
part 14.  Remove most use of nsIDOMNode in dom/.  r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/a81f7f1c0303
part 15.  Stop using nsIDOMNode in the a11y talos tests.  r=surkov
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e33b4da5aa6
part 16.  Mostly stop using nsIDOMNode in browser/.  r=mossop
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcf0bd0ee7bb
part 17.  Stop using nsIDOMNode in devtools/.  r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/8210471a0b5e
part 18.  Stop using getInterface as the primary API for the indexeddb permission notifications.  r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/94070be7a406
part 19.  Remove some remaining nsIDOMNode uses.  r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/5eb989e189c7
part 20.  Remove now-unused AsDOMNode methods.  r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/26548ddbd8d4
part 21.  Remove nsIDOMNode.  r=qdot
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.