Remove optional trailing parameters

RESOLVED FIXED in Firefox 55

Status

()

Firefox
General
RESOLVED FIXED
12 days ago
7 days ago

People

(Reporter: florian, Assigned: florian)

Tracking

(Depends on: 1 bug)

unspecified
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(9 attachments)

(Assignee)

Description

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

Comment 1

12 days ago
Created attachment 8858304 [details] [diff] [review]
script-generated patch for addObserver
Attachment #8858304 - Flags: review?(jaws)
(Assignee)

Comment 2

12 days ago
Created attachment 8858306 [details] [diff] [review]
script-generated patch for appendElement
Attachment #8858306 - Flags: review?(jaws)
(Assignee)

Comment 3

12 days ago
Created attachment 8858308 [details] [diff] [review]
script-generated patch for notifyObservers
Attachment #8858308 - Flags: review?(jaws)
(Assignee)

Comment 4

12 days ago
Created attachment 8858310 [details] [diff] [review]
additional newURI patch

This is mostly script-generated, but I handled 2 or 3 cases by hand.
Attachment #8858310 - Flags: review?(jaws)
(Assignee)

Comment 5

12 days ago
Created attachment 8858311 [details] [diff] [review]
Update the no-useless-parameters eslint rule
Attachment #8858311 - Flags: review?(jaws)
(Assignee)

Comment 6

12 days ago
Created attachment 8858313 [details]
addObserver xpcshell script
(Assignee)

Comment 7

12 days ago
Created attachment 8858315 [details]
appendElement xpcshell script
(Assignee)

Comment 8

12 days ago
Created attachment 8858316 [details]
notifyObservers xpcshell script
(Assignee)

Comment 9

12 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4134f0960ff070f394daabe9fad62badb577a1a5
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+
(Assignee)

Comment 12

12 days ago
(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.
(Assignee)

Comment 14

12 days ago
Created attachment 8858373 [details] [diff] [review]
fix test_SyncedTabsDeckComponent.js
Attachment #8858373 - Flags: review?(jaws)
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+

Comment 16

12 days ago
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?
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
Flags: needinfo?(florian)

Comment 19

12 days ago
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.
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
Status: NEW → RESOLVED
Last Resolved: 11 days ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
(Assignee)

Updated

8 days ago
Flags: needinfo?(florian)
Depends on: 1357345
You need to log in before you can comment on or make changes to this bug.