Closed Bug 970605 Opened 7 years ago Closed 7 years ago

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

Categories

(Thunderbird :: General, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 30.0

People

(Reporter: aryx, Assigned: aryx)

References

Details

Attachments

(5 files, 5 obsolete files)

39.58 KB, patch
Details | Diff | Splinter Review
14.84 KB, patch
Details | Diff | Splinter Review
18.60 KB, patch
Details | Diff | Splinter Review
52.66 KB, patch
Details | Diff | Splinter Review
1.38 KB, patch
Details | Diff | Splinter Review
Attached patch newsblog, v1 (obsolete) — Splinter Review
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.
Attachment #8373668 - Flags: review?(iann_bugzilla)
Attached patch mailnews, v1 (obsolete) — Splinter Review
Assignee: nobody → archaeopteryx
Status: NEW → ASSIGNED
Attachment #8373669 - Flags: review?(iann_bugzilla)
Attached patch mail, v1 (obsolete) — Splinter Review
Attachment #8373670 - Flags: review?(mkmelin+mozilla)
Attached patch chat / im, v1 (obsolete) — Splinter Review
Attachment #8373674 - Flags: review?(clokep)
I presume suite/ and calendar/ patches are coming too?
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.
Attachment #8373674 - Flags: review?(clokep) → review+
Attachment #8373678 - Flags: review?(neil) → review+
Comment on attachment 8373670 [details] [diff] [review]
mail, v1

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

Looks good, aryx! r=mkelin
Attachment #8373670 - Flags: review?(mkmelin+mozilla) → review+
Attachment #8373668 - Flags: review?(iann_bugzilla) → review+
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.
Attachment #8373669 - Flags: review?(iann_bugzilla) → review+
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?
Flags: needinfo?(archaeopteryx)
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).
Flags: needinfo?(archaeopteryx)
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) :)
Depends on: 980089
Depends on: 982752
You need to log in before you can comment on or make changes to this bug.