1455676 - Remove nsIDOMNode

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Make it possible to importGlobalProperties Node

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Remove nsIDOMNode usage from accessible/

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Remove nsIDOMNode usage from widget/

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Remove nsIDOMNode usage from uriloader/

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Remove nsIDOMNode usage from netwerk/

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Remove nsIDOMNode usage from docshell/

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Remove nsIDOMNode usage from layout/inspector/

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8. Remove nsIDOMNode usage from layout/

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 9. Mostly remove use of nsIDOMNode from mobile/

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 10. Remove use of nsIDOMNode from remaining xpidl

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 11. Remove use of nsIDOMNode from xpfe/

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: part 12. Remove use of nsIDOMNode from xpcom/

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 13. Remove use of nsIDOMNode from toolkit/

This needs pretty careful review for the update service changes, though I think
they're pretty mechanical and safe.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: part 14. Remove most use of nsIDOMNode in dom/

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: part 15. Stop using nsIDOMNode in the a11y talos tests

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: part 16. Mostly stop using nsIDOMNode in browser/

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: part 17. Stop using nsIDOMNode in devtools/

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: part 18. Stop using getInterface as the primary API for the indexeddb permission notifications

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: part 19. Remove some remaining nsIDOMNode uses

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: part 20. Remove now-unused AsDOMNode methods

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: part 21. Remove nsIDOMNode

Comment 22

[alexander :surkov (:asurkov)]

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.

Comment 23

[alexander :surkov (:asurkov)]

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

Comment 24

[Boris Zbarsky [:bzbarsky]]

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?

Comment 25

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 26

[Tom Tromey :tromey]

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.

Comment 27

[Julian Descottes [: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

Comment 28

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 29

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
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

Comment 31

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/c38176356708
https://hg.mozilla.org/mozilla-central/rev/1ccf1348f689
https://hg.mozilla.org/mozilla-central/rev/7161cb18122e
https://hg.mozilla.org/mozilla-central/rev/5d81f28df4d2
https://hg.mozilla.org/mozilla-central/rev/55a0726047bb
https://hg.mozilla.org/mozilla-central/rev/9013597c936e
https://hg.mozilla.org/mozilla-central/rev/4eb337cfade1
https://hg.mozilla.org/mozilla-central/rev/7e6146338754
https://hg.mozilla.org/mozilla-central/rev/62ece2b265ab
https://hg.mozilla.org/mozilla-central/rev/94ab3582901b
https://hg.mozilla.org/mozilla-central/rev/c6e11e7c543c
https://hg.mozilla.org/mozilla-central/rev/220898648f5f
https://hg.mozilla.org/mozilla-central/rev/3b02b634a18a
https://hg.mozilla.org/mozilla-central/rev/e46b5b302771
https://hg.mozilla.org/mozilla-central/rev/a81f7f1c0303
https://hg.mozilla.org/mozilla-central/rev/2e33b4da5aa6
https://hg.mozilla.org/mozilla-central/rev/bcf0bd0ee7bb
https://hg.mozilla.org/mozilla-central/rev/8210471a0b5e
https://hg.mozilla.org/mozilla-central/rev/94070be7a406
https://hg.mozilla.org/mozilla-central/rev/5eb989e189c7
https://hg.mozilla.org/mozilla-central/rev/26548ddbd8d4