management UI via about:addons

VERIFIED FIXED in Firefox 22

Status

defect
VERIFIED FIXED
7 years ago
5 months ago

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Tracking

unspecified
Firefox 22
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 22+)

Details

(Whiteboard: [needs-ux])

Attachments

(4 attachments, 13 obsolete attachments)

54.76 KB, image/png
jboriss
: ui-review+
Details
57.38 KB, patch
Felipe
: review+
Details | Diff | Splinter Review
627.74 KB, image/png
jboriss
: ui-review+
Details
678.98 KB, image/png
jboriss
: ui-review+
Details
No description provided.
moved from https://github.com/mozilla/socialapi-dev/issues/9

We have a working pref page, it needs ux/design
do you have all the functions working?
Summary: pref page needs ux → management UI (about:social or prefs panel)
Whiteboard: [needs-ux][ms2]
Whiteboard: [needs-ux][ms2] → [needs-ux][Fx17]
Whiteboard: [needs-ux][Fx17] → [needs-ux]
OS: Mac OS X → All
Hardware: x86 → All
Posted patch addonprovider.patch (obsolete) — Splinter Review
This patch uses a technique similar to lightweight themes to add a category into the about:addons tab.  As well, it enables the use of the addons blocklist to prevent loading of a blocklisted provider.
Attachment #702541 - Flags: feedback?(jaws)
Attachment #702541 - Flags: feedback?(felipc)
Summary: management UI (about:social or prefs panel) → management UI via about:addons
Blocks: 756591
Attachment #702541 - Flags: feedback?(dtownsend+bugmail)
Comment on attachment 702541 [details] [diff] [review]
addonprovider.patch

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

At a high-level I think treating the SocialAPI similar to Lightweight Themes will work good from a usability aspect.

::: toolkit/themes/gnomestripe/mozapps/extensions/extensions.css
@@ +214,5 @@
>  #category-extension > .category-icon {
>    list-style-image: url("chrome://mozapps/skin/extensions/category-extensions.png");
>  }
> +#category-service > .category-icon {
> +  list-style-image: url("chrome://mozapps/skin/extensions/category-extensions.png");

Where does #category-service get defined? Also, we should probably get an icon made for the SocialAPI.
Attachment #702541 - Flags: feedback?(jaws) → feedback+
Comment on attachment 702541 [details] [diff] [review]
addonprovider.patch

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

Looking good, a few comments though. Some of these come from my lack of knowledge about the social service in general:

Is there a way to install/enable/disable/uninstall providers through an API or something outside of the add-ons manager UI? If so those need to send the appropriate add-on events so the UI can update if it is already open.

The add-on objects would ideally have two distinct properties, appDisabled and userDisabled. appDisabled would be true if it should be impossible to enable the provider, the only case for here would be if it is blocklisted. userDisabled would ideally maintained separate to that so if the provider changes from blocklisted to un-blocklisted it enables or stays disabled depending on what state it was in before blocklisting. Depending on what level of blocklisting support you expect to want there is also softDisabled which means the blocklist service instructed Firefox to disable it but the user can then subsequently enable it, or it would become enabled if the block went away for some reason. I don't know that the blocklist expectations are here so it's difficult to guess which circumstances we should care about supporting.

You need to patch the blocklist service so it looks at service type add-ons when notifying the user about new blocks. We've never used this for other add-on types though so I'd really want to see good tests for that to make sure we don't accidentally regress blocklisting somewhere.

::: toolkit/components/social/SocialService.jsm
@@ +259,5 @@
>      throw new Error("SocialProvider must be passed a name");
>    if (!input.origin)
>      throw new Error("SocialProvider must be passed an origin");
> +  if (getBlocklistState(input) == Ci.nsIBlocklistService.STATE_BLOCKED)
> +    throw new Error("SocialService.addProvider: provider with origin ["+input.origin+"] is blocklisted");

Nit: spaces around operators

@@ +559,5 @@
> +      }
> +    }
> +  }
> +  return null;
> +}

I'm not sure it's worth including hardcoded blocks for now.

@@ +568,5 @@
> +    return staticItem.level;
> +
> +  let bs = Cc["@mozilla.org/extensions/blocklist;1"].
> +           getService(Ci.nsIBlocklistService);
> +  return bs.getAddonBlocklistState(_getInternalID(item.id), item.version);

For the blocklist service to work correctly you need to be using the wrapper's ID here, not the internal ID.

@@ +580,5 @@
> +  }
> +
> +  let bs = Cc["@mozilla.org/extensions/blocklist;1"].
> +           getService(Ci.nsIBlocklistService);
> +  return bs.getAddonBlocklistURL(_getInternalID(item.id), item.version);

Ditto.

@@ +612,5 @@
> +  //addonChanged: function() {
> +  //
> +  //},
> +  //updateAddonAppDisabledStates: function() {
> +  //

You must implement this for blocklisting to work.

@@ +645,5 @@
> +    let id = _getInternalID(aId);
> +    if (!id) {
> +      aCallback(null);
> +      return;
> +     }

Nit: Intentation

@@ +648,5 @@
> +      return;
> +     }
> +    for (let manifest of SocialServiceInternal.manifests) {
> +      if (id == manifest.id)
> +        aCallback(new AddonWrapper(manifest));

Is there any possibility of accidentally ending up with two manifests with the same ID? Maybe return after finding one for safety, things will go strange if you call the callback twice.

@@ +723,5 @@
> +  });
> +
> +  this.__defineGetter__("permissions", function AddonWrapper_permissionsGetter() {
> +    let permissions = 0;
> +    if (Services.prefs.prefHasUserValue("social.manifest."+aManifest.id))

Nit: spaces around operators

@@ +749,5 @@
> +        ActiveProviders.flush();
> +        this._pending = AddonManager.PENDING_NONE;
> +        AddonManagerPrivate.callAddonListeners("onDisabled", this);
> +      }.bind(this));
> +    } else {

You need to test whether the provider is blocklisted before allowing it to be enabled, just checking appDisabled would normally be enough.

@@ +765,5 @@
> +  this.uninstall = function AddonWrapper_uninstall() {
> +    let prefName = "social.manifest."+aManifest.id;
> +    if (Services.prefs.prefHasUserValue(prefName)) {
> +      Services.prefs.clearUserPref(prefName);
> +      SocialService.removeProvider(aManifest.origin, function() { ActiveProviders.flush() });

You need to send out the appropriate events here too.

::: toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd
@@ +92,5 @@
>  <!ENTITY cmd.disableTheme.accesskey           "W">
> +<!ENTITY cmd.enableService.label                "Enable Service">
> +<!ENTITY cmd.enableService.accesskey            "E">
> +<!ENTITY cmd.disableSevice.label               "Disable Service">
> +<!ENTITY cmd.disableService.accesskey           "D">

Indenting problem

::: toolkit/themes/gnomestripe/mozapps/extensions/extensions.css
@@ +214,5 @@
>  #category-extension > .category-icon {
>    list-style-image: url("chrome://mozapps/skin/extensions/category-extensions.png");
>  }
> +#category-service > .category-icon {
> +  list-style-image: url("chrome://mozapps/skin/extensions/category-extensions.png");

It comes from "category-" + addon type.
Attachment #702541 - Flags: feedback?(dtownsend+bugmail) → feedback+
(In reply to Jared Wein [:jaws] from comment #4)
> Comment on attachment 702541 [details] [diff] [review]
> addonprovider.patch
> 
> Review of attachment 702541 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> At a high-level I think treating the SocialAPI similar to Lightweight Themes
> will work good from a usability aspect.
> 
> ::: toolkit/themes/gnomestripe/mozapps/extensions/extensions.css
> @@ +214,5 @@
> >  #category-extension > .category-icon {
> >    list-style-image: url("chrome://mozapps/skin/extensions/category-extensions.png");
> >  }
> > +#category-service > .category-icon {
> > +  list-style-image: url("chrome://mozapps/skin/extensions/category-extensions.png");
> 
> Where does #category-service get defined? Also, we should probably get an
> icon made for the SocialAPI.

#category-service is created/added by the view code for about:addons.  Yes, we need an icon (related bug 831442).  We also need an ability to define a larger icon for the providers, which could also be used in bug 828778.
Depends on: 828778
Depends on: 831442
(In reply to Dave Townsend (:Mossop) from comment #5)
> Comment on attachment 702541 [details] [diff] [review]
> addonprovider.patch
> 
> Review of attachment 702541 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking good, a few comments though. Some of these come from my lack of
> knowledge about the social service in general:
> 
> Is there a way to install/enable/disable/uninstall providers through an API
> or something outside of the add-ons manager UI? If so those need to send the
> appropriate add-on events so the UI can update if it is already open.

some background info: Discovery/Installation will be something similar to the discovery/installation for opensearch.  Some providers are moving ahead of us by creating addons to add their provider.  Partnered providers will be built into Firefox as Facebook is.  If a provider is builtin, uninstall will not be available.  Uninstall will only be available via about:addons.

Currently we have a way to disable providers from the social toolbar button.  However IMO that will change to a menuitem that opens about:addons to the social services group and users would enable/disable from there.

What I haven't looked into is how to deal with a provider being added via an addon.  We cannot really prevent that, so what does it mean to "uninstall" the provider from the providers group.  I think this will be an edge case that we will only run into with a couple providers, so I'm inclined to not worry about it.

> The add-on objects would ideally have two distinct properties ...

awesome info, will look into all of that.

> I'm not sure it's worth including hardcoded blocks for now.

yeah, it's only there right now for me to mock up some manual testing, it helps me to see how things react visually.
Posted patch addonprovider.patch (obsolete) — Splinter Review
updated patch with blocklist tests
- adds larger images for providers, but still need to resolve how it is handled via bug 828778
- still need to resolve images/name for feature (bug 831442)
- still has static blocklist for manual testing, will remove after resolving everything else
- still has some cruft commented out to be removed later

Further feedback on blocklist support would be great.  Everything appears to work either by manual testing or via the added tests.
Assignee: nobody → mixedpuppy
Attachment #702541 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #702541 - Flags: feedback?(felipc)
Attachment #703634 - Flags: feedback?(jaws)
Attachment #703634 - Flags: feedback?(dtownsend+bugmail)
Posted patch addonprovider.patch (obsolete) — Splinter Review
In addition to previous changes, this removes the "remove provider" menu and changes it to "Manage Services..." which opens the addon manager to the services tab.
Attachment #703634 - Attachment is obsolete: true
Attachment #703634 - Flags: feedback?(jaws)
Attachment #703634 - Flags: feedback?(dtownsend+bugmail)
Attachment #703679 - Flags: feedback?(jaws)
Attachment #703679 - Flags: feedback?(dtownsend+bugmail)
Blocks: 786133
Duplicate of this bug: 815546
Depends on: 821262
Posted patch addonprovider.patch (obsolete) — Splinter Review
updated to latest m-c, only change is a fixed test
Attachment #703679 - Attachment is obsolete: true
Attachment #703679 - Flags: feedback?(jaws)
Attachment #703679 - Flags: feedback?(dtownsend+bugmail)
Attachment #706075 - Flags: feedback?(mhammond)
Attachment #706075 - Flags: feedback?(dtownsend+bugmail)
Comment on attachment 706075 [details] [diff] [review]
addonprovider.patch

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

Looking good

::: toolkit/components/social/SocialService.jsm
@@ +768,5 @@
> +  }.bind(this));
> +
> +  this.__defineSetter__("softDisabled", function(val) {
> +    if (val == this.userDisabled)
> +      return val;

Since you're not actually tracking this separately I'd just pass it straight through to userDisabled
Attachment #706075 - Flags: feedback?(dtownsend+bugmail) → feedback+
Posted patch addonprovider.patch (obsolete) — Splinter Review
implements last feedback and removes install/uninstall functionality which will become a part of the patch in bug 786133
Attachment #706075 - Attachment is obsolete: true
Attachment #706075 - Flags: feedback?(mhammond)
Attachment #708263 - Flags: review?(felipc)
Comment on attachment 708263 [details] [diff] [review]
addonprovider.patch

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

I've done a first pass on the main files here, posting my comments as they don't need to wait before I make more progress

::: toolkit/components/social/SocialService.jsm
@@ +10,5 @@
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/AddonManager.jsm");
> +
> +const URI_EXTENSION_STRINGS  = "chrome://mozapps/locale/extensions/extensions.properties";
> +const ADDON_TYPE             = "service";

I got fooled a number of times by this const name, it seems to imply type = "addon". Can we change it to e.g. ADDON_TYPE_SERVICE?

@@ +582,5 @@
> +  }
> +}
> +
> +
> +function AddonWrapper(aManifest) {

a bunch of the simple functions can be easily moved the prototype, and don't need the .bind(this).

they are: type, screenshots, pendingOperations, operationsRequiringRestart, size, permissions, softDisabled, uninstall, cancelUninstall. (if I haven't missed any).

It's better for memory usage and also code readability IMO to have those in the prototype, so it'd be great if you could move them.
The other ones that uses the aManifest closure could also be moved but that will take more work (as you'll need to store the manifest, change some things, etc.), so those are not necessary.
Attachment #708263 - Flags: review?(felipc)
Posted patch addonprovider.patch (obsolete) — Splinter Review
updated with comments, I moved everything into prototype for addonwrapper.
Attachment #708263 - Attachment is obsolete: true
Attachment #708583 - Flags: review?(felipc)
Posted patch addonprovider.patch (obsolete) — Splinter Review
updated with in person comments
Attachment #708583 - Attachment is obsolete: true
Attachment #708583 - Flags: review?(felipc)
Attachment #708736 - Flags: review?(felipc)
Attachment #708736 - Flags: review?(felipc) → review+
(In reply to Shane Caraveo (:mixedpuppy) from comment #7)
> Currently we have a way to disable providers from the social toolbar button.
> However IMO that will change to a menuitem that opens about:addons to the
> social services group and users would enable/disable from there.
> 
> What I haven't looked into is how to deal with a provider being added via an
> addon.  We cannot really prevent that, so what does it mean to "uninstall"
> the provider from the providers group.  I think this will be an edge case
> that we will only run into with a couple providers, so I'm inclined to not
> worry about it.

We can punt on it for the purposes of getting this patch landed, but we should have the social service fire these events to handle about:addons updating itself when the backend state changes (even if there are no other ways to trigger that in our UI).
Comment on attachment 708736 [details] [diff] [review]
addonprovider.patch

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

>   get manifests() {

>+        if (manifest && typeof(manifest) == "object" && manifest.origin) {
>+          manifest.id = pref;

I don't much like this. Can't you use manifest.origin instead of "pref"?
Also writing down so we don't forget: we need to check if there's any server-side change required (before landing this) to the blocklist service due to the new type of item introduced
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #18)
> Comment on attachment 708736 [details] [diff] [review]
> addonprovider.patch
> 
> >diff --git a/toolkit/components/social/SocialService.jsm b/toolkit/components/social/SocialService.jsm
> 
> >   get manifests() {
> 
> >+        if (manifest && typeof(manifest) == "object" && manifest.origin) {
> >+          manifest.id = pref;
> 
> I don't much like this. Can't you use manifest.origin instead of "pref"?

I need it to be able to update the pref.  I could loop through the prefs to find the prefname when I need to update, but I felt this was easier.
Comment on attachment 708736 [details] [diff] [review]
addonprovider.patch

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

Driveby!

No tests for the provider APIs and events?

::: toolkit/components/social/SocialService.jsm
@@ +702,5 @@
> +
> +  get iconURL() {
> +    return this.manifest.icon32URL ? this.manifest.icon32URL : this.manifest.iconURL;
> +  },
> +  get icon64URL() {

iconURL and icon64URL are basically deprecated (but still required for backwards compat). You'll need to also implement the new 'icons' property, which should return an object with keys named as the various icon sizes. ie:

  let icons = {};
  if (this.manifest.icon32URL)
    icons[32] = this.manifest.icon32URL;
  ...

@@ +734,5 @@
> +  get userDisabled() {
> +    return !ActiveProviders.has(this.manifest.origin);
> +  },
> +
> +  set userDisabled(val) {

This needs to take into account the value of appDisabled (ie, it needs to explicitly enforce the permissions property).

::: toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd
@@ +92,5 @@
>  <!ENTITY cmd.disableTheme.accesskey           "W">
> +<!ENTITY cmd.enableService.label              "Enable Service">
> +<!ENTITY cmd.enableService.accesskey          "E">
> +<!ENTITY cmd.disableSevice.label              "Disable Service">
> +<!ENTITY cmd.disableService.accesskey         "D">

These strings don't seem to be used.
Depends on: 838883
Posted patch addonprovider.patch (obsolete) — Splinter Review
patch rebased, everyones feedback implemented, more tests added.  landing should wait on:

- discussion about a services page on AMO (similar to themes)
- discussion about the server side blocklist support
Attachment #708736 - Attachment is obsolete: true
Attachment #711642 - Flags: review?(gavin.sharp)
Depends on: 839654
I chatted with jorgev on irc, the blocklist looks good to go, pending testing.  Bug 839654 is a request to get test blocks added on the staging server.
On a related note, there is a criteria document defining our blocklist criteria at

https://developer.mozilla.org/en-US/docs/Social_API/Criteria

It is largely copied from Marketplace criteria, modified for social.
Comment on attachment 711642 [details] [diff] [review]
addonprovider.patch

Does this need additional review beyond felipe's r+? Re-request if so.

It's probably a good idea to get Boriss to ui-r+ in the bug as well.
Attachment #711642 - Flags: review?(gavin.sharp)
Posted patch addonprovider.patch (obsolete) — Splinter Review
updated to current m-c and minor fix in testing blocklist against staging server (bug 839654).  I'll carry forward felipe's r+ since the patch hasn't really changed since then (other then adding tests and minor fixes related to testing)
Attachment #711642 - Attachment is obsolete: true
Attachment #716779 - Flags: review+
last item in menu has changed to "Manage Services..." which opens about:addons on the services tab.
Attachment #716781 - Flags: ui-review?(jboriss)
Posted image config via about:addons.png (obsolete) —
about:addons view.  The icons for my demo providers are small or missing, but the facebook icon shows properly.
Attachment #716793 - Flags: ui-review?(jboriss)
Posted patch addonprovider.patch (obsolete) — Splinter Review
fixes a test failure found on try.  Basically, having our builtin social providers look like addons to the addon manager broke a different test.  I did a simple fix on it, asking r+ from gavin since it is changing a test in toolkit.
Attachment #716779 - Attachment is obsolete: true
Attachment #716877 - Flags: review?(gavin.sharp)
Posted patch addonprovider.patch (obsolete) — Splinter Review
undo an inadvertent change in the test file.  gavin, the change in question is specifically in browser_select_selection.js
Attachment #716877 - Attachment is obsolete: true
Attachment #716877 - Flags: review?(gavin.sharp)
Attachment #716884 - Flags: review?(gavin.sharp)
Comment on attachment 716884 [details] [diff] [review]
addonprovider.patch

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

::: toolkit/mozapps/extensions/test/browser/browser_select_selection.js
@@ +224,5 @@
>      // Ignore non-test add-ons that may be present
>      if (row.id.substr(-18) != "@tests.mozilla.org")
>        continue;
>  
> +    is(row._addon.type, "extension", "Should only be listing extensions");

Er, hmm, forgot about this. This is the dialog the Add-ons Manager shows when upgrading from a version of Firefox that didn't automatically disable 3rd party installs of add-ons (anything before Fx8).

The original position of that check is correct - moving means it could miss cases where it *should* fail. If we wanted to just fix the test, best way would be to special-case the new "service" add-on type.

However, I wonder if we want service providers to ever show up in that dialog... I'm thinking not. In which case, make the dialog ignore the new type here (as it does with plugins): 
https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/selectAddons.js#77
Attachment #716884 - Flags: review?(gavin.sharp) → review-
Posted patch addonprovider.patch (obsolete) — Splinter Review
asking for r+ on changes suggested by Blair in comment 32, first come first serve :)
Attachment #716884 - Attachment is obsolete: true
Attachment #717170 - Flags: review?(felipc)
Attachment #717170 - Flags: review?(bmcbride)
patch fixes socialapi xpcshell tests that failed in last try.  The addition of the blocklist support requires us to prepare a little more to properly run tests from xpcshell.
Attachment #717170 - Attachment is obsolete: true
Attachment #717170 - Flags: review?(felipc)
Attachment #717170 - Flags: review?(bmcbride)
Attachment #717366 - Flags: review?(felipc)
Attachment #717366 - Flags: review?(bmcbride)
Blocks: 845151
Comment on attachment 717366 [details] [diff] [review]
addonprovider.patch

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

not a big specialist in this but it's just test changes that all seems very sensible to me, plus the change that Blair suggested to do.
Attachment #717366 - Flags: review?(felipc)
Attachment #717366 - Flags: review?(bmcbride)
Attachment #717366 - Flags: review+
Attachment #718202 - Flags: ui-review?(jboriss)
Attachment #716793 - Attachment is obsolete: true
Attachment #716793 - Flags: ui-review?(jboriss)
Attachment #718203 - Flags: ui-review?(jboriss)
Attachment #716781 - Flags: ui-review?(jboriss) → ui-review+
Attachment #718202 - Flags: ui-review?(jboriss) → ui-review+
Attachment #718203 - Flags: ui-review?(jboriss) → ui-review+
a quick verification that fixed tests also pass on try

https://tbpl.mozilla.org/?tree=Try&rev=85761bc7774d
https://hg.mozilla.org/mozilla-central/rev/d84192e9779f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
relnote-firefox: --- → ?
Verified as fixed on the 04/03 Aurora (20130403004013), on Windows 7 64bit, Ubuntu 12.10 32bit and Mac OSX 10.7.5 64bit.

Facebook Messenger is displayed in the Services section of about:addons. Clicking the Manage Services option from toolbar->facebook leads to this page. Enabling and disabling the service from there works fine. I also did a fast smoke on the sidebar, and didn't find any unknown issues.

I did find one issue that has to do with enabling the sidebar: it doesn't get enabled when going to facebook.com/about/messenger-for-firefox and clicking the Turn On button. The button does work for older versions. Should I file a new bug, or is this by design and that facebook page will be removed?
Status: RESOLVED → VERIFIED
Depends on: 862314
This will be release noted as part of the FF22 release notes as "Social services management implemented in Add-ons Manager"
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.