provider must be re-usable after having been terminated

RESOLVED FIXED in Firefox 16

Status

()

RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Gavin, Assigned: Gavin)

Tracking

Trunk
Firefox 16
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Created attachment 640146 [details] [diff] [review]
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).
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #640146 - Flags: feedback?(mixedpuppy)
Attachment #640146 - Flags: feedback?(jaws)
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.
Attachment #640146 - Flags: feedback?(mixedpuppy) → feedback+
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...
Attachment #640146 - Flags: feedback?(jaws) → review+
Created attachment 640875 [details] [diff] [review]
patch

Service "enabled" state is now in sync with the pref, which I add in this patch.
Attachment #640146 - Attachment is obsolete: true
Attachment #640875 - Flags: review?(jaws)
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.
Attachment #640875 - Flags: review?(jaws) → review+
(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.
Created attachment 641113 [details] [diff] [review]
patch with comments addressed
Attachment #640875 - Attachment is obsolete: true
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/c612d081dc38
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Status: RESOLVED → UNCONFIRMED
Ever confirmed: false
Resolution: FIXED → ---
Target Milestone: Firefox 16 → ---
Adam: I'm assuming you didn't mean to reopen this!
Status: UNCONFIRMED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
You need to log in before you can comment on or make changes to this bug.