Note: There are a few cases of duplicates in user autocompletion which are being worked on.

provider must be re-usable after having been terminated

RESOLVED FIXED in Firefox 16

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
5 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+
Blocks: 765874
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)
https://tbpl.mozilla.org/?tree=Try&rev=116724e70916
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
Blocks: 771826
https://hg.mozilla.org/integration/mozilla-inbound/rev/c612d081dc38
Target Milestone: --- → Firefox 16
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/c612d081dc38
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
You need to log in before you can comment on or make changes to this bug.