970605 - Use node.remove(), especially instead of node.parentNode.removeChild(node)

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: newsblog, v1

Like pointed out on firefox-dev, node.remove() should be used, especially instead of node.parentNode.removeChild(node).

Patches in total remove 10th line touched.

Comment 1

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: mailnews, v1

Comment 2

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: mail, v1

Comment 3

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: chat / im, v1

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: editor, v1

Try run with the patches applied: https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=fd55f8ee2d3d

Comment 5

[Ian Neal]

I presume suite/ and calendar/ patches are coming too?

Comment 6

[Florian Quèze [:florian]]

Comment on attachment 8373674 [details] [diff] [review]
chat / im, v1

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

2 nits; looks great otherwise, thanks!

::: mail/components/im/content/am-im.js
@@ +105,5 @@
>    },
>  
>    populateProtoSpecificBox: function account_populate() {
>      var gbox = document.getElementById("protoSpecific");
>      let child;

This variable isn't used any more, please remove it.

::: mail/components/im/content/imAccountWizard.js
@@ +126,5 @@
>      document.getElementById("usernameInfo").textContent = usernameInfo;
>  
>      var vbox = document.getElementById("userNameBox");
>      // remove anything that may be there for another protocol
>      var child;

Same here.

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 8373670 [details] [diff] [review]
mail, v1

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

Looks good, aryx! r=mkelin

Comment 8

[Ian Neal]

Comment on attachment 8373669 [details] [diff] [review]
mailnews, v1

>+++ b/mailnews/base/prefs/content/AccountManager.js
>-    while (mainTree.firstChild)
>-      mainTree.removeChild(mainTree.firstChild);
>+    while (mainTree.hasChildNodes())
>+      mainTree.lastChild.remove();

>+++ b/mailnews/base/prefs/content/accountcreation/emailWizard.js
>     while (popup.hasChildNodes())
>-      popup.removeChild(popup.firstChild);
>+      popup.lastChild.remove();

>+++ b/mailnews/db/gloda/content/glodacomplete.xml
>           while (aDescriptionElement.hasChildNodes())
>-            aDescriptionElement.removeChild(aDescriptionElement.firstChild);
>+            aDescriptionElement.lastChild.remove();

>           while (this._identityHolder.hasChildNodes())
>-            this._identityHolder.removeChild(this._identityHolder.firstChild);
>+            this._identityHolder.firstChild.remove();

Just wondering why the change for one of these four is different to the rest. r=me with this addressed.

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: chat / im, v2, r=florian

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: editor, v2, r=Neil

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: mailnews, v2, r=IanN

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: mail, v2, r=mkmelin

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: newsblog, v2, r=IanN

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/694f62f8834a
https://hg.mozilla.org/comm-central/rev/875250f407aa
https://hg.mozilla.org/comm-central/rev/245701d317e7
https://hg.mozilla.org/comm-central/rev/6b9c9c7190e8
https://hg.mozilla.org/comm-central/rev/c167539f1169

Comment 15

[:aceman]

For completeness, is this about the method here feature https://developer.mozilla.org/en-US/docs/Web/API/ChildNode.remove ?

Thanks for another sweep through the tree, Aryx :)

What about comment 5?

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

suite has already landed, see 972331. For calendar, I want to fix the mozmill tests before (bug 975738). https://developer.mozilla.org/en-US/docs/Web/API/ChildNode.remove seems to be wrong because .remove() doesn't returning anything (.removeChild returns the removed node).

Comment 17

[:aceman]

OK, I see bug 972331. It has the same summary as this one so if none of them is the meta bug, you could at least distinguish their summaries (one is for TB, one for SM) :)