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

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement, P3)

Description

[Dão Gottwald [:dao]]

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

Comment 1

[Dão Gottwald [:dao]]

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?

Comment 2

[Florian Quèze [: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).

Comment 3

[Sophie Helf]

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!

Comment 4

[Dão Gottwald [:dao]]

(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.

Comment 5

[kev.ftw595]

HI can I work on the bug?

Comment 6

[akira.jainn2]

HI can I work on the bug?

Comment 7

[Dão Gottwald [:dao]]

(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.

Comment 8

[akira.jainn2]

working on this

Comment 9

[Dão Gottwald [:dao]]

(In reply to akira.jainn2 from comment #8)
> working on this

Any update? Please let me know if you need help.

Comment 10

[constfilin]

@dao: can I please get this assigned to myself???

Comment 11

[Dão Gottwald [:dao]]

(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.

Comment 12

[constfilin]

Attachment: Bug 1446721: replacing parent.removeChild(child) with simpler child.remove;

Review commit: https://reviewboard.mozilla.org/r/244570/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/244570/

Comment 13

[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 14

[Dão Gottwald [:dao]]

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();

Comment 15

[constfilin]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244570/diff/1-2/

Comment 16

[constfilin]

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 17

[Dão Gottwald [:dao]]

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!

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ed45b1e16eff
replacing parent.removeChild(child) with simpler child.remove; r=dao

Comment 19

[constfilin]

Thanks, @dao. Is there anything else I need to do to get this merged and pushed across the finish line?

Comment 20

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/ed45b1e16eff

Comment 21

[Dão Gottwald [:dao]]

(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!