Last Comment Bug 766403 - Create a provider class for the social service
: Create a provider class for the social service
Status: RESOLVED FIXED
[Fx16]
:
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]
:
:
Mentors:
Depends on: 762569 762579
Blocks: 765874
  Show dependency treegraph
 
Reported: 2012-06-19 17:19 PDT by Drew Willcoxon :adw
Modified: 2012-07-07 11:04 PDT (History)
9 users (show)
gavin.sharp: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (9.71 KB, patch)
2012-07-05 09:34 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review
patch (6.40 KB, patch)
2012-07-05 11:30 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
jaws: review+
adw: review+
Details | Diff | Splinter Review
patch with comments addressed (6.42 KB, patch)
2012-07-05 13:57 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review

Description Drew Willcoxon :adw 2012-06-19 17:19:37 PDT
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: gavin@gavinsharp.com] 2012-06-21 17:38:32 PDT
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: gavin@gavinsharp.com] 2012-07-05 09:34:36 PDT
Created attachment 639376 [details] [diff] [review]
patch

Will comment further when I get in to work, but wanted to attach this now.
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-05 11:30:55 PDT
Created attachment 639410 [details] [diff] [review]
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) 2012-07-05 12:32:20 PDT
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 2012-07-05 12:57:11 PDT
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 2012-07-05 13:02:17 PDT
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) 2012-07-05 13:48:39 PDT
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: gavin@gavinsharp.com] 2012-07-05 13:54:26 PDT
(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: gavin@gavinsharp.com] 2012-07-05 13:57:05 PDT
Created attachment 639461 [details] [diff] [review]
patch with comments addressed

Also renamed shutdown()->terminate()
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-05 14:04:02 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/c265b644a72c
Comment 11 Drew Willcoxon :adw 2012-07-05 14:18:43 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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] 2012-07-05 17:29:19 PDT
https://hg.mozilla.org/mozilla-central/rev/c265b644a72c
Comment 13 Drew Willcoxon :adw 2012-07-05 18:30:41 PDT
I'm dumb, the keys of the providers object come from manifest origins, not pref names.  Nevermind. :\
Comment 14 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-07 11:04:47 PDT
(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.

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