762579 - Implement "SocialAPI service" (provider registry)

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

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

At a high level, this component should be similar to the nsIBrowserSearchService. It will be a registry of "social providers", available to be used as part of the social functionality we'll be implementing.

See the following links for details of the current implementation in the social add-on:

https://github.com/mozilla/socialapi-dev/blob/develop/modules/registry.js
https://wiki.mozilla.org/Firefox_Social_Integration_Design_Spec#Social_Service_Registry

In a meeting we discussed moving away from SQLite in favor of either simple prefs, or a JSON file serialized to disk. To start, we'll just want the ability to have "built-in" providers, but we'll eventually be extending this to support installation of additional providers, so we should keep that in mind.

This might also involve incorporating parts of the existing "Social Service Provider" concept:
https://wiki.mozilla.org/Firefox_Social_Integration_Design_Spec#Social_Service_Provider
https://github.com/mozilla/socialapi-dev/blob/develop/modules/provider.js
This seems similar to e.g. nsISearchEngine, though the currently implementation has it tied somewhat closely to behaviors and UI, which might need to be re-worked.

Comment 1

[Shane Caraveo (:mixedpuppy)]

File name changes are happening, the files pointed to in this bug have moved to:

registry.js -> ProviderRegistry.jsm
provider.js -> Provider.jsm

Comment 2

[Drew Willcoxon :adw]

Attachment: first-pass patch

This is just a simple first pass that tries to make as few changes as possible to the code from https://github.com/mozilla/socialapi-dev/.  It basically copies ProviderRegistry.jsm, ManifestRegistry.jsm, and Provider.jsm into toolkit/components/social/.

One change is that it expects the manifests of built-in providers to be already in preferences instead of reading them from files and then adding them to preferences.  I've included an example.com manifest.  It doesn't make any changes to how content patches are handled, though; it still looks for them in resource:// URLs.  If the plan is to bundle content patches with the app rather than downloading them, I think we could include those in toolkit/components/social/ too and map them to resource:// URLs.  Although if the content patch for a provider is in a file, it would make more sense from a locality of reference perspective for the manifest to live alongside it.  Read it into memory when the manifest registry starts up and forget altogether about putting it in preferences.

Another change is that for now I've just commented out parts that deal with UI and modifying the provider registry.  I think keeping the manifests of providers added (or updated) by the user in preferences would be OK, too.  There would be a distinct branch for them, and the built-in manifests, however they're stored, would be considered immutable so that when a provider/manifest is fetched, the mutable user store would be checked before the immutable built-in store.

A consequence of keeping built-in manifests in preferences is that the API can be synchronous.  If we keep them in files instead, we ought to read them off the main thread, so there'd need to be async init methods on the registries, or async accessors, or something along those lines.

Comment 3

[Drew Willcoxon :adw]

Attachment: patch

This cleans up the first-pass patch.  It also makes several substantive changes, but I've still kept as much of the original code as I could.

It decouples a provider's state from its manifest.  Currently, changing the enabled status of a provider sets a bool on the provider's manifest and saves the updated manifest in the pref store.  This patch keeps manifests in the social.manifest pref branch (as before) and uses a new social.provider branch to save provider state that doesn't belong in the manifest.

It removes SocialProvider.jsm (Provider.jsm in the Github repo) and replaces it with a stub provider type that basically reflects the provider's manifest and keeps the social.provider pref updated.  Gavin and I aren't sure yet how consumers will use providers (just because we have zero experience with this system), so we'll add bits back in when we need them.  I bet we'll end up adding all of Provider.jsm back in.

It removes the {enable,disable}Provider methods on the registry.  Consumers can instead get a provider from the registry and then toggle the enabled bool on it.  That's not equivalent to what disableProvider currently does:  if the provider to disable is the current provider, it goes and finds a new enabled provider and makes it current, and if no provider is enabled, it disables the service.  Right now I don't feel too bad about it since all I'm doing is removing from the API, and currently it's possible to toggle a provider's enabled bool.  If we end up needing {enable,disable}Provider, we can add them back in.  Although I think a better implementation would be for the currentProvider getter to do the search for an enabled provider (of course only when it sees that the current provider is disabled or null), returning null only if there isn't one.  I don't think disabling the whole service logically follows from the case where all providers are disabled.

It uses origins as keys in the prefs, not hosts as is done now.  social.manifest.http://example.com, not social.manifest.example.com.  The main reason is that I didn't want to copy and paste normalizeKey() from ManifestRegistry.jsm to ProviderRegistry.jsm, where I would have needed it for the social.provider branch.  But another reason is that "origins" and hosts aren't one-to-one, and unless I'm missing something the manifest registry doesn't recognize that and is therefore vulnerable to collisions.  Also it's just easier.

Comment 4

[Drew Willcoxon :adw]

Oh, about the content patches I mentioned in comment 2:

[3:28pm] gavin: michaelhanson: maybe you can explain - I'm wondering why "content patches" are necessary
[3:28pm] michaelhanson: gavin: they're not - they're useful during the hacking phase where we don't have support from networks
[3:28pm] gavin: ah, I see
[3:28pm] michaelhanson: gavin: to bootstrap this thing I had to monkeypatch a bunch of functionality into existing pages.
[3:28pm] gavin: makes sense
[3:29pm] gavin: so we can drop them now?
[3:29pm] michaelhanson: gavin: I think so.  I suspect webhackers could use greasemonkey if they want such a thing anyway
[3:29pm] michaelhanson: gavin: I see no product reason for them to continue

Comment 5

[Mark Hammond [:markh] [:mhammond]]

It sounds like some of these changes will break the existing tests, but I can't see tests in the patch?

Comment 6

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

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

Yeah, we'll need some tests for this as well. Just basic ones would be fine for the moment, since there isn't too much functionality. The existing test at https://github.com/mozilla/socialapi-dev/blob/develop/test/browser_registry.js looks suitable for porting if installTestProvider is stubbed out to just add stuff to prefs.

>diff --git a/toolkit/components/social/SocialManifestRegistry.jsm b/toolkit/components/social/SocialManifestRegistry.jsm

>+var SocialManifestRegistry = (function() {
>+  var _prefBranch = Services.prefs.getBranch("social.manifest.").QueryInterface(Ci.nsIPrefBranch2);;

We should just use Services.prefs throughout, since it's already imported (also nsIPrefBranch2 has been obsoleted now: bug 718255)

>+  function normalizeKey(aURL) {
>+    return aURL;
>+  }

Do we plan to change this? Could just remove it for now.

>+  function put(origin, manifest, cb) {

>+      manifest.last_modified = new Date().getTime();

nit: Date.now()

>+      if (cb) cb(true);
>+    } catch(e) {
>+      if (cb) cb(false);

nit: newline after if() (applies elsewhere)

>+  function get(origin, cb) {

>+        Cu.reportError("repo.get: "+e);

We should either remove this or make the message more descriptive. I think probably just removing it is fine.

>+  function iterate(cb, finalize) {

>+    for each(let key in manifests) {
>+      this.get(key, cb);
>+    }

nit: for each iteration of arrays frowned upon, use .forEach or "for of" (https://developer.mozilla.org/en/JavaScript/Reference/Statements/for...of)

>+var EXPORTED_SYMBOLS = ["SocialManifestRegistry"];

Looks like this object's only property is ".repo". Should that be flattened onto this object directly? Would it make sense to have other properties?

>diff --git a/toolkit/components/social/SocialProviderRegistry.jsm b/toolkit/components/social/SocialProviderRegistry.jsm

>+ * Contributor(s):
>+ *  Michael Hanson <[e-mail]>
>+ *  Edward Lee <[e-mail]>
>+ *  Mark Hammond <[e-mail]>
>+ *  Shane Caraveo <[e-mail]>
>+ */

MPL2 license blocks don't list contributors. Just include their names in the commit message (applies to both files I guess).

>+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>+["Services", "SocialManifestRegistry"].forEach(function (name) {
>+  let url = "resource://gre/modules/" + name + ".jsm";
>+  XPCOMUtils.defineLazyModuleGetter(this, name, url);
>+}, this);

Can't say that I like this magic much :) Makes grepping for imports harder, and lazy import doesn't have much benefit (Services.jsm will already have been loaded anyways, and who would import this module and then not cause SocialManifestRegistry to be imported?).

>+XPCOMUtils.defineLazyGetter(this, "SocialProviderRegistry", function () {
>+  return new Registry(newProvider);
>+});

Similarly, isn't this triggered as soon as you import the module? You could just make it:

var SocialProviderRegistry = new Registry(newProvider);

For the moment, you could also just hardcode newProvider as the createCallback, and remove it as an argument to the Registry contructor.

>+function Registry(createCallback) {

>+  // Populate our in-memory provider store.
>+  SocialManifestRegistry.repo.iterate((function(key, manifest) {

Given that this is the only use of the SocialManifestRegistry module, can we just make it a singleton object in this file and not export it, rather than splitting it into its own module? It won't really have any use externally, right? Seems like we can also drop its put/remove support for the moment.

>+  set currentProvider(provider) {

>+    try {
>+      this._prefBranch.setCharPref("current", provider.origin);
>+    }
>+    catch(e) {
>+      // just during dev, otherwise we shouldn't log here
>+      Cu.reportError(e);

Seems like we can get rid of this try/catch and reportError entirely.

>+function newProvider(manifest) {
>+  let pref = Services.prefs.getBranch("social.provider." + manifest.origin);
>+  try {
>+    var state = JSON.parse(pref.getCharPref(""));
>+  }
>+  catch (err) {}
>+  if (!state || typeof(state) != "object")
>+    state = {};
>+  function store() {
>+    pref.setCharPref("", JSON.stringify(state));
>+  }

You could define |var prefName = "social.provider." + manifest.origin;| and then make these setCharPref calls just be Services.prefs.[sg]etCharPref(prefName, ...).

Otherwise this looks good, I'll do another pass once these comments have been addressed.

Comment 7

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #6)


> >+  function normalizeKey(aURL) {
> >+    return aURL;
> >+  }
> 
> Do we plan to change this? Could just remove it for now.

normalizeKey was meant to ensure that the keys added to the data storage were always "host:port".  Is there a reason why that is not included?

Comment 8

[Drew Willcoxon :adw]

Comment 3:

> It uses origins as keys in the prefs, not hosts as is done now. 
> social.manifest.http://example.com, not social.manifest.example.com.  The
> main reason is that I didn't want to copy and paste normalizeKey() from
> ManifestRegistry.jsm to ProviderRegistry.jsm, where I would have needed it
> for the social.provider branch.  But another reason is that "origins" and
> hosts aren't one-to-one, and unless I'm missing something the manifest
> registry doesn't recognize that and is therefore vulnerable to collisions. 
> Also it's just easier.

Comment 9

[Shane Caraveo (:mixedpuppy)]

(In reply to Drew Willcoxon :adw from comment #8)

Sorry, found that after the fact, was writing a response on it.

> 
> > It uses origins as keys in the prefs, not hosts as is done now. 
> > social.manifest.http://example.com, not social.manifest.example.com.  The
> > main reason is that I didn't want to copy and paste normalizeKey() from
> > ManifestRegistry.jsm to ProviderRegistry.jsm, where I would have needed it
> > for the social.provider branch.  But another reason is that "origins" and
> > hosts aren't one-to-one, and unless I'm missing something the manifest
> > registry doesn't recognize that and is therefore vulnerable to collisions. 
> > Also it's just easier.

While origin may be fine for large services, I'm not certain it handles long tail services well.  Is foobar.wordpress.com the same services as xyz.wordpress.com?  What if those two wordpress sites have different socialapi plugins for wordpress?  Even for large services, what if they have different socialapi plugins for different services? e.g. one for gmail, one for google plus.

Comment 10

[Drew Willcoxon :adw]

I'm not sure I follow... normalizeKey("http://foo.bar.example.com") returns "foo.bar.example.com".  In my patch, we'd use "http://foo.bar.example.com".  The only difference is my patch keeps the scheme.  The potential collisions I'm referring to are collisions due to different schemes but same hosts.  e.g., normalizeKey("http://example.com") and normalizeKey("https://example.com") (note the "https") both return "example.com".  But like I said, I might be missing something.

Comment 11

[Drew Willcoxon :adw]

Attachment: patch

OK, this strips the service to the bone like we talked about today.

Comment 12

[Shane Caraveo (:mixedpuppy)]

(In reply to Drew Willcoxon :adw from comment #10)
> I'm not sure I follow... normalizeKey("http://foo.bar.example.com") returns
> "foo.bar.example.com".  In my patch, we'd use "http://foo.bar.example.com". 
> The only difference is my patch keeps the scheme.  The potential collisions
> I'm referring to are collisions due to different schemes but same hosts. 
> e.g., normalizeKey("http://example.com") and
> normalizeKey("https://example.com") (note the "https") both return
> "example.com".  But like I said, I might be missing something.

ok, I think I misread what you were saying before, so ignore my comment.

Comment 13

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

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

>diff --git a/toolkit/components/social/SocialService.jsm b/toolkit/components/social/SocialService.jsm

>+const SocialService = {

>+  _init: function _init() {

>+    this._providers = origins.reduce(function (memo, origin) {

Why .reduce instead of forEach? Code seems harder to follow this way.

>+  getProvider: function getProvider(origin) {

I know we went back and forth on the API, but I still wonder whether we'll regret not making the exposed interface be fully async from the get-go. How about we make this async now (have it take a callback, etc.) with a simple executeSoon implementation for now, just to be safe, and then re-evaluate once we've written some more code that actually makes use of this API?

Comment 14

[Drew Willcoxon :adw]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #13)
> Why .reduce instead of forEach? Code seems harder to follow this way.

Functional programming!  It's satisfying. :)

> >+  getProvider: function getProvider(origin) {
> 
> I know we went back and forth on the API, but I still wonder whether we'll
> regret not making the exposed interface be fully async from the get-go. How
> about we make this async now (have it take a callback, etc.) with a simple
> executeSoon implementation for now, just to be safe, and then re-evaluate
> once we've written some more code that actually makes use of this API?

OK, I'll make this change since you think we should, but we could do the reverse, too:  keep it sync now and then reevaluate once we've written some code.  (But I don't think writing consumers will help us know if this API should be async or not.  It should be a conscious design decision.  Do we think we'll ever want to allow some providers/manifests not to be in memory at all times?  Are there reasons for keeping some out of memory?  (That's not to say we couldn't change the decision later.))

I'll land it tomorrow.

Comment 15

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

I think "fully async API" is the best default for something that could conceivably depend on I/O in the future (once we add the ability to have custom/installed providers). Storing stuff in prefs doesn't seem like it will scale very well (and some day in the distant future we'll probably want an async prefs API, too). I think we need a very good reason to deviate from that default, and I suspect "async API slightly more annoying to use" isn't sufficient - but the only way to confirm that suspicion is to write code that uses this API.

Comment 16

[Drew Willcoxon :adw]

Attachment: patch

This makes getProvider async and uses AsyncRunner from my async content pref service patch (bug 699859) to make writing tests easier.  It also renames test_basic.js to test_getProvider.js.

When I finish rebuilding locally I'll push this to try to make sure there are no surprises, and then I'll land it.

Comment 17

[Drew Willcoxon :adw]

https://tbpl.mozilla.org/?tree=Try&rev=4ef8939ce0ae
https://hg.mozilla.org/integration/mozilla-inbound/rev/34be3078ae5e

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/34be3078ae5e