Create a provider class for the social service

RESOLVED FIXED in Firefox 16

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: adw, Assigned: Gavin)

Tracking

Trunk
Firefox 16
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Fx16])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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
(Reporter)

Updated

5 years ago
Depends on: 762569
Blocks: 765874
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.
Assignee: adw → gavin.sharp
Whiteboard: [ms1]

Updated

5 years ago
Whiteboard: [ms1] → [Fx16]
Created attachment 639376 [details] [diff] [review]
patch

Will comment further when I get in to work, but wanted to attach this now.
Attachment #639376 - Flags: review?(jaws)
Attachment #639376 - Flags: review?(adw)
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
Attachment #639376 - Attachment is obsolete: true
Attachment #639376 - Flags: review?(jaws)
Attachment #639376 - Flags: review?(adw)
Attachment #639410 - Flags: review?(jaws)
Attachment #639410 - Flags: review?(adw)
Attachment #639410 - Flags: feedback?(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?
(Reporter)

Comment 5

5 years ago
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.
Attachment #639410 - Flags: review?(adw) → review+
(Reporter)

Comment 6

5 years ago
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 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.
Attachment #639410 - Flags: review?(jaws) → review+
(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).
Created attachment 639461 [details] [diff] [review]
patch with comments addressed

Also renamed shutdown()->terminate()
Attachment #639410 - Attachment is obsolete: true
Attachment #639410 - Flags: feedback?(mixedpuppy)
https://hg.mozilla.org/integration/mozilla-inbound/rev/c265b644a72c
Flags: in-testsuite+
Target Milestone: --- → Firefox 16
(Reporter)

Comment 11

5 years ago
(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. :)
https://hg.mozilla.org/mozilla-central/rev/c265b644a72c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Comment 13

5 years ago
I'm dumb, the keys of the providers object come from manifest origins, not pref names.  Nevermind. :\
(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.
You need to log in before you can comment on or make changes to this bug.