1356569 - Remove optional trailing parameters

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

[Florian Quèze [:florian]]

In bug 1355216 I made the last parameters of addObserver, appendElement and notifyObservers optional. Time to cleanup our codebase :).

Comment 1

[Florian Quèze [:florian]]

Attachment: script-generated patch for addObserver

Comment 2

[Florian Quèze [:florian]]

Attachment: script-generated patch for appendElement

Comment 3

[Florian Quèze [:florian]]

Attachment: script-generated patch for notifyObservers

Comment 4

[Florian Quèze [:florian]]

Attachment: additional newURI patch

This is mostly script-generated, but I handled 2 or 3 cases by hand.

Comment 5

[Florian Quèze [:florian]]

Attachment: Update the no-useless-parameters eslint rule

Comment 6

[Florian Quèze [:florian]]

Attachment: addObserver xpcshell script

Comment 7

[Florian Quèze [:florian]]

Attachment: appendElement xpcshell script

Comment 8

[Florian Quèze [:florian]]

Attachment: notifyObservers xpcshell script

Comment 9

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4134f0960ff070f394daabe9fad62badb577a1a5

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8858304 [details] [diff] [review]
script-generated patch for addObserver

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

rs=me

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8858311 [details] [diff] [review]
Update the no-useless-parameters eslint rule

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

::: toolkit/content/widgets/preferences.xml
@@ +180,5 @@
>            this.preferences.rootBranchInternal
>                .removeObserver(this.name, this.preferences);
>            this.setAttribute("name", val);
>            this.preferences.rootBranchInternal
> +              .addObserver(val, this.preferences);

Did this change sneak in here from the addObserver patch?

Comment 12

[Florian Quèze [:florian]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #11)
> Comment on attachment 8858311 [details] [diff] [review]
> Update the no-useless-parameters eslint rule
> 
> Review of attachment 8858311 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/content/widgets/preferences.xml
> @@ +180,5 @@
> >            this.preferences.rootBranchInternal
> >                .removeObserver(this.name, this.preferences);
> >            this.setAttribute("name", val);
> >            this.preferences.rootBranchInternal
> > +              .addObserver(val, this.preferences);
> 
> Did this change sneak in here from the addObserver patch?

I did it by hand because it caused an eslint error. My automated script didn't process this setter because it's not in a CDATA block. I can fold this into the addObserver patch if you prefer, but from previous similar bugs I remember that you prefer hand-made changes to be separate from script-generated changes.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Florian Quèze [:florian] [:flo] from comment #12)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #11)
> > Comment on attachment 8858311 [details] [diff] [review]
> > Update the no-useless-parameters eslint rule
> > 
> > Review of attachment 8858311 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: toolkit/content/widgets/preferences.xml
> > @@ +180,5 @@
> > >            this.preferences.rootBranchInternal
> > >                .removeObserver(this.name, this.preferences);
> > >            this.setAttribute("name", val);
> > >            this.preferences.rootBranchInternal
> > > +              .addObserver(val, this.preferences);
> > 
> > Did this change sneak in here from the addObserver patch?
> 
> I did it by hand because it caused an eslint error. My automated script
> didn't process this setter because it's not in a CDATA block. I can fold
> this into the addObserver patch if you prefer, but from previous similar
> bugs I remember that you prefer hand-made changes to be separate from
> script-generated changes.

Yes, it is fine to keep it in this patch. Thanks for the explanation. Also, I guess we should file a bug about getting this in a CDATA block.

Comment 14

[Florian Quèze [:florian]]

Attachment: fix test_SyncedTabsDeckComponent.js

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8858373 [details] [diff] [review]
fix test_SyncedTabsDeckComponent.js

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

A very precise test! r=me

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/322fde2d53bf
Remove addObserver's last parameter when it is false, r=jaws.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1f191aad863
Remove appendElement's last parameter when it is false, r=jaws.
https://hg.mozilla.org/integration/mozilla-inbound/rev/55f3df15eaa6
Remove notifyObservers' last parameter when it is falsy, r=jaws.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f85a9a62c5bb
Remove some more newURI null trailing parameters, r=jaws.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c724e1485608
Update the no-useless-parameters eslint rule to report trailing optional parameters for addObserver, appendElement and notifyObservers, r=jaws.
https://hg.mozilla.org/integration/mozilla-inbound/rev/02bf71459f40
fix test_SyncedTabsDeckComponent.js, r=jaws.

Comment 17

[Kris Maglione [:kmag]]

So, the thing about these mass style rewrites is that they cause merge headaches for people who are working on large patches.

In the future, can you please try to send a heads-up email to dev-firefox or another suitable list when you start working something like this, so people working on those patches can at least prepare?

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed these out so the backouts for bug 1355161 did apply:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d523c49136775a7187486dbc96858a2cb0383bfb
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3ab2d3bdab32c0d2776fe240c01e792367ce5d6
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e76e3e17f3828a6eaa629501ee00ddf7806e1bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/68d33da0be4282e9f0d4203a35699f4b9aea74c9
https://hg.mozilla.org/integration/mozilla-inbound/rev/294af09c9558557e6a33d3d67fa366d612f951c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3e2fb843717c5d79e5cf6a1a32f702b0c4340ca

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/56bf52d4ddeb
Remove addObserver's last parameter when it is false, r=jaws.
https://hg.mozilla.org/integration/mozilla-inbound/rev/78a33503027d
Remove appendElement's last parameter when it is false, r=jaws.
https://hg.mozilla.org/integration/mozilla-inbound/rev/aadfcd0ab3c3
Remove notifyObservers' last parameter when it is falsy, r=jaws.
https://hg.mozilla.org/integration/mozilla-inbound/rev/912a3bdc2409
Remove some more newURI null trailing parameters, r=jaws.
https://hg.mozilla.org/integration/mozilla-inbound/rev/932958782079
Update the no-useless-parameters eslint rule to report trailing optional parameters for addObserver, appendElement and notifyObservers, r=jaws.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bd131795d81
fix test_SyncedTabsDeckComponent.js, r=jaws.

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/56bf52d4ddeb
https://hg.mozilla.org/mozilla-central/rev/78a33503027d
https://hg.mozilla.org/mozilla-central/rev/aadfcd0ab3c3
https://hg.mozilla.org/mozilla-central/rev/912a3bdc2409
https://hg.mozilla.org/mozilla-central/rev/932958782079
https://hg.mozilla.org/mozilla-central/rev/9bd131795d81

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9992e10b341b
Remove addObserver's last parameter when it is false, rs=Fallen.
https://hg.mozilla.org/comm-central/rev/53145b875f09
Remove appendElement's last parameter when it is false, rs=Fallen.
https://hg.mozilla.org/comm-central/rev/c1bac684acda
Remove notifyObservers' last parameter when it is falsy, rs=Fallen.
https://hg.mozilla.org/comm-central/rev/6ad50e5e75fa
Remove some more newURI null trailing parameters, rs=Fallen.