Closed
Bug 970605
Opened 9 years ago
Closed 9 years ago
Use node.remove(), especially instead of node.parentNode.removeChild(node)
Categories
(Thunderbird :: General, enhancement)
Thunderbird
General
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 |
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)
![]() |
Assignee | |
Comment 1•9 years ago
|
||
Assignee: nobody → archaeopteryx
Status: NEW → ASSIGNED
Attachment #8373669 -
Flags: review?(iann_bugzilla)
![]() |
Assignee | |
Comment 2•9 years ago
|
||
Attachment #8373670 -
Flags: review?(mkmelin+mozilla)
![]() |
Assignee | |
Comment 3•9 years ago
|
||
Attachment #8373674 -
Flags: review?(clokep)
![]() |
Assignee | |
Comment 4•9 years ago
|
||
Try run with the patches applied: https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=fd55f8ee2d3d
Attachment #8373678 -
Flags: review?(neil)
Comment 6•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8373678 -
Flags: review?(neil) → review+
Comment 7•9 years ago
|
||
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+
![]() |
Assignee | |
Comment 9•9 years ago
|
||
Attachment #8373674 -
Attachment is obsolete: true
![]() |
Assignee | |
Comment 10•9 years ago
|
||
Attachment #8373678 -
Attachment is obsolete: true
![]() |
Assignee | |
Comment 11•9 years ago
|
||
Attachment #8373669 -
Attachment is obsolete: true
![]() |
Assignee | |
Comment 12•9 years ago
|
||
Attachment #8373670 -
Attachment is obsolete: true
![]() |
Assignee | |
Comment 13•9 years ago
|
||
Attachment #8373668 -
Attachment is obsolete: true
![]() |
Assignee | |
Updated•9 years ago
|
Keywords: checkin-needed
Comment 14•9 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 30.0
![]() |
||
Comment 15•9 years ago
|
||
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)
![]() |
Assignee | |
Comment 16•9 years ago
|
||
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)
![]() |
||
Comment 17•9 years ago
|
||
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) :)
You need to log in
before you can comment on or make changes to this bug.
Description
•