Closed Bug 771247 Opened 12 years ago Closed 12 years ago

lazify social service initialization, and add getProviderList method

Categories

(Firefox Graveyard :: SocialAPI, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 16

People

(Reporter: Gavin, Assigned: Gavin)

Details

Attachments

(1 file, 1 obsolete file)

- Make initialization of SocialService API lazy (no side-effects to importing it)
- Add getProviderList API to enumerate available providers (with very basic test)
Attached patch patch (obsolete) — Splinter Review
This applies on top of the patch for bug 766403.
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #639411 - Flags: review?(adw)
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. :)
Attachment #639411 - Flags: review?(adw) → review+
(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.
(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.
Made the test actually (inefficiently) check that the test providers are all there.
Attachment #639411 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/e548482bc255
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
Flags: in-testsuite+
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/e548482bc255
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: