Last Comment Bug 771247 - lazify social service initialization, and add getProviderList method
: lazify social service initialization, and add getProviderList method
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 16
Assigned To: :Gavin Sharp [email: gavin@gavinsharp.com]
:
: Shane Caraveo (:mixedpuppy)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-05 11:33 PDT by :Gavin Sharp [email: gavin@gavinsharp.com]
Modified: 2012-07-05 17:29 PDT (History)
1 user (show)
gavin.sharp: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (4.06 KB, patch)
2012-07-05 11:33 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
adw: review+
Details | Diff | Splinter Review
patch with comments addressed (4.50 KB, patch)
2012-07-05 14:00 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review

Description :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-05 11:33:05 PDT
- Make initialization of SocialService API lazy (no side-effects to importing it)
- Add getProviderList API to enumerate available providers (with very basic test)
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-05 11:33:58 PDT
Created attachment 639411 [details] [diff] [review]
patch

This applies on top of the patch for bug 766403.
Comment 2 Drew Willcoxon :adw 2012-07-05 13:34:48 PDT
Comment on attachment 639411 [details] [diff] [review]
patch

Review of attachment 639411 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the getProviderList inconsistency addressed (and its test updated).

::: toolkit/components/social/SocialService.jsm
@@ +12,5 @@
>  const MANIFEST_PREFS = Services.prefs.getBranch("social.manifest.");
>  
> +let SocialServiceInternal = {};
> +
> +XPCOMUtils.defineLazyGetter(SocialServiceInternal, "providers", function () {

Just curious why you're using this new SocialServiceInternal object instead of defining providers on SocialService.  To keep it truly private from consumers?

Also, whenever we want to add more properties to SocialServiceInternal, we're going to have to use defineLazyGetter for all of them.  Don't you think it would be better to create SocialServiceInternal itself lazily rather than its properties?  Something like:

> XPCOMUtils.defineLazyGetter(this, "SocialServiceInternal", function () {
>   let internal = {
>     init: function () {
>       // populate internal.providers...
>     }
>   };
>   internal.init();
>   return internal;
> });

@@ +40,4 @@
>    },
>  
> +  // Returns an array of installed provider origins.
> +  getProviderList: function getProviderList(onDone) {

This method doesn't do what its name says it does.  getProvider returns a SocialProvider object, not an origin.  So this should either be renamed something like getOriginList or it should return a list of SocialProvider objects.  I think the latter is the much better option.

Also, the comment should probably mention that the ordering of the providers in the list is undefined.

::: toolkit/components/social/test/xpcshell/test_getProvider.js
@@ +24,5 @@
>  
>  function test(manifests, next) {
> +  let providers = yield SocialService.getProviderList(next);
> +  do_check_true(providers.length >= 3);
> +  do_check_neq(providers.indexOf(manifests[0].origin), -1);

The test adds three manifests, so this should check that providers for all three are in the list.

::: toolkit/components/social/test/xpcshell/xpcshell.ini
@@ +2,5 @@
>  head = head.js
>  tail =
>  
>  [test_getProvider.js]
> +[test_getProviderList.js]

This file isn't actually in the patch.  It looks like you modified test_getProvider.js instead. :)
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-05 13:42:24 PDT
(In reply to Drew Willcoxon :adw from comment #2)
> Just curious why you're using this new SocialServiceInternal object instead
> of defining providers on SocialService.  To keep it truly private from
> consumers?

Yeah, following a similar pattern to AddonManagerInternal, to keep from leaking internal details to module consumers (e.g. don't want extensions to muck with ._providers directly).

> Also, whenever we want to add more properties to SocialServiceInternal,
> we're going to have to use defineLazyGetter for all of them.  Don't you
> think it would be better to create SocialServiceInternal itself lazily
> rather than its properties?  Something like:

That might make sense in the future. Let's wait until we actually need to add stuff, and we can re-assess then.

> This method doesn't do what its name says it does.  getProvider returns a
> SocialProvider object, not an origin.  So this should either be renamed
> something like getOriginList or it should return a list of SocialProvider
> objects.  I think the latter is the much better option.

You're right - I chose to return an "origin list" because getProvider takes an origin, but it probably makes more sense to just return an array of providers as you suggest.

> The test adds three manifests, so this should check that providers for all
> three are in the list.

The addition of the default provider in bug 766403 means that there are more than just the test-added providers, so I had to account for that. I didn't really want to make the test depend on the default provider list.

> This file isn't actually in the patch.  It looks like you modified
> test_getProvider.js instead. :)

Just an hg diff being misleading (particularly if you used the bugzilla diff view) - I |hg cp|ed and modified test_getProvider.js, so it shows as a "copy from" + diff from the previous file.
Comment 4 Drew Willcoxon :adw 2012-07-05 13:47:39 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #3)
> The addition of the default provider in bug 766403 means that there are more
> than just the test-added providers, so I had to account for that. I didn't
> really want to make the test depend on the default provider list.

I get that, but I mean that the test adds three toy manifests before it does anything else.  Therefore you should ensure that getProviderList actually contains providers for those three manifests.  (Obviously the list may contain more providers corresponding to the default manifests, and the test doesn't have to worry about those.)

> Just an hg diff being misleading (particularly if you used the bugzilla diff
> view) - I |hg cp|ed and modified test_getProvider.js, so it shows as a "copy
> from" + diff from the previous file.

Huh.  That's interesting.
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-05 14:00:50 PDT
Created attachment 639462 [details] [diff] [review]
patch with comments addressed

Made the test actually (inefficiently) check that the test providers are all there.
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-05 14:03:44 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/e548482bc255
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-07-05 17:29:15 PDT
https://hg.mozilla.org/mozilla-central/rev/e548482bc255

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