Last Comment Bug 771344 - workerURL should be optional
: workerURL should be optional
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: Trunk
: All All
-- normal (vote)
: Firefox 16
Assigned To: :Gavin Sharp [email:]
: Shane Caraveo (:mixedpuppy)
Depends on:
  Show dependency treegraph
Reported: 2012-07-05 14:36 PDT by :Gavin Sharp [email:]
Modified: 2012-07-07 12:03 PDT (History)
1 user (show) in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (6.82 KB, patch)
2012-07-05 15:02 PDT, :Gavin Sharp [email:]
adw: review+
Details | Diff | Splinter Review

Description User image :Gavin Sharp [email:] 2012-07-05 14:36:54 PDT
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 User image :Gavin Sharp [email:] 2012-07-05 15:02:10 PDT
Created attachment 639480 [details] [diff] [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!
Comment 2 User image Drew Willcoxon :adw 2012-07-05 16:06:27 PDT
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.
Comment 3 User image :Gavin Sharp [email:] 2012-07-06 08:49:58 PDT
Made the test changes you suggested, but kept test_SocialService - we can always split it up again if it gets too unwieldy.
Comment 4 User image Ryan VanderMeulen [:RyanVM] 2012-07-07 12:03:31 PDT

Note You need to log in before you can comment on or make changes to this bug.