1128736 - Add a way to pref off prpls.

Status: RESOLVED FIXED

(Chat Core :: General, enhancement)

Description

[Patrick Cloke [:clokep]]

Attachment: Patch v1

This is similar to bug 955489, but adds the ability to pref off a prpl entirely (instead of choosing between a JS and a libpurple version).

The code in bug 955489 is specific to Instantbird, we need something shared between Instantbird and Thunderbird (i.e. in chat core).

Add a pref like chat.prpls.disable.prpl-irc and you will be unable to create a new IRC account (it will be removed from the list). Note that directly trying to load a prpl by ID will still work (so previous accounts will be unaffected, and anything in the "top protocols list" will be unaffected).

This doesn't actually add any preferences, but bug 953999 will need this.

Comment 1

[Florian Quèze [:florian]]

Comment on attachment 8558177 [details] [diff] [review]
Patch v1

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

::: chat/components/src/imCore.js
@@ +249,5 @@
>    init: function() {
>      if (this._initialized)
>        return;
>  
> +    initLogModule("core", this);

Doing this early at startup unconditionally for an edge case that hopefully won't happen much doesn't seem great.

@@ +313,5 @@
> +      // If the preference is set to disable this prpl, don't show it in the
> +      // full list of protocols.
> +      let pref = kPrefDisablePrplBranch + "." + id;
> +      if (Services.prefs.getPrefType(pref) != Services.prefs.PREF_INVALID &&
> +          Services.prefs.getBoolPref(pref)) {

You are checking the type, so you should verify it's a boolean. If it's a string, getBoolPref will throw...

Is there a reason why doing 2 calls to the pref service for each prpl we load is better than having a comma separated list in a single pref read outside the loop?

Comment 2

[Patrick Cloke [:clokep]]

(In reply to Florian Quèze [:florian] [:flo] from comment #1)
> ::: chat/components/src/imCore.js
> @@ +249,5 @@
> >    init: function() {
> >      if (this._initialized)
> >        return;
> >  
> > +    initLogModule("core", this);
> 
> Doing this early at startup unconditionally for an edge case that hopefully
> won't happen much doesn't seem great.

We should likely have logging in this code in general.

> @@ +313,5 @@
> > +      // If the preference is set to disable this prpl, don't show it in the
> > +      // full list of protocols.
> > +      let pref = kPrefDisablePrplBranch + "." + id;
> > +      if (Services.prefs.getPrefType(pref) != Services.prefs.PREF_INVALID &&
> > +          Services.prefs.getBoolPref(pref)) {
> 
> You are checking the type, so you should verify it's a boolean. If it's a
> string, getBoolPref will throw...

Yeah, my bad. I was thinking this would let us save ourselves some trouble if someone made e.g. an int pref by mistake. But it just won't work.

> Is there a reason why doing 2 calls to the pref service for each prpl we
> load is better than having a comma separated list in a single pref read
> outside the loop?
Yes, it let's us actually modify what's enabled/disabled via the application. If it is a single pref and the user modifies it then you can't do this. Are we really concerned about this many pref reads? For the record, this does *not* happen on boot. It only happens when clicking the "next" button on the new account wizard.

Florian suggested we could change the pref name each time to ensure we can properly read this...but that would be super annoying for people who DO want to enable some prpls manually. E.g. if we have two things disabled (foo and bar). If a user wants to enable only bar, they edit the pref. Now when we change the pref name to enable foo, all of a sudden they have foo enabled and not bar.

On IRC Mook suggested I change the pref to chat.prpls.<ID>.disabled, which I think I like better. (And we should probably attempt to namespaces things like that in the future!)

Florian asked on IRC if disabling only creation of a prpl is really a feature. I think it is, it will ease the lives of our testers if we end up flip-flopping on if something is enabled or not.

I don't want to bikeshed too much on this, but if there is some serious performance issue with the implementation, we should choose a different way.

Comment 3

[Patrick Cloke [:clokep]]

Attachment: Patch v2

Comment 4

[Florian Quèze [:florian]]

Comment on attachment 8558847 [details] [diff] [review]
Patch v2

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

Fair enough.

Comment 5

[Patrick Cloke [:clokep]]

https://hg.mozilla.org/comm-central/rev/65cb08f4bfef