Closed
Bug 771247
Opened 11 years ago
Closed 11 years ago
lazify social service initialization, and add getProviderList method
Categories
(Firefox Graveyard :: SocialAPI, defect)
Firefox Graveyard
SocialAPI
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 16
People
(Reporter: Gavin, Assigned: Gavin)
Details
Attachments
(1 file, 1 obsolete file)
4.50 KB,
patch
|
Details | Diff | Splinter Review |
- Make initialization of SocialService API lazy (no side-effects to importing it) - Add getProviderList API to enumerate available providers (with very basic test)
Assignee | ||
Comment 1•11 years ago
|
||
This applies on top of the patch for bug 766403.
Comment 2•11 years ago
|
||
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+
Assignee | ||
Comment 3•11 years ago
|
||
(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•11 years ago
|
||
(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.
Assignee | ||
Comment 5•11 years ago
|
||
Made the test actually (inefficiently) check that the test providers are all there.
Attachment #639411 -
Attachment is obsolete: true
Assignee | ||
Comment 6•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e548482bc255
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
Assignee | ||
Updated•11 years ago
|
Flags: in-testsuite+
Assignee | ||
Updated•11 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 7•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e548482bc255
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•