771980 - provider must be re-usable after having been terminated

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

Shane brought this up in bug 771877 - after a provider has been terminated, it needs to be re-usable, since providers can be enabled/disabled as you toggle the global state on/off.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

This adds an "enabled" property to both the service and the provider objects. Perhaps we should hook the service's property to the browser.social.enabled pref. It just controls the state of all of its providers.

This changes the provider API slightly - instead of exposing getWorkerPort, there is now a single "provider.port", which is nulled out when the provider isn't enabled, and re-set when it is. provider.workerAPI works the same way. Is there a valid use case for having multiple ports to a frameworker? I've not seen one yet in the worker code I've read, but I admit to not having thought this through too much (and it's late).

Comment 2

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 640146 [details] [diff] [review]
patch

In the future when we do multi provider support provider.enabled would be a pref separate from service.enabled.

SocialService.enabled should get/set a pref rather than defaulting to enabled. We dont want to reenable on startup if the user disabled the service.

Comment 3

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

Comment on attachment 640146 [details] [diff] [review]
patch

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

r=me, for Fx16 this is simple and we can refactor this for future Firefox when we have multi-provider support.

::: toolkit/components/social/SocialService.jsm
@@ +45,5 @@
> +    let enable = !!val;
> +    if (enable == SocialServiceInternal.enabled)
> +      return;
> +
> +    let providers = [p for ([k, p] of Iterator(SocialServiceInternal.providers))];

out of curiosity, why specify |k| here and also below? it's not used...

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Service "enabled" state is now in sync with the pref, which I add in this patch.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

https://tbpl.mozilla.org/?tree=Try&rev=116724e70916

Comment 6

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

Comment on attachment 640875 [details] [diff] [review]
patch

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

r=me with the two changes requested.

::: toolkit/components/social/SocialProvider.jsm
@@ +22,2 @@
>   */
> +function SocialProvider(input, disabled) {

Can we put the variable in the positive and make it |enabled|?

::: toolkit/components/social/test/xpcshell/test_SocialService.js
@@ +22,5 @@
>    });
>    do_register_cleanup(function () MANIFEST_PREFS.deleteBranch(""));
>  
> +  // Enable the service for this test
> +  Services.prefs.setBoolPref("social.enabled", true);

This pref is not reset after the test ends.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

(In reply to Jared Wein [:jaws] from comment #6)
> Can we put the variable in the positive and make it |enabled|?

I wanted to leave it optional, so I took the lazy way. I'll make it enabled and add a check for undefined.

> This pref is not reset after the test ends.

Shouldn't matter for xpcshell, I think. I'll remove the other clearing of prefs that's pre-existing.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch with comments addressed

Comment 9

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c612d081dc38

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c612d081dc38

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Adam: I'm assuming you didn't mean to reopen this!