Explicit extension points for front-end code

NEW
Unassigned

Status

defect
10 years ago
8 months ago

People

(Reporter: davida, Unassigned)

Tracking

({dev-doc-needed})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patchlove])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
Posted patch first cut (obsolete) — Splinter Review
Right now there's no explicit extension point for add-on authors to change what happens when the multi-message/thread summary is displayed.

I think it'd be good to provide some explicit extension points.  Attached is a first cut, which maintains a list of extensions for each of these two cases, and invokes them (in order of registration).

(Without an explicit extension point, people will just monkey-patch their way in, which feels dangerous and not so friendly).

So, usage in an add-on goes like so in an onload handler:

      gThreadSummary.registerExtension(obj);

where obj is an object which has a threadSummary() function.  I pass in the ThreadSummary object as an argument to the threadSummary() call so that the add-on author has a handle on the relevant Document, and whatever state we keep in ThreadSummary.

I had to do some refactoring of selectionsummaries.js to allow extensions to persist across summarizations.

I have a PoC add-on that uses this to provide "expanding" snippets.

Feedback on this kind of an extension model?

Note: there's no way for this extension model to do stuff before the base FolderSummary/MultiMessageSummary's summarize() functions are called.  Is that ok?

Adding jminta and myk as add-on authors to see if they have input.

(aside: i have a similar scheme in the folder summary patch (bug 492158), and indeed use the extension model to do the main feature work)
(Reporter)

Comment 1

10 years ago
Posted patch second passSplinter Review
There's nothing like having to write an add-on to figure out better ways of having an add-on API.  

The attached patch is one possible way of clarifying extension points in the (JS) front-end.  It is not _at all_ intended to supplant any of the other extension mechanisms (observers, explicit registries, etc.), but it provides what I hope is a really simple way for core code to define extension points in a simple but effective way, and it's also really easy for add-on devs to use.  Here's how it goes:

1) core code includes:

  Components.utils.import("resource://app/modules/extensionPointRegistry.js");

in whatever namespace is in play.

2) whenever the core code wants to call out to specific extension points, it does something like:

        extensionPointRegistry.callOut("onMsgStreamedInFolderSummary", [msgNode])

where the first argument is a unique name, and the second is a list of arguments.

2) add-ons register interest in specific extension points using:

  Components.utils.import("resource://app/modules/extensionPointRegistry.js");
and
  extensionPointRegistry.register("onMsgStreamedInFolderSummary", obj);

where the first argument is the (matching, duh) extension point name, and the second argument is an object who should have a function of the specified name (in this case onMsgStreamedInFolderSummary). The arguments are passed as arguments to the extension's function.

That's all.

The attached patch includes:

 - the JS module "extensionPointRegistry"
 - patches needed to the selectionsummary code to use it

I also have an example add-on which uses these extension points to add avatars to thread views.  I'll attach a screenshot.  The add-on lives at: http://hg.mozilla.org/users/dascher_mozilla.com/avatarthreads/.

Note in particular the key code is: http://hg.mozilla.org/users/dascher_mozilla.com/avatarthreads/file/c5eb3f7d44cb/content/folder.js
Assignee: nobody → david.ascher
Attachment #381438 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Reporter)

Comment 2

10 years ago
Posted image screenshot
add-on in action.
(Reporter)

Comment 3

10 years ago
oops.  that add-on shot is a bugmail thread, and so won't look like that if you don't have asuth's gp-bugzilla add-on (http://hg.mozilla.org/users/bugmail_asutherland.org/gp-bugzilla/) installed.  But you get the idea.
(Reporter)

Updated

10 years ago
Summary: Extension points for thread/multi-message selection → Explicit extension points for front-end code
Hmm, interesting.  This seems similar in some respects to the intent of the Observers module <https://wiki.mozilla.org/Labs/JS_Modules#Observers>, which also lets you alert extensions (and core code) at extension points and pass them JS arguments.

Its API is a bit different, as registration takes a callback function with optional thisObject in Observers (à la array extras and some other JavaScript callback interfaces), whereas ExtensionPointRegistry calls the method with the same name as the extension point in the passed-in object.

And Observers requires multiple arguments to be named arguments encapsulated in an object ({ foo: 1, bar: true, baz: "stuff" }), whereas ExtensionPointRegistry allows multiple arguments in the traditional argument passing style (1, true, "stuff").

Observers is also backwards-compatible with nsIObserverService, which is both a blessing and a curse, since it means you can use it to simplify the handling of existing code that uses that service and even give native code access to the notifications (modulo its ability to handle JS objects), while it also limits its ability to evolve, f.e. to prioritize extensions (as suggested in a comment in ExtensionPointRegistry) or let them stop the registry from notifying later extensions about an extension point (which ExtensionPointRegistry already allows).

Overall, it seems like this'll come in quite handy.  My only suggestion for the initial API would be to consider switching registration to taking a callback function and optional thisObject instead of an object with a same-named method, as the former approach is more flexible, both in terms of how extensions can structure their code and how simple you can make the simplest case (which can then be as minimal as a function literal) and in terms of what you can name the extension points (which can then contain characters not allowed in function names).

thunder and mardak have been thinking about APIs involving callbacks lately, too.  cc:ing them in case they have thoughts.
(Reporter)

Comment 5

10 years ago
myk, thanks for pointing out the similarity to Observers, which I'd seen but forgotten.  I'll give it a shot and make the API changes you suggest.  I'm a bit concerned about the comments about the compatibility with the nsIObserverService, so we may just include the existing API as a starting point and declare backwards compatibility a non-goal. 

jminta pointed out a couple of other things in IRC that I want to make sure to record:

1) there's mileage in asking providers of extension points to "declare" them somehow, and to include a short description, to avoid extension point names that are hugely long so as to include docs.  [Bonus points for detecting that an extension point is already declared, to detect name clashes early.]

[One of the challenges w/ the nsIObserverService is that AFAIK there is no way to know what topics/messages are available to add-on authors except through mxr.  Being able to at least have an about:extensionpoints page that lists the hooks available, would be sweet.]

[One of the subtle points about that declaration is that I think it'd be reasonable to setup review requirements that say "and you have to write a wiki page about it on mdc"]

2) this might want to live in as Steel.UI.hooks.
FWIW, I didn't mean to suggest that Observers obviates the need for this new registry, especially if backwards compatibility would hold you back at all from implementing all the great new functionality you're cooking up (prioritization, cancelability, auto-generated docs, etc.).  Full speed ahead!
Nice. This is basically the subject/observer design pattern. Good idea.

An alternative API would be to make the "extension point" a global object:
In TB core:
gThreadSummaryLoad = new Subject();
...
function foo() {
  ...
  gThreadSummaryLoad.notify();
}
and in extension (same window context)
function onLoad() {
   gThreadSummaryLoad.subscribe(function() { dump("wow"); });

Advantage:
- No global namespace for observers (scope is window, which is appropriate)
- Notifications can be specific for a window
  (not notifying all windows of the same type)
Disadvantage:
- One more line to define the global var
- Need to get a window reference first (usually easy, but not always)

Updated

5 years ago
Assignee: davida → nobody
Status: ASSIGNED → NEW
Whiteboard: [patchlove]
Component: Mail Window Front End → Add-Ons: Extensions API
You need to log in before you can comment on or make changes to this bug.