Closed Bug 1356569 Opened 7 years ago Closed 7 years ago

Remove optional trailing parameters

Categories

(Firefox :: General, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: florian, Assigned: florian)

References

Details

Attachments

(9 files)

In bug 1355216 I made the last parameters of addObserver, appendElement and notifyObservers optional. Time to cleanup our codebase :).
This is mostly script-generated, but I handled 2 or 3 cases by hand.
Attachment #8858310 - Flags: review?(jaws)
Comment on attachment 8858304 [details] [diff] [review]
script-generated patch for addObserver

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

rs=me
Attachment #8858304 - Flags: review?(jaws) → review+
Attachment #8858306 - Flags: review?(jaws) → review+
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?
Attachment #8858311 - Flags: review?(jaws) → review+
Attachment #8858308 - Flags: review?(jaws) → review+
Attachment #8858310 - Flags: review?(jaws) → review+
(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.
(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 on attachment 8858373 [details] [diff] [review]
fix test_SyncedTabsDeckComponent.js

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

A very precise test! r=me
Attachment #8858373 - Flags: review?(jaws) → review+
Pushed by florian@queze.net:
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.
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?
Pushed by florian@queze.net:
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.
Flags: needinfo?(florian)
Pushed by florian@queze.net:
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.
You need to log in before you can comment on or make changes to this bug.