595848 - Support registering custom add-on types that appear in the UI as a regular list

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Anthony Lieuallen]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.2.8) Gecko/20100722 Firefox/3.6.8
Build Identifier: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:2.0b5) Gecko/20100101 Firefox/4.0b5

Greasemonkey is trying its hardest to integrate properly with the new Add-ons Manager API in Firefox 4.  To date, we've followed the SlipperyMonkey (http://www.oxymoronical.com/blog/2010/07/How-to-extend-the-new-Add-ons-Manager) example.

So we've been hacking the DOM to add in a "menu item" (what's the proper name here?) for User Scripts.  This works fine, but if that is the selected item and the manager is closed and reopened, it gives:

Error: this.node.selectedItem is null
Source File: chrome://mozapps/content/extensions/extensions.js
Line: 1046
Error: Invalid view: list
Source File: chrome://mozapps/content/extensions/extensions.js
Line: 354

Because at the point in time that bit of script runs, it's true: that part of the DOM doesn't exist because our script hasn't yet run to add it.  It appears we need a more reliable way to register an "add-on type" such that it consistently works in the manager's menu.

Closing and reopening the manager a second time without any other action will bring up the "Extensions" type.

Reproducible: Always

Steps to Reproduce:
1. Install a Greasemonkey nightly (e.g. https://arantius.com/misc/gm-nightly/greasemonkey-2010.09.10.beta.xpi)
2. Open the Add-ons manager
3. Select the "User Scripts" tab
4. Close and re-open the Add-ons manager
Actual Results:  
Errors thrown to the console, add-ons manager is permanently broken (any selected type produces empty results pane) until the window is closed and reopened (or reloaded e.g. F5) without a custom type selected.

Expected Results:  
Add-ons manager opens to the previously selected add-on type, even if it is not a stock type.

Comment 1

[Dave Townsend [:mossop]]

The problem here is that XUL is persisting the selected category but when the XUL is loaded it is no longer there I think. We might have to switch to a different mechanism for persistence for this to work.

Comment 2

[Anthony Lieuallen]

I had known/assumed that clearly when I discovered the bug, but for some reason I was stuck in pre #565610 land.  Pointing that out made me realize we're not, so there's an obvious, though not perfect, work around: use an XUL overlay to define this DOM manipulation, and persistence works.

It also appears that in b5 the heading that used to need the extended string bundle is gone, so the string bundle is never referenced, and doesn't need to be extended.

This magical XUL overlay isn't perfect, but is probably "Good Enough".  I'd prefer to "unregister the addon type" than to write a bunch of JS to "un-apply" the overlay, in the case that we'd want Greasemonkey to become restartless.

Comment 3

[Dave Townsend [:mossop]]

Much as I want to see this done we aren't going to block the release on it.

Comment 4

[Dave Townsend [:mossop]]

Attachment: WIP 1

This is a first hash of allowing providers to define their own custom types to be included in the add-ons manager UI as a list view. The APi is fairly simple, when providers register themselves they have to pass a list of new types they are defining. Any conflicting type IDs will be ignored. The type object includes some basic info about the types including a priority and flags for ordering and controlling the behaviour in the UI.

This breaks persistence right now since the richlistbox constructor runs before the full set of categories is in the DOM so for now I've dropped down to persisting in a pref *sigh*. It might be possible to solve this but then there are other bugs which might make XUL persistence difficult to use anyway (bug 639928).

Comment 5

[Dave Townsend [:mossop]]

Should mention, this is rough. The name property for the types should be replaced with a smart getter that retrieves the localised name. There are other things that it might make sense to add in the future (default sort order) but this is enough to work for now.

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 519264 [details] [diff] [review]
WIP 1

Warning: This is a bunch of unordered notes/thoughts. Some might be better suited for followup bugs.


(In reply to comment #4)
> This breaks persistence right now since the richlistbox constructor runs before
> the full set of categories is in the DOM so for now I've dropped down to
> persisting in a pref *sigh*. It might be possible to solve this but then there
> are other bugs which might make XUL persistence difficult to use anyway (bug
> 639928).

I'd rather just get rid of using XUL persistance for this, rather than try to fix it/work around it for every edge case. Even without the edge cases, we want too much control over it for XUL persistance to fit right.

Since providers will be required to tell advertise what types they support, APIs like getAddonsByTypes() can be optimized to not query providers that don't support the type we're looking for. Should also be able to return early if a type is passed in that no provider supports. Of course, that means AddonManagerInternal needs to keep track of which providers support which types.


+  TYPE_HIDE_EMPTY: 1,

I think UI-related flags (which are really hints) should indicate that in the name, so they're noticeably different from the flags that affect how types are treated by the API. (I'm asuming there will be flags that aren't just for the UI.)

+  get addonTypes() {
+    return AddonManagerInternal.addonTypes;
+  },

Since these are objects, you'll probably want to either copy them or seal them. Weird things would happen if these are ever modified outside AddonManagerInternal without the UI being notified of the changes.


Need to figure out a way to replace the hidden attribute being persisted, so bug 577990 isn't regressed.

+    keys.sort(function(a, b) {
+      return types[a].priority - types[b].priority;
+    });

Do you imagine priority being used for anything other than ordering in the UI? If not, would rather have it named something like uiPriority.


+        this._maybeHidden.push("addons://list/" + type.id);

The type will also need to define what view should be used - which will be needed once we get the new appearance pane.

It would be nice to have the theme and lightweight-theme types separate, since there's no reliable way to tell them apart at the moment (see bug 629474). Given that, it'd be nice for the type definition to say either "don't make a separate list for me", or "merge me in with type XXX"

Since providers can be registered on the fly, we'll need an event when a new type is registered and unregistered, so the UI can pick up the changes. I'd also be nice for other reasons to have an event listener specifically for system events (eg, notifying when AddonManager.autoUpdateDefault changes).

+      category.setAttribute("id", "category-" + type.id);

So the UI doesn't break on weird type IDs, it'd be nice to have AddonManagerInternal be strict about what type IDs it will accept (and throw if a type ID doesn't match /^[a-z-_]+$/).

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Hm, not sure if this solves the issue of adding views that expose something that isn't a specific addon type. For example, the Get Add-ons pane. Could be done via registering a dummy type, but that seems kinda hackish. Anyway, followup fodder.

Comment 8

[Brian King [:kinger]]

In the Collector extension, we don't get any errors and the Collections panel persists between sessions, but we are probably doing something tricky to work around it.

https://addons.mozilla.org/en-US/firefox/addon/add-on-collector/

Code:
http://viewvc.svn.mozilla.org/vc/addons/trunk/bandwagon/

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #8)
> In the Collector extension, we don't get any errors and the Collections panel
> persists between sessions, but we are probably doing something tricky to work
> around it.

The issue described in comment 0 only occurs if you're not using an overlay, so wouldn't happen with the Addon Collector. 

Was more interested in your opinion of the new API in the attachment (see comment 4) - would you be able to use that new API in the Addons Collector, without touching the categories list in the DOM, and without touching gCategories in JS? Or do we need another API that adds a category item, without adding a new addon type? (I'd like to have it so no one ever has to touch the categories list directly)

Comment 10

[Dave Townsend [:mossop]]

This patch mainly focuses on allowing the simple case of adding a type that displays as a regular list view which collections isn't really. We could maybe extend it to allow also specifying a custom chrome url to load in the view frame for that type but I think that's follow-up fodder for now.

Comment 11

[Brian King [:kinger]]

(In reply to comment #9)
> Was more interested in your opinion of the new API in the attachment (see
> comment 4) - would you be able to use that new API in the Addons Collector,
> without touching the categories list in the DOM, and without touching
> gCategories in JS?

This is certainly something we can potentially use.

> Or do we need another API that adds a category item, without
> adding a new addon type? (I'd like to have it so no one ever has to touch the
> categories list directly)

I'm not sure what you mean here .. what are the advantages of not having types?

Also what is not clear from the patch is if it handles the action for each item, i.e. what content is loaded when selected? Or is that not in scope here, and handled elsewhere?

Comment 12

[Erik Vold]

(In reply to comment #10)
> This patch mainly focuses on allowing the simple case of adding a type that
> displays as a regular list view which collections isn't really. We could maybe
> extend it to allow also specifying a custom chrome url to load in the view
> frame for that type but I think that's follow-up fodder for now.

So it looks to me like one could do this:

AddonManager.registerProvider({}, {id: "magic-addon-type", name: "Magic Add-ons", priority: 0}); 


and a new category would be added, and a add-on dev can hack their way to adding a pane from there atm. Which seems fine to me, and it does seem like this could be extended to allow different types of categories.

I wonder why not create a new AddonManager method called say, AddonManager.registerCategory? since a add-on dev may want to add a provider but not a category, or vice versa.

Also priority should have a default value of 0 imo.

Comment 13

[Erik Vold]

* AddonManagerPrivate

Comment 14

[Dave Townsend [:mossop]]

So there are really two different problems that people want solved I think:

1. We want to be able to enumerate the different types of add-ons that could be available in the app (this is outside of the UI)
2. We want the UI to be configurable by add-ons
2a. By displaying custom add-on types in the regular list view
2b. By displaying custom views provided by an add-on (like the discovery pane or the new appearance view f.e.)

The existing patch takes care of (1) and (2a) and I think is probably the right API to do so. I think I agree though we need an additional part to take care of (2b). As Eric says this will probably be just a straightforward way to register a new custom view, not tied to any particular type or provider and then probably a flag on types that tells the UI to not construct a list view for them.

My thinking is that rather than being an API on AddonManager or AddonManagerPrivate it'd probably be an API in the extensions UI itself. When the UI opens a notification would go out saying so and giving consumers a chance to register themselves and then we'd create the category selector and call the category object whenever it was viewed. We'd just leave it up to the category provider to add whatever UI it wanted in its own way.

Does this make sense or am I missing something?

Comment 15

[Erik Vold]

(In reply to comment #14)
> My thinking is that rather than being an API on AddonManager or
> AddonManagerPrivate it'd probably be an API in the extensions UI itself.

That makes good sense.

> When the UI opens a notification would go out saying so and giving consumers a
> chance to register themselves and then we'd create the category selector and
> call the category object whenever it was viewed. We'd just leave it up to the
> category provider to add whatever UI it wanted in its own way.

That sounds good to me.

Comment 16

[Brian King [:kinger]]

+1 for makes sense / sounds good.

Comment 17

[Dave Townsend [:mossop]]

Since this patch is nearly done I'm going to spin off the other part into bug 647265

Comment 18

[Anthony Lieuallen]

I took a shallow look at this patch and I think it probably handles Greasemonkey's needs (but it's not clear if we'll use it, as we have to support 4.0 w/out it anyway).  

But I wonder: this may be a separate bug's worth but I feel it's related here, too: this doesn't appear to hook into the "available add-ons" search functionality at all.  It might be nice to offer the ability to search over e.g. userscripts.org, just like you can now search a.m.o.

Comment 19

[Dave Townsend [:mossop]]

(In reply to comment #18)
> I took a shallow look at this patch and I think it probably handles
> Greasemonkey's needs (but it's not clear if we'll use it, as we have to support
> 4.0 w/out it anyway).  
> 
> But I wonder: this may be a separate bug's worth but I feel it's related here,
> too: this doesn't appear to hook into the "available add-ons" search
> functionality at all.  It might be nice to offer the ability to search over
> e.g. userscripts.org, just like you can now search a.m.o.

That is something that would have to be handled separately

Comment 20

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Ok so this implements the custom type listing parts. It is a little complicated due to having to handle session history when types are removed when the manager is open:

If a custom type is being displayed when it is removed then the manager switched to the default view.
If the user tries to go back/forward to a type that no longer exists then we keep going back/forward till a valid category is found.
If no valid category is found and the start/end of history is reached then the default view is shown.

To save a little bit of tinkering with the custom types array I used a proxy around the array. I haven't put any protection in place around the type objects themselves, do you think this is necessary?

By default any registered custom types do not show up in the manager unless they have a flag saying how to make them appear, so far we only support one way of showing, the list view.

Comment 21

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 529749 [details] [diff] [review]
patch rev 1

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

Missing changes to winstripe and gnomestripe.

::: toolkit/mozapps/extensions/AddonManager.jsm
@@ +45,5 @@
>  const PREF_EM_UPDATE_ENABLED   = "extensions.update.enabled";
>  const PREF_EM_LAST_APP_VERSION = "extensions.lastAppVersion";
>  const PREF_EM_AUTOUPDATE_DEFAULT = "extensions.update.autoUpdateDefault";
>  
> +const VALID_TYPES = /^[\w\-]+$/;

VALID_TYPES_REGEXP (since its not the actual types).

@@ +225,5 @@
>    providers: [],
> +  types: {},
> +
> +  // A read-only wrapper around the types dictionary
> +  typesProxy: Proxy.create({

Congrats on the first use of Proxy in the tree! 

Giving out a read-only view of an object seems like a useful/generic thing to do, so I wonder if this should eventually be split out to a JSM (or functionality added directly to Proxy).

@@ +231,5 @@
> +      if (!(aName in AddonManagerInternal.types))
> +        return undefined;
> +
> +      return {
> +        value: AddonManagerInternal.types[aName].type,

I'm 50/50 as to whether this needs wrapped too. One one hand, it might be nice to be able to modify a type. On the other hand, doing so might break something.

@@ +257,5 @@
> +    },
> +
> +    defineProperty: function(aName, aProperty) {
> +      // Ignore attempts to define properties
> +      return undefined;

No need to explicitly return undefined, since the return value is ignored here. (Keep it for fix() though, since its an expected return value there.)

@@ +265,5 @@
> +      return undefined;
> +    },
> +
> +    // Despite MDC's claims to the contrary, it is required that this trap
> +    // be defined

Er, lets just correct MDC... (or file a bug - seems like it should work without it implemented)

@@ +335,5 @@
>     *
>     * @param  aProvider
>     *         The provider to register
> +   * @param  aTypes
> +   *         The types the provider supports

Make this more concise? "Array of type descriptors" or something.

@@ +351,5 @@
> +
> +          this.types[aType.id] = {
> +            type: aType,
> +            providers: [aProvider]
> +          }

Nit: Missing semicolon.

@@ +1139,5 @@
>    SCOPE_ALL: 15,
>  
> +  // 1-15 are different built-in views for the add-on type
> +  TYPE_UI_VIEW_MASK: 15,
> +  TYPE_UI_VIEW_LIST: 1,

"14 views types ought to be enough for anyone."

Having these be numeric flags makes it difficult for addons to add and use new views (makes it very easy to have collisions). I'd much rather have the view ID be a separate property, with the value being a string (TYPE_UI_VIEW_LIST can still be kept as a constant). That would also mean there could be a 1:1 mapping between the view ID defined for the type, and the view ID that gViewController uses (and the addons://VIEW/ URIs).

::: toolkit/mozapps/extensions/LightweightThemeManager.jsm
@@ +809,5 @@
> +  delete aType.name;
> +  let bundle = Services.strings.createBundle(URI_EXTENSION_STRINGS);
> +  aType.name = bundle.GetStringFromName("type." + aType.id + ".name");
> +  return aType.name;
> +}

I wonder if this would be better put in a JSM that every provider can use, instead of duplicating code each time. AddonManager.jsm would be a good a place as any - perhaps in a AddonManagerUtils object?

::: toolkit/mozapps/extensions/content/extensions.js
@@ +550,5 @@
> +    try {
> +      this.loadViewInternal(state.view, state.previousView, state);
> +      this.lastHistoryIndex = gHistory.index;
> +    }
> +    catch (e) {

Interesting side-effect: If the view's show() method throws, you'll go back in history twice. Not sure if that's right or not.

@@ +1444,5 @@
>    initialize: function() {
>      this.node = document.getElementById("categories");
>      this._search = this.get("addons://search/");
>  
> +    var updates = document.getElementById("category-availableUpdates");

This doesn't appear to be used.

@@ +1537,5 @@
> +  onTypeAdded: function(aType) {
> +    var viewType = aType.flags & AddonManager.TYPE_UI_VIEW_MASK;
> +    // Only list views are known at the moment
> +    if (viewType != AddonManager.TYPE_UI_VIEW_LIST)
> +      return;

Is the type has a view property which is a string, you'd only need to check if its one of the keys in gViewController.viewObjects, to automatically support custom views.

@@ +1563,4 @@
>            gViewController.loadView(VIEW_DEFAULT);
>  
>          item.hidden = hidden;
> +        Services.prefs.setBoolPref("extensions.ui." + aType.id + ".hidden", hidden);

Put this pref name in a const.

::: toolkit/mozapps/extensions/content/extensions.xul
@@ +219,3 @@
>        <richlistitem id="category-availableUpdates" value="addons://updates/available"
>                      class="category"
> +                    name="&view.availableUpdates.label;" priority="7000"

Think this priority (and for recent-updates) should be much higher (as in, add a couple of zeros), to indicate that these should usually stay at the bottom, below any custom categories.

::: toolkit/mozapps/extensions/test/xpcshell/test_types.js
@@ +11,5 @@
> +
> +  do_check_false("test" in AddonManager.addonTypes);
> +  let types = AddonManager.addonTypes;
> +
> +  // The dumbest provider possible

Heh...

Comment 22

[Dave Townsend [:mossop]]

(In reply to comment #21)
> @@ +265,5 @@
> > +      return undefined;
> > +    },
> > +
> > +    // Despite MDC's claims to the contrary, it is required that this trap
> > +    // be defined
> 
> Er, lets just correct MDC... (or file a bug - seems like it should work
> without it implemented)

I spoke to the JS guys about it but they didn't have a good answer on whether spidermonkey or MDC was wrong, apparently the spec is in flux right now.

> ::: toolkit/mozapps/extensions/LightweightThemeManager.jsm
> @@ +809,5 @@
> > +  delete aType.name;
> > +  let bundle = Services.strings.createBundle(URI_EXTENSION_STRINGS);
> > +  aType.name = bundle.GetStringFromName("type." + aType.id + ".name");
> > +  return aType.name;
> > +}
> 
> I wonder if this would be better put in a JSM that every provider can use,
> instead of duplicating code each time. AddonManager.jsm would be a good a
> place as any - perhaps in a AddonManagerUtils object?

I'm not entirely sure that it makes much sense, unless we force a particular style of string name on the other types. Maybe I'm not seeing what you're thinking of?

> ::: toolkit/mozapps/extensions/content/extensions.js
> @@ +550,5 @@
> > +    try {
> > +      this.loadViewInternal(state.view, state.previousView, state);
> > +      this.lastHistoryIndex = gHistory.index;
> > +    }
> > +    catch (e) {
> 
> Interesting side-effect: If the view's show() method throws, you'll go back
> in history twice. Not sure if that's right or not.

The alternative is to add a new method to view objects to be able to ask whether a view really exists or not. I can do that if you like, but if the show() method does throw then chances are the UI is broken anyway so maybe skipping it makes sense anyway?

Updated patch coming once it's been through the tryserver.

Comment 23

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Comment 24

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #22)
> > Interesting side-effect: If the view's show() method throws, you'll go back
> > in history twice. Not sure if that's right or not.
> 
> The alternative is to add a new method to view objects to be able to ask
> whether a view really exists or not. I can do that if you like, but if the
> show() method does throw then chances are the UI is broken anyway so maybe
> skipping it makes sense anyway?

Yea, lets keep it this way (throwing causes it to go back twice).

Comment 25

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #22)
> > I wonder if this would be better put in a JSM that every provider can use,
> > instead of duplicating code each time. AddonManager.jsm would be a good a
> > place as any - perhaps in a AddonManagerUtils object?
> 
> I'm not entirely sure that it makes much sense, unless we force a particular
> style of string name on the other types. Maybe I'm not seeing what you're
> thinking of?

Sorry, I meant every *built-in* provider - since all their strings use the same format and are in the same bundle.

Comment 26

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 532057 [details] [diff] [review]
patch rev 2

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

Looks good - just needs comment 25 seen to. Also, this is still missing winstripe and gnomestripe changes.

Comment 27

[Dave Townsend [:mossop]]

Attachment: patch rev 3

Comment 28

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 533461 [details] [diff] [review]
patch rev 3

Looks good.

As discussed on IRC, the 3 instances of getLocalisedName() should be removed before landing.

Comment 29

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/7d17e8e3c617

Comment 30

[Dave Townsend [:mossop]]

Updated the API docs on MDN with this and blogged a little intro to the changes and how they might affect existing add-ons: http://www.oxymoronical.com/blog/2011/05/Creating-custom-add-on-types-just-got-easier

Comment 31

[Jason Barnabe (np)]

How do you define the icon to use in the left side of the add-ons manager with this new interface?

It would be useful if https://developer.mozilla.org/en/Addons/Add-on_Manager/AddonType had info on the uiPriority for the built-in entries.

Comment 32

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

Asa: not sure why you moved the flags here, this wasn't fixed in Firefox 5 AFAICT.

Comment 33

[Dave Townsend [:mossop]]

(In reply to comment #32)
> Asa: not sure why you moved the flags here, this wasn't fixed in Firefox 5
> AFAICT.

This is correct

Comment 34

[LegNeato]

Clearing because there are no known regressions or fallout.

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Marking as verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0) Gecko/20100101 Firefox/6.0 and tests with GreaseMonkey.