Status

()

P2
normal
RESOLVED FIXED
11 months ago
13 days 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
Comment hidden (empty)
Depends on: 1456703

Updated

11 months ago
Priority: -- → P2
(Assignee)

Updated

10 months ago
Depends on: 1463981
(Assignee)

Updated

10 months ago
Depends on: 1464478
(Assignee)

Updated

10 months ago
Depends on: 1464519
(Assignee)

Updated

10 months ago
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)
Attachment #8980943 - Flags: review?(kyle)
(Assignee)

Updated

10 months ago
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.

Updated

10 months ago
Attachment #8980924 - Flags: review?(bugs) → review+

Comment 26

10 months ago
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)
Attachment #8980931 - Flags: review?(snorp) → review+
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

10 months ago
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
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.