771344 - workerURL should be optional

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

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

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.

Comment 1

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

Attachment: patch

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!

Comment 2

[Drew Willcoxon :adw]

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

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.next.bind(runner)));
> +  runner.appendIterator(testGetProviderList(manifests, runner.next.bind(runner)));

Store runner.next.bind(runner) 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(providers.map(function (p) p.origin).indexOf(manifests[i].origin), -1);
> +    do_check_neq(providers.map(function (p) p.workerURL).indexOf(manifests[i].workerURL), -1);
> +    do_check_neq(providers.map(function (p) p.name).indexOf(manifests[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.

Comment 3

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

Made the test changes you suggested, but kept test_SocialService - we can always split it up again if it gets too unwieldy.

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

Comment 4

[Ryan VanderMeulen [:RyanVM]]

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