Closed Bug 972331 Opened 6 years ago Closed 6 years ago

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

Categories

(SeaMonkey :: General, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.27

People

(Reporter: aryx, Assigned: aryx)

Details

Attachments

(6 files, 6 obsolete files)

20.08 KB, patch
Details | Diff | Splinter Review
31.81 KB, patch
Details | Diff | Splinter Review
1.07 KB, patch
Details | Diff | Splinter Review
1.22 KB, patch
Details | Diff | Splinter Review
19.81 KB, patch
Details | Diff | Splinter Review
2.44 KB, patch
Details | Diff | Splinter Review
Attached patch browser, v1 (obsolete) — Splinter Review
Like pointed out on firefox-dev, node.remove() should be used, especially instead of node.parentNode.removeChild(node).
Attachment #8375456 - Flags: review?(neil)
Attached patch common, v1 (obsolete) — Splinter Review
Attachment #8375457 - Flags: review?(iann_bugzilla)
Attached patch debugqa, v1 (obsolete) — Splinter Review
Attachment #8375458 - Flags: review?(iann_bugzilla)
Attached patch feeds, v1 (obsolete) — Splinter Review
Attachment #8375459 - Flags: review?(bugzilla)
Attached patch mailnews, v1 (obsolete) — Splinter Review
Attachment #8375460 - Flags: review?(mnyromyr)
Attached patch profile, v1 (obsolete) — Splinter Review
Attachment #8375461 - Flags: review?(neil)
Attachment #8375461 - Flags: review?(neil) → review+
Comment on attachment 8375456 [details] [diff] [review]
browser, v1

Thanks for doing this.

>         while (node.hasChildNodes())
>-            node.removeChild(node.firstChild);
>+            node.lastChild.remove();
>         node.appendChild(node.ownerDocument.createTextNode(value));
Nit: this block can more easily be written:

node.textContent = value;

>   if (element.localName == "textbox" || element.localName == "label")
>     element.value = value;
>   else {
>     if (element.hasChildNodes())
>-      element.removeChild(element.firstChild);
>+      element.firstChild.remove();
>     var textNode = document.createTextNode(value);
>     element.appendChild(textNode);
>   }
Again, this can be written:

else
  element.textContent = value;
Attachment #8375456 - Flags: review?(neil) → review+
Attachment #8375458 - Flags: review?(iann_bugzilla) → review+
Attachment #8375457 - Flags: review?(iann_bugzilla) → review+
Attachment #8375459 - Flags: review?(bugzilla) → review+
Version: unspecified → Trunk
Comment on attachment 8375460 [details] [diff] [review]
mailnews, v1

>+++ b/suite/mailnews/mailWidgets.xml

>             while (numItemsInNode && (numItemsInNode > numItemsToPreserve))
>             {
>-              aParentNode.removeChild(aParentNode.childNodes[numItemsInNode-1]);
>+              aParentNode.childNodes[numItemsInNode-1].remove();
Could you put spaces either side of the minus sign whilst you're changing this line?

>               numItemsInNode = numItemsInNode - 1;
>             }

>+++ b/suite/mailnews/msgHdrViewOverlay.js

>+     while ( popup.firstChild.localName == 'menu' )
Could you remove the spaces between the () and the expression?

>+       popup.firstChild.remove();
Attachment #8375460 - Flags: review?(mnyromyr) → review+
You need to log in before you can comment on or make changes to this bug.