766403 - Create a provider class for the social service

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Drew Willcoxon :adw]

The toolkit social service bug, bug 762579, uses simple provider objects that just wrap manifests.  This bug is for creating a real, minimal provider class.  It should only be as complex as the planned first-pass "share this" UI button (bug 765874) requires.  Gavin thinks that means it will basically encapsulate a frame worker object through which the UI and providers will communicate, but we're not entirely sure yet.

Current implementation in the social add-on:
https://github.com/mozilla/socialapi-dev/blob/develop/modules/Provider.jsm

Comment 1

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

See my comment at bug 762569 comment 18 - we're going to additionally need to land the workerAPI module for a functional baseline, I think.

Comment 2

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

Attachment: patch

Will comment further when I get in to work, but wanted to attach this now.

Comment 3

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

Attachment: patch

Oops, attached the wrong patch. Here's the right one.

List of changes here:
- Add MoTown as an example provider
- Add "SocialProvider" module, representing a provider object with these methods/properties:
  - name: provider name
  - workerURL: string of the URL pointing to the provider's worker script
  - origin: string of the URL representing the provider's origin
  - shutdown: method called to shut down the provider's FrameWorker completely
  - getWorkerPort: method returning a reference to a port from the provider's FrameWorker, which can be used to send messages

Comment 4

[Shane Caraveo (:mixedpuppy)]

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

Am I correct in assuming the other provider values (e.g. sidebarURL) will land with those pieces?

The workerURL had not been a required value in the past, which was useful for mockups, demos and some testing, is it necessary to require it now?

Comment 5

[Drew Willcoxon :adw]

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

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

::: browser/app/profile/firefox.js
@@ +1183,5 @@
>  // (This is intentionally on the high side; see bug 746055.)
>  pref("image.mem.max_decoded_image_kb", 256000);
> +
> +// Example social provider
> +pref("social.manifest.motown", "{\"origin\":\"https://motown-dev.mozillalabs.com\",\"name\":\"MoTown\",\"workerURL\":\"https://motown-dev.mozillalabs.com/social/worker.js\"}");

Any reason you used social.manifest.motown instead of social.manifest.https://motown-dev.mozillalabs.com?  Right now there's no technical reason you can't do that, but SocialService (well, its one method) assumes that manifests are keyed on origin and that pref names end in origins.  What happens if there are more motown origins in the future?  Do you envision the pref names of manifests added by the user using aliases like you do here or origins?

::: toolkit/components/social/SocialProvider.jsm
@@ +21,5 @@
> + * @param {jsobj} portion of the manifest file describing this provider
> + */
> +function SocialProvider(input) {
> +  if (!input.name)
> +    throw "SocialProvider must be passed a name";

Throw Errors here so that callers get a backtrace, i.e., throw new Error("...") instead of throw "...".

@@ +29,5 @@
> +    throw "SocialProvider must be passed an origin";
> +
> +  this.name = input.name;
> +  this.workerURL = input.workerURL;
> +  this.origin = input.origin;

Nit: I'm not sure on how much of this error checking you intend to keep around, but rather than checking each property by hand you could automate it with something like:

> ["name", "workerURL", "origin"].forEach(function (prop) {
>   if (!input[prop])
>     throw new Error("SocialProvider must be passed a " + prop);
>   this[prop] = input[prop];
> }, this);

@@ +34,5 @@
> +}
> +
> +SocialProvider.prototype = {
> +  /**
> +   * shutdown

Nit: I'm not a fan of including method names at the tops of comments because they often get out of sync with the actual names and they make your eye do more work for little reason.  Plus it's not m-c style (right?) for whatever that's worth.

@@ +38,5 @@
> +   * shutdown
> +   *
> +   * Shuts down the provider's FrameWorker.
> +   */
> +  shutdown: function() {

Nit: Name these functions so something useful shows up in backtraces.

Comment 6

[Drew Willcoxon :adw]

One more thing: the shutdown method calls the terminate method on the frame worker handle.  It would be great to use the same terminology throughout the social code.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

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

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

I didn't see anything outside of what Drew had already commented on.

Comment 8

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

(In reply to Drew Willcoxon :adw from comment #5)
> Any reason you used social.manifest.motown instead of
> social.manifest.https://motown-dev.mozillalabs.com?  Right now there's no
> technical reason you can't do that, but SocialService (well, its one method)
> assumes that manifests are keyed on origin and that pref names end in
> origins.  What happens if there are more motown origins in the future?  Do
> you envision the pref names of manifests added by the user using aliases
> like you do here or origins?

I don't really see us using prefs to store user-added origins (once we expose adding providers), so I don't think this problem will exist then. For the moment, this seems simpler, since as you say SocialService doesn't really actually care what the pref names are.

I fixed the other nits (naming functions, throwing Errors, removing redundant name comments).

Comment 9

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

Attachment: patch with comments addressed

Also renamed shutdown()->terminate()

Comment 10

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/c265b644a72c

Comment 11

[Drew Willcoxon :adw]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #8)
> I don't really see us using prefs to store user-added origins (once we
> expose adding providers), so I don't think this problem will exist then.

That might be true, but the "keys" are still exposed through the API (through getProvider at least), and there's only going to be one SocialService for both default and user-added providers, so assuming user-added providers are keyed on origins, this arrangement means that for some providers you'll pass an alias to the API and for others you'll pass an origin.  Seems like unnecessary complexity that might cause trouble down the road.

Just wanted to register that opinion. :)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c265b644a72c

Comment 13

[Drew Willcoxon :adw]

I'm dumb, the keys of the providers object come from manifest origins, not pref names.  Nevermind. :\

Comment 14

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

(In reply to Shane Caraveo (:mixedpuppy) from comment #4)
> Am I correct in assuming the other provider values (e.g. sidebarURL) will
> land with those pieces?

Yep, that's the idea.

> The workerURL had not been a required value in the past, which was useful
> for mockups, demos and some testing, is it necessary to require it now?

Fixed this in bug 771344.