755126 - management UI via about:addons

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Comment 1

[Shane Caraveo (:mixedpuppy)]

moved from https://github.com/mozilla/socialapi-dev/issues/9

We have a working pref page, it needs ux/design

Comment 2

[Joshua Smith [:joshua-s]]

do you have all the functions working?

Comment 3

[Shane Caraveo (:mixedpuppy)]

Attachment: addonprovider.patch

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.

Comment 4

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

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.

Comment 5

[Dave Townsend [:mossop]]

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.

Comment 6

[Shane Caraveo (:mixedpuppy)]

(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.

Comment 7

[Shane Caraveo (:mixedpuppy)]

(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.

Comment 8

[Shane Caraveo (:mixedpuppy)]

Attachment: addonprovider.patch

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.

Comment 9

[Shane Caraveo (:mixedpuppy)]

Attachment: addonprovider.patch

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.

Comment 11

[Shane Caraveo (:mixedpuppy)]

Attachment: addonprovider.patch

updated to latest m-c, only change is a fixed test

Comment 12

[Dave Townsend [:mossop]]

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

Comment 13

[Shane Caraveo (:mixedpuppy)]

Attachment: addonprovider.patch

implements last feedback and removes install/uninstall functionality which will become a part of the patch in bug 786133

Comment 14

[:Felipe Gomes (needinfo for replies!)]

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.

Comment 15

[Shane Caraveo (:mixedpuppy)]

Attachment: addonprovider.patch

updated with comments, I moved everything into prototype for addonwrapper.

Comment 16

[Shane Caraveo (:mixedpuppy)]

Attachment: addonprovider.patch

updated with in person comments

Comment 17

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

(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 18

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

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"?

Comment 19

[:Felipe Gomes (needinfo for replies!)]

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

Comment 20

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gavin Sharp (use [e-mail] 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 21

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 22

[Shane Caraveo (:mixedpuppy)]

Attachment: addonprovider.patch

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

Comment 23

[Shane Caraveo (:mixedpuppy)]

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.

Comment 24

[Shane Caraveo (:mixedpuppy)]

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 25

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

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.

Comment 26

[Shane Caraveo (:mixedpuppy)]

Attachment: addonprovider.patch

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)

Comment 27

[Shane Caraveo (:mixedpuppy)]

Attachment: toolbar menu change.png

last item in menu has changed to "Manage Services..." which opens about:addons on the services tab.

Comment 28

[Shane Caraveo (:mixedpuppy)]

Attachment: config via about:addons.png

about:addons view.  The icons for my demo providers are small or missing, but the facebook icon shows properly.

Comment 29

[Shane Caraveo (:mixedpuppy)]

try build

https://tbpl.mozilla.org/?tree=Try&rev=f78adf79ace3

Comment 30

[Shane Caraveo (:mixedpuppy)]

Attachment: addonprovider.patch

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.

Comment 31

[Shane Caraveo (:mixedpuppy)]

Attachment: addonprovider.patch

undo an inadvertent change in the test file.  gavin, the change in question is specifically in browser_select_selection.js

Comment 32

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 33

[Shane Caraveo (:mixedpuppy)]

Attachment: addonprovider.patch

asking for r+ on changes suggested by Blair in comment 32, first come first serve :)

Comment 34

[Shane Caraveo (:mixedpuppy)]

new try at https://tbpl.mozilla.org/?tree=Try&rev=db53cf8ffa2e

Comment 35

[Shane Caraveo (:mixedpuppy)]

Attachment: addonprovider.patch

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.

Comment 36

[:Felipe Gomes (needinfo for replies!)]

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.

Comment 37

[Shane Caraveo (:mixedpuppy)]

Attachment: detail view in about:addons.png

Comment 38

[Shane Caraveo (:mixedpuppy)]

Attachment: config via about:addons.png

Comment 39

[Shane Caraveo (:mixedpuppy)]

a quick verification that fixed tests also pass on try

https://tbpl.mozilla.org/?tree=Try&rev=85761bc7774d

Comment 40

[Shane Caraveo (:mixedpuppy)]

fixed try at https://tbpl.mozilla.org/?tree=Try&rev=dcd27edf41d2

Comment 41

[Shane Caraveo (:mixedpuppy)]

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

yay!

Comment 42

[Gregory Szorc [:gps]]

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

Comment 43

[Ioana (away)]

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?

Comment 44

[Alex Keybl [:akeybl]]

This will be release noted as part of the FF22 release notes as "Social services management implemented in Add-ons Manager"