Aza is point, see JEP for further details: https://wiki.mozilla.org/Labs/Jetpack/Reboot/JEP/105

Note these changes to the JEP that I proposed earlier in a discussion group conversation: * instead of the "me" module, provide the API via the (now existing) "self" module, as that is a more conventional term for such functionality; * implement onInstall and onUpgrade but not bind, per design guidelines for callback properties; * delay implementation of the install/uninstall manifest pages to a later phase of development, as it feels like those need more thinking, particularly the uninstall page, and it is relatively easy to open an install page via the onInstall callback and the (future) tabs API.

Those changes sound great. Another issue: The JEP says "no use case for uninstall, enable, disable", which sounds very wrong to me. Wrt to uninstall, the JEP itself lists a survey as an uninstall use-case in the Use Cases section :) Regarding enable/disable: I think these are more important than ever, now that those events take effect without an application restart. We should be encouraging add-ons to cleanly stop work (shut down page-workers, kill timers, close streams) when they're disabled. And conversely we should tell an add-on to kick it's services back into gear when it's enabled. I'm going to update the JEP to reflect Myk's comments, add these additional APIs, and then restart the conversation on the forum.

JEP updated: https://wiki.mozilla.org/Labs/Jetpack/Reboot/JEP/105

Actually, the disable/enable case is just conflated with load/unload in Jetpack: when the addon is disabled, for instance, any callbacks the code registered with require("unload").when() or require("unload").ensure() are automatically called, and when the addon is enabled, its code is re-evaluated and its exports.main() is called.

Oh sweet, so we get it for free! I'll update the JEP again.

stub impl, based on the JEP and comments so far. no tests yet, haven't even tried running the code. at this point, it registers an event handler object with a fictitious addLifeCycleEventHandler() method of the packaging global, to allow the harness to call these. once Atul nails down rebootless add-on support, we can look at exactly how it needs to be done. from looking at the harness, seems like we'll need to either put this stuff in a bootstrap file per add-on, or call main() and *then* call these handlers.

Simple storage needs this, or maybe a lower-level version, to purge persistent data after an extension's been uninstalled. Does the rebootless stuff really block, or can we just view rebootfull as a subset of rebootless?

Removing simple storage dependency -- it handles uninstall internally. Maybe later we can switch it to the life-cycle module.

Notes to self: - extension manager notifications - 1.9.3 https://wiki.mozilla.org/Extension_Manager:API_Rewrite:API - 1.9.2 https://developer.mozilla.org/en/Observer_Notifications#Extension_Manager - my test extension is notified about itself for these events: - 1.9.3 - onInstalled - onDisabling (from [Remove] [Disable]) - onEnabled (from [Remove] [Enable]) - onUninstalling (from [Remove] [Disable]) - 1.9.2 - item-disabled (from [Disable] [Uninstall]) - item-cancel-action (after disabling, from [Enable] [Uninstall]) - item-uninstalled (from [Disable] [Uninstall]) - item-cancel-action (after uninstalling, from [Cancel]) - item-uninstalled (after disabling, from [Enable] [Uninstall]) - item-disabled (after canceling uninstall after disabling, from [Cancel]) - so on 1.9.2, not notified about its own install

bootstrap.js makes this almost trival on 1.9.3. This patch pretty much completely works already. (Thanks to Dietrich for hinting that these notifications should originate in the bootstapper on 1.9.3, not the extensions manager.) The question is how best to wire everything up, since there are multiple possibilities: 1. bootstrap.js uses nsIObserverService to notify, and the self module listens and in turn calls its observers. 2. Both the bootstrapper and self have access to packaging, so define some observer list on it, let self add observers, and route messages through it. But, that's kind of abusing packaging, since life cycle notifications aren't "metadata related to the packages available in the current environment," as the docs put it. 3. So add some new global for that purpose that self could access. 4. Life cycle notifications are similar to unload notifications already supported by the unload module. So emulate that example. Define a method on the loader that the bootstrapper calls. This method in turn require()s self and passes along the notification by send()ing it to self. self then passes it to its observers. This patch does 4. For 1.9.2, I'm thinking the harness.js component will have to talk to the extension manager. harness.js manages the loader, so notifying the loader from there should be OK. It seems artificial to consider unload as not a life cycle event. I think it would be cool to merge the unload module into a new life cycle module and forget about adding life cycle stuff to self.

(In reply to comment #10) > It seems artificial to consider unload as not a life cycle event. I think it > would be cool to merge the unload module into a new life cycle module and > forget about adding life cycle stuff to self. That's fine with me. The main impetus for just copying over Narwhal's unload module was so that if it eventually became a CommonJS standard, we'd support it out-of-the-box, but we can always just add it to lifecycle and folks can make a stub unload module that trivially delegates to lifecycle if needed.

Adds install, uninstall, and disable support for 1.9.2 by observing EM nsIObserverService notifications in the harness.js component and calling a life-cycle notification method on the loader, the same method called in the 1.9.3 case by bootstrap.js. Still need to do enable, upgrade, and downgrade on 1.9.2. AFAICT the EM manager doesn't provide an easy way of detecting these. In the worst case we could do a lot of work by serializing all add-on metadata at shutdown so old state is available after reboot for comparison with current state. With Fx 4 on the horizon, I'd rather just not support these on 1.9.2 if it comes to that. (In reply to comment #11) > That's fine with me. The main impetus for just copying over Narwhal's unload > module was so that if it eventually became a CommonJS standard, we'd support it > out-of-the-box, but we can always just add it to lifecycle and folks can make a > stub unload module that trivially delegates to lifecycle if needed. Cool, actually I'm planning to do that work myself, if only because it means not having to replace modules' uses of the unload module everywhere.

(In reply to comment #12) > In the worst case we could do a lot of work by serializing all add-on metadata > at shutdown so old state is available after reboot for comparison with current > state. As a matter of fact, the EM does just that for its own needs in extensions.{rdf,ini}. I could sniff those files like it does, but the problem is that it removes the relevant info and then tells the app to restart itself so the add-on changes can be made. After that second restart, the info's gone.

Atul, how about this. unload module callbacks are called with a single argument, the unload reason: "uninstalled", "disabled", etc. Add a new "load" module that works like unload, except it only has a when(). load module callbacks are likewise called with a load reason: "installed", "enabled", etc. Or instead of a load module, pass the load reason to main(). That would mean only programs are notified. That doesn't seem unreasonable, since being installed or enabled has meaning only at the granularity of programs and not really for modules. One other concern I had is extensions (or "hosted" programs) vs. standalone programs. Do unload and load reasons make sense for the latter?

Good questions. I like the extra argument for unload.when(), though I'm also wondering about when it gets called: if I've got an extension that's installed and enabled, and I tell Firefox to uninstall it, does unload.when() get called once for 'disable' and again for 'uninstall'? This seems like it might break existing unload() behavior... And what if I had an extension that was disabled, and I wanted to uninstall it? Would any callbacks get called at all, or would Firefox load the extension for a split-second just so it could send it an uninstall event? I found the new bootstrap.js events sent by the new addon manager similarly edge-casey, which made it a bit more confusing to use than I'd expected (not that this is Mossop's fault, it's just a very thorny topic). One thing we may want to consider is actually having the reason be, in C terms, a collection of bitflags rather than a string, so that e.g. unload.when() could be called *both* because of disable *and* because of uninstall. In JS this could be implemented e.g. by passing a custom object with property names that are present or not-present depending on the value of the bitflag, e.g. "{disabled: 1, uninstalled: 1}", which an unload.when() handler could query via "if 'disabled' in reason" or "if 'uninstalled' in reason". This could also potentially be more forwards-compatible, if we decide to change things later for some reason. As for your last question, I think that load/unload reasons make some sense for the standalone app case, since standalone programs have some notion of being installed/uninstalled vs. being started-up/shutdown--only in this case enable == startup and disable == shutdown. Not that we necessarily have any way of telling when a program is uninstalled on some systems, but there seem to both be enough analogs, and standalone programs are enough of a non-priority, that I don't think it's that big a deal to just focus on the addon case for now.

I'm not proposing any changes to unload observers or when they're called other than the new reason argument. Practically speaking, we don't have to worry about any of those thorny issues (or at least we shouldn't worry ourselves with them) because the bootstrap.js interface limits our visibility. On both startup() and shutdown() we're passed a single reason, not a bit field. And as long as an extension is unloaded -- because it's disabled for example -- there's no expectation that it will be notified on "subsequent" unloads or loads or anything else. For rebootfull extensions, there's at most one pending action at shutdown, so for instance if you disable, then uninstall, and then exit the app, the uninstall event wins, and that's what's passed to unload.

Atul, requesting feedback on these platform changes. A `loadReason` property is included on the options object passed to a program's main(), if a load reason could be determined. Similarly, a reason argument is passed to unload.when() callbacks and unload.ensure() callbacks, if an unload reason could be determined. (Maybe "event" is a better name than "reason"?) As I mention in comment 16, nothing changes about unload callbacks except this new argument.

Comment on attachment 448781 [details] [diff] [review] WIP 4 Myk, requesting feedback since this approach ignores the JEP altogether. As I talk about starting in comment 10, it seems more reasonable to treat life cycle events as part of the platform, like the unload event currently is, not as a discrete high-level API. This patch also changes the names of two of the events slightly, making "installed" and "enabled" past tense to indicate that they have already happened by the time you receive them. ("upgrade" and "downgrade" are potentially a problem, since they can occur on both startup and shutdown and therefore might need to be past tense sometimes (startup) and present tense others (shutdown). I need to investigate, but if that's the case, probably all events should just be present tense to avoid complexity.) If you have comments on the platform changes, that would be great too.

Comment on attachment 448781 [details] [diff] [review] WIP 4 This looks good overall. A few comments: * Since this is a WIP, I assume you'll do it once you're done, but just wanted to make sure you update the unload() docs with info on the new behavior. I don't think we currently document the 'options' object passed to main() but we need to... * Could you move lifecycleobserver192 outside of the harnessservice closure and have all its dependencies passed in to it? i think it'd make it easier to understand what its dependencies are if they're explicitly passed in, and easier to extract it from the rest of the harness if/when we stop supporting 1.92, but it's just a suggestion.

(In reply to comment #18) > (From update of attachment 448781 [details] [diff] [review]) > Myk, requesting feedback since this approach ignores the JEP altogether. As I > talk about starting in comment 10, it seems more reasonable to treat life cycle > events as part of the platform, like the unload event currently is, not as a > discrete high-level API. It is important for these events to be part of the platform. And that is certainly the appropriate way to provide them to SDK and library developers who need them to clean up after their high-level APIs when users disable and uninstall addons. However, there are also important and common use cases for regular addon developers to have access to versions of these events, and we should provide a high-level API for those developers. Two particular cases are knowing when an addon has been installed in order to provide a first run experience and knowing when an addon is being disabled in order to remove transient modifications to browser chrome that are not managed by the platform. Even though we may provide an even higher-level API for handling first run in a consistent fashion across addons in the future, and the need for unmanaged modifications will decrease over time as we build out our API set, we aren't there yet, and even when we get there, there will always be exceptions. We could decide to treat all developers who need to track these events as SDK/library-level developers who should be able to handle low-level interfaces, but some of them won't be, and I think we're better off providing those ones with a high-level API whose design is consistent with that of the other APIs they are used to using. So I do think we should provide at least self.onInstall (which is called "install", even if it runs on "startup" or at some later stage in the bootstrapping process, because it matches the conceptual model for the target audience) and self.onDisable. I'm not as sure about the value of self.onEnable, so I could go either way on it. And as Atul notes, self.onUninstall is a tricky case. I can think of a few possible approaches but no obvious winners, and I think we should punt on it for the initial implementation, then cogitate some more while turning a keen eye to the kinds of things addons developers are doing that we might want to enable them to clean up after. > This patch also changes the names of two of the > events slightly, making "installed" and "enabled" past tense to indicate that > they have already happened by the time you receive them. ("upgrade" and > "downgrade" are potentially a problem, since they can occur on both startup and > shutdown and therefore might need to be past tense sometimes (startup) and > present tense others (shutdown). I need to investigate, but if that's the > case, probably all events should just be present tense to avoid complexity.) I'm ok with mixing tenses for low-level APIs, if you feel it's appropriate. For high-level APIs, however, I'd like to stick with the present-tense naming convention specified in the API design guidelines, because the most common DOM events for web pages (focus, load, click, etc.) use that convention, and the consistency makes it easier to remember the names of the events (unlike, say, for Mozilla-specific DOM events and observer notifications, which employ a number of different naming styles, with variations not only in tense but in several other respects). Also, in most cases there is only a single point in time at which it makes sense to notify handlers about an event, even if that point is right after the event has happened, so there is no ambiguity about when the events actually occur, even if some events are dispatched while something is happening and others just afterward. Of course there are a few cases where it makes sense to conceptualize something that happens as a single event with multiple notification-worthy stages, like when a XUL app shows a popup (popupshowing, popupshown) or a user enters private browsing mode (private-browsing-cancel-vote, private-browsing-change-granted, private-browsing, private-browsing-transition-complete). We don't have a convention for handling such events yet, but for the high-level API of the private browsing service we use onBeforeStart, onStart, and onAfterStart for the stages of entering private browsing mode and onBeforeStop, onStop, and onAfterStop for the stages of leaving it. We might use this as the template for future such multi-stage events (f.e. onBeforeShow in addition to onShow in the Panel API if we identify a use case for notifying consumers before we show the popup). > If you have comments on the platform changes, that would be great too. I'm not yet very familiar with the cuddlefish code, but the changes all look reasonable to me. One note is that it would be handy for reasonToString() to be somehow provided by bootstrap.js to harness.js, so lifeCycleObserver192 could have access to the strings it provides (instead of repeating them).

Comment on attachment 448781 [details] [diff] [review] WIP 4 Erm, I forgot to twiddle the flag. Feedback+ for the platform changes, but of course I'd still like to see a high-level API that sits on top of them.

Thanks Myk. I guess I don't understand what "high-level" means. Why are the unload module and the options passed to main() not high-level or insufficient for regular add-on developers?

(In reply to comment #22) > Thanks Myk. I guess I don't understand what "high-level" means. Why are the > unload module and the options passed to main() not high-level or insufficient > for regular add-on developers? It's mostly that the module uses an unconventional callback property name and method of registering a callback, and one of my goals for the high-level API set is for similar interfaces to appear and behave similarly, so developers can transfer their expectations from one module to another to reduce the learning curve for each additional API they use, which serves our target audience better than consistency with Narwhal or a theoretical future CommonJS standard. A minor issue is that it is one more module to require, which seems unnecessary, given that the unload module provides so little functionality, and what it does provide is a good conceptual fit for the self module. The load functionality is a bit of a different story. Although passing the reason as an option to main is unconventional and somewhat inconsistent with the interface for unload (whether we put that interface into unload or self, and regardless of what we call it), it is certainly an elegant interface that provides the relevant information at just the right time. So I could go with it for load notification. And if load notification is provided via a "reason" option to main, then it makes sense to provide unload notification via a "reason" parameter to an unload callback. However, I would still want that callback to have a conventional name (like onUnload). And it still makes more sense to me for it to be a property of the self module (or perhaps a callback exported by the main module itself) than a property of a separate unload module.

Just a note while it's on my mind: If we want add-ons to always be notified on uninstall (even if they're disabled) or otherwise be able to provide some persistent callback that's guaranteed to be invoked on uninstall, we should talk to Dave about providing support for it at the Firefox/platform level ASAP.

Thanks for explaining, Myk. That all makes sense. > onUnload). And it still makes more sense to me for it to be a property of the > self module (or perhaps a callback exported by the main module itself) than a > property of a separate unload module. An unload callback exported by the main module -- I like it. If a load reason is passed to main(), then there's a nice symmetry in likewise not having to require a module to set up an unload handler; it's all "built in," contained to the main module. There's a function for entering and a function for exiting. As a secondary benefit, I think it also avoids potential confusion for people writing libraries/packages -- do I use the unload module or the self module to observe unload? Atul, how's that sound? (If on the other hand we have self.onLoad/onInstall/etc. instead of a reason passed to main(), I think self.onUnload/onUninstall/etc. would be better. But it sounds like you're OK with passing reason to main().)

(In reply to comment #25) > (If on the other hand we have self.onLoad/onInstall/etc. instead of a reason > passed to main(), I think self.onUnload/onUninstall/etc. would be better. But > it sounds like you're OK with passing reason to main().) Yup, I'm ok with it and agree about the symmetry.

* options.loadReason is passed to main(). * A program can export an `onUnload` function that will be called when its loader is unloaded. onUnload is called just before the loader is unloaded. It's passed an unload reason. * Adds "startup" and "shutdown" events to signal normal app startup and shutdown, rather than the reasons being undefined in those cases. * On 1.9.2 we support "install", "startup", "uninstall", "disable", and "shutdown", but not "enable", "upgrade", and "downgrade". * As Myk mentioned in comment 20, the event strings shouldn't be duped between bootstrap.js and harness.js. But bootstrap.js can't pass anything to harness.js, since bootstrap.js is ignored on 1.9.2. The passing could go the other way, but then harness.js would need to dupe the ADDON_* constants. No avoiding duping something, so I've just left it and added some comments about it. If I've missed a better way, let me know!

Comment on attachment 449718 [details] [diff] [review] for review v1 Looks good, I only really have one main question: * unload.send(reason) - can the reason be undefined or have a default value? i.e., what should .send() do when it just wants to send a generic unload event to a module, as was the previous usage, and which is currently done when e.g. a cuddlefish Loader's unload() method is called? Should unload.send() w/ no args just default to a reason of 'shutdown'? or should it actually raise an exception? And a few nits: * while a program's exported onUnload() seems symmetrical with main(), it also seems odd that we're introducing another unload-notification-mechanism here. I suppose it is semi-analogous to the way the web platform allows window.onload to be defined, yet also supports addEventListener... Still, the addition of this method seems odd to me--i could see new developers stressing over what the difference is and which one they should use. * it seems DRY-violating that lifeCycleObserver192._addonId() adds "@jetpack", while the 'self' module should really be the one doing it. I wonder if there's a way we can define the transformation function in harness.js, add it to the 'packaging' export, and have lifeCycleObserver192 and 'self' ultimately call the same functionality. Probably not that big a deal if it's a huge hassle, though.

(In reply to comment #28) > * unload.send(reason) - can the reason be undefined or have a > default value? i.e., what should .send() do when it just wants to > send a generic unload event to a module, as was the previous > usage, and which is currently done when e.g. a cuddlefish Loader's > unload() method is called? If for some reason an unload reason can't be determined, I'm fine with leaving the reason undefined and documenting that. But otherwise, all calls to send() and harness' and loaders' unload() (except in tests, where it's not necessary) should be updated to pass a real and accurate reason. If you agree, could you point me to calls that I've missed? I grepped for send(), and the only one not in a test is the one in the Cuddlefish loader, which I updated. > * while a program's exported onUnload() seems symmetrical with > main(), it also seems odd that we're introducing another > unload-notification-mechanism here. I suppose it is semi-analogous > to the way the web platform allows window.onload to be defined, > yet also supports addEventListener... Still, the addition of this > method seems odd to me--i could see new developers stressing over > what the difference is and which one they should use. It sounds like you and Myk disagree. Is that right? (Asking high-level clients to use the unload module and not adding a separate "high-level" version(s) of unload(), which is what the JEP specifies, was my ideal solution FWIW.) > * it seems DRY-violating that lifeCycleObserver192._addonId() adds > "@jetpack", while the 'self' module should really be the one > doing it. I wonder if there's a way we can define the > transformation function in harness.js, add it to the > 'packaging' export, and have lifeCycleObserver192 and 'self' > ultimately call the same functionality. Probably not that > big a deal if it's a huge hassle, though. I'll try that.

(In reply to comment #29) > If for some reason an unload reason can't be determined, I'm fine with leaving > the reason undefined and documenting that. Sounds good! > But otherwise, all calls to send() > and harness' and loaders' unload() (except in tests, where it's not necessary) > should be updated to pass a real and accurate reason. If you agree, could you > point me to calls that I've missed? I grepped for send(), and the only one not > in a test is the one in the Cuddlefish loader, which I updated. Cool, I don't know of any calls that you missed. > It sounds like you and Myk disagree. Is that right? (Asking high-level > clients to use the unload module and not adding a separate "high-level" > version(s) of unload(), which is what the JEP specifies, was my ideal solution > FWIW.) As we just discussed in the weekly Jetpack meeting, it looks like it's just a matter of distinguishing between the low-level and high-level APIs, so this is no longer an issue. Woot!

Comment on attachment 449718 [details] [diff] [review] for review v1 >diff --git a/packages/jetpack-core/docs/unload.md b/packages/jetpack-core/docs/unload.md >+called. *callback* is called with a single argument, one of the >+following strings describing the reason for unload: `"uninstall"`, >+`"disable"`, `"shutdown"`, `"upgrade"`, or `"downgrade"`. (On Gecko >+1.9.2-based applications, such as Firefox 3.6, `"upgrade"` and >+`"downgrade"` are not available, and `"shutdown"` will be sent in >+their place.) Nit: it would be useful to mention that an addon that is unloaded because "disable" will not be notified about "uninstall" if it is uninstalled after being disabled (and that we are investigating future solutions to this issue). Otherwise this looks great, r=myk.

* Updates cuddlefish/__init__.py and harness options for bundleID as Brian suggested in bug 567904 comment 9 * Adds bundleID to Packaging * Uses bundleID in lifeCycleObserver192 as Atul suggested in comment 28 * Updates unload docs to mention reason might be undefined * Updates unload docs to mention the "uninstall" after "disable" case as Myk suggested in comment 31

Comment on attachment 450157 [details] [diff] [review] for review v2 Awesome! Only other thing I could think of doing is creating a bug # to resolve the issue of notifying disabled addons when they're uninstalled, and writing that bug # in the unload docs... cfx docs will automatically hyperlink it, no need to add any extra junk. anyways, just a thought, it looks great as-is. Thanks!

(In reply to comment #33) > Awesome! Only other thing I could think of doing is creating a bug # to resolve > the issue of notifying disabled addons when they're uninstalled, and writing > that bug # in the unload docs... Oh yeah, I meant to bring this up again (see comment 24): If we really want to solve that, we should talk to Dave ASAP about providing support in toolkit for some persistent on-uninstall callback, no? > cfx docs will automatically hyperlink it, no > need to add any extra junk. Uh, awesome.

(In reply to comment #34) > Oh yeah, I meant to bring this up again (see comment 24): If we really want to > solve that, we should talk to Dave ASAP about providing support in toolkit for > some persistent on-uninstall callback, no? Yeah, though I'm not sure if it's actually that viable, after talking to dave. We should at least make a boog for it, if for no other reason than to ultimately mark it WONTFIX and give the rationale in the comment stream. > > cfx docs will automatically hyperlink it, no > > need to add any extra junk. > > Uh, awesome. Hehe. There's a number of conveniences like that in the doc server, but I really need to document them so that if something else wants to serve the markdown, like flightdeck, it appears the same.

Filed bug 571049 and referred to it in the unload docs. Thanks guys! http://hg.mozilla.org/labs/jetpack-sdk/rev/fc935cf8bcb0

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product. To filter bugmail related to this change, filter on the word "looptid".