Closed Bug 771344 Opened 9 years ago Closed 9 years ago

workerURL should be optional


(Firefox Graveyard :: SocialAPI, defect)

Not set


(Not tracked)

Firefox 16


(Reporter: Gavin, Assigned: Gavin)



(1 file)

Shane points out that it's perfectly reasonable for a provider to not have a worker, so there's no reason to enforce the presence of a worker URL in the provider object.
Attached patch patchSplinter Review
This also merges the tests into one, since the duplication was getting to me (I know I just landed that a few hours ago). I'm not sure I understand the AsyncRunner magic well enough to be confident that this is right, but it seems to work!
Assignee: nobody →
Attachment #639480 - Flags: review?(adw)
Comment on attachment 639480 [details] [diff] [review]

Review of attachment 639480 [details] [diff] [review]:

I prefer small, separate test files per SocialService method rather than a big omnibus test_SocialService.js that's only going to get bigger.  (We can factor out common code into head.js, which is what head.js is for.)  I'd really like that changed back before landing, but I bet you disagree.

::: toolkit/components/social/test/xpcshell/test_getProvider.js
@@ +23,5 @@
>    Cu.import("resource://gre/modules/SocialService.jsm");
>    let runner = new AsyncRunner();
> +  runner.appendIterator(testGetProvider(manifests,;
> +  runner.appendIterator(testGetProviderList(manifests,;

Store in a local variable and pass that variable to the generator functions so that we're not repeating it every time a new generator is added.  (If you keep test_SocialService.js.)

@@ +45,5 @@
> +  do_check_true(providers.length >= manifests.length);
> +  for (let i = 0; i < manifests.length; i++) {
> +    do_check_neq( (p) p.origin).indexOf(manifests[i].origin), -1);
> +    do_check_neq( (p) p.workerURL).indexOf(manifests[i].workerURL), -1);
> +    do_check_neq( (p)[i].name), -1);

These just check that the origin, workerURL, and name exist in some provider in the list.  They don't ensure that all three are in the same provider.  What I would do is for each manifest, first find the provider with the right origin, and then check that its workerURL and name are right.
Attachment #639480 - Flags: review?(adw) → review+
Made the test changes you suggested, but kept test_SocialService - we can always split it up again if it gets too unwieldy.
Flags: in-testsuite+
Target Milestone: --- → Firefox 16
Closed: 9 years ago
Resolution: --- → FIXED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.