Closed Bug 771980 Opened 11 years ago Closed 11 years ago

provider must be re-usable after having been terminated

Categories

(Firefox Graveyard :: SocialAPI, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 16

People

(Reporter: Gavin, Assigned: Gavin)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
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+
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #640875 - Attachment is obsolete: true
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/c612d081dc38
Status: ASSIGNED → RESOLVED
Closed: 11 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
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.