Closed Bug 1446721 Opened 2 years ago Closed 2 years ago

Replace parent.removeChild(element) with element.remove() in tabbrowser.js

Categories

(Firefox :: Tabbed Browser, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 62
Tracking Status
firefox62 --- fixed

People

(Reporter: dao, Assigned: constfilin, Mentored)

Details

(Keywords: good-first-bug, Whiteboard: [lang=js])

Attachments

(1 file)

Instead of parent.removeChild(element), we should use the newer and simpler element.remove():

https://searchfox.org/mozilla-central/search?q=removeChild&case=true&regexp=false&path=tabbrowser.js
Priority: -- → P3
I see there's an eslint rule for this from bug 1334831. Florian, is this not enabled by default or why hasn't it caught this?
Flags: needinfo?(florian)
(In reply to Dão Gottwald [::dao] from comment #1)
> I see there's an eslint rule for this from bug 1334831. Florian, is this not
> enabled by default or why hasn't it caught this?

Because of the 'parent' temporary variable here. We are rejecting .parentNode.removeChild, and elt.removeChild(elt.firstChild), but there's nothing obviously wrong with var1.removeChild(var2).
Flags: needinfo?(florian)
Hi, I'm interested in working on this bug. Could you please point me in the right direction to get me started working on it? Thanks!
(In reply to Sophie Helf from comment #3)
> Hi, I'm interested in working on this bug. Could you please point me in the
> right direction to get me started working on it? Thanks!

Hi! Have you built Firefox yet? Have you read how to create a patch? Please let me know how far you got and where you potentially got stuck so I can help effectively.

The file that needs to be changed is browser/base/content/tabbrowser.js and I described in comment 0 what needs to change.
Flags: needinfo?(sophie.helf)
HI can I work on the bug?
HI can I work on the bug?
(In reply to akira.jainn2 from comment #6)
> HI can I work on the bug?

Alright! I haven't heard back from Sophie and it looks like Kev is working on bug 1447941.
Assignee: nobody → akira.jainn2
Status: NEW → ASSIGNED
Flags: needinfo?(sophie.helf)
working on this
(In reply to akira.jainn2 from comment #8)
> working on this

Any update? Please let me know if you need help.
Flags: needinfo?(akira.jainn2)
Assignee: akira.jainn2 → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(akira.jainn2)
@dao: can I please get this assigned to myself???
(In reply to constfilin from comment #10)
> @dao: can I please get this assigned to myself???

Sure... Let me know if something isn't clear about what needs to be done in this bug.
Assignee: nobody → constfilin
Thanks, @dao.

Please review:

submitting 1 changesets for review

changeset:  466557:5891c9ae66d2
summary:    Bug 1446721: replacing parent.removeChild(child) with simpler child.remove; r?dao
review:     https://reviewboard.mozilla.org/r/244570 (draft)

review id:  bz://1446721/mr31415926
review url: https://reviewboard.mozilla.org/r/244568 (draft)

publish these review requests now (Yn)?
(published review request 244568)

Thanks!
Comment on attachment 8976425 [details]
Bug 1446721: replacing parent.removeChild(child) with simpler child.remove;

https://reviewboard.mozilla.org/r/244570/#review250614

::: browser/base/content/tabbrowser.js:2135
(Diff revision 1)
>      }
>  
>      aBrowser.destroy();
>  
>      let notificationbox = this.getNotificationBox(aBrowser);
> -    this.tabpanels.removeChild(notificationbox);
> +    if( notificationbox ) notificationbox.remove();

Is this null-check really needed? I would think that the browser should always have a notification box at this point. Also, you could get rid of the notificationbox variable:

this.getNotificationBox(aBrowser).remove();
Attachment #8976425 - Flags: review?(dao+bmo)
ok, @dao. I made the fix, merged the two commits into a single review using "hg histedit"
Please take a look again at https://reviewboard.mozilla.org/r/244568/
Thanks!
Comment on attachment 8976425 [details]
Bug 1446721: replacing parent.removeChild(child) with simpler child.remove;

https://reviewboard.mozilla.org/r/244570/#review250748

Looks good. Thanks!
Attachment #8976425 - Flags: review?(dao+bmo) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ed45b1e16eff
replacing parent.removeChild(child) with simpler child.remove; r=dao
Thanks, @dao. Is there anything else I need to do to get this merged and pushed across the finish line?
https://hg.mozilla.org/mozilla-central/rev/ed45b1e16eff
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
(In reply to constfilin from comment #19)
> Thanks, @dao. Is there anything else I need to do to get this merged and
> pushed across the finish line?

Patches are merged automatically from autoland to central unless they cause tests to fail. Everything went smooth in this case. Well done!
You need to log in before you can comment on or make changes to this bug.