1355216 - Mark last parameters that are almost always falsy as optional

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Florian Quèze [:florian]]

Attachment: xpcshell script

I wrote an xpcshell script that looks at all the method calls in JS code in browser/ and toolkit/ to identify methods that are almost always called with a falsy last parameter.

Here are the interesting results when looking for methods on Service.stuff.*:
Services.prefs.addObserver 80 out of 86 calls, 93%
Services.obs.notifyObservers 236 out of 361 calls, 65%
Services.obs.addObserver 542 out of 578 calls, 94%

And when looking at all methods (set the "onlyServices" variable to false in the script) :
appendElement 58 out of 58 calls, 100%
notifyObservers 248 out of 381 calls, 65%
addObserver 839 out of 958 calls, 88%

Comment 1

[Florian Quèze [:florian]]

Attachment: Patch

I did the change to all the addObserver methods in the tree that had an ownsWeak parameter, but I wouldn't mind doing it only to nsIPrefBranch and nsIObserverService if you prefer.

If this gets an r+, I'll file another bug to cleanup all the existing callers in the tree with a script, like I did in bug 1331081.

Comment 2

[Florian Quèze [:florian]]

On nsIMutableArray, I think it would make sense to do it on insertElementAt and replaceElementAt too for consistency, even if they have very few calls from JS.

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8856717 [details] [diff] [review]
Patch

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

Thanks for looking at this and producing a patch.

The only issue is that we really want most of these APIs to be split into separate functions.  For instance, nsIObserverService.registerObserver should really be two functions: registerStrongObserver and registerWeakObserver.  We can't really do this until we move completely to WebExtensions, since we have to maintain addon compat.  But I suppose this is a reasonable step along the way and makes things a little clearer in the interim, especially if we have linting rules to enforce things like in bug 1331081.

I'd be interested in seeing the script that does the replacement, since something like that will be useful once we don't have non-WebExtension addons and can do the refactoring outlined above.

::: xpcom/ds/nsIMutableArray.idl
@@ +35,5 @@
>       *                          but the element does not support
>       *                          nsIWeakReference.
>       */
> +    void appendElement(in nsISupports element,
> +                       [optional] in boolean weak);

I think we should just remove this parameter entirely; that should probably be a separate bug.

Comment 4

[Florian Quèze [:florian]]

(In reply to Nathan Froyd [:froydnj] from comment #3)

> I'd be interested in seeing the script that does the replacement, since
> something like that will be useful once we don't have non-WebExtension
> addons and can do the refactoring outlined above.

The script will definitely be attached to the bug where I'll be doing the mass cleanup.

> ::: xpcom/ds/nsIMutableArray.idl
> @@ +35,5 @@
> >       *                          but the element does not support
> >       *                          nsIWeakReference.
> >       */
> > +    void appendElement(in nsISupports element,
> > +                       [optional] in boolean weak);
> 
> I think we should just remove this parameter entirely; that should probably
> be a separate bug.

So do you want me to skip the nsIMutableArray.idl changes here and keep them for a separate bug? Or are you ok with making the parameter optional for JS callers as a first step? I don't have a script to rewrite all the C++ callers automatically.

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Florian Quèze [:florian] [:flo] from comment #4)
> So do you want me to skip the nsIMutableArray.idl changes here and keep them
> for a separate bug? Or are you ok with making the parameter optional for JS
> callers as a first step?

Making the parameter optional for JS callers as a first step is fine.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/84fda80ad999
Mark last parameters that are almost always falsy as optional, r=froydnj.

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/84fda80ad999