Closed Bug 535464 Opened 15 years ago Closed 15 years ago

Implement jetpack.me.onFirstRun()

Categories

(Mozilla Labs :: Jetpack Prototype, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: adw, Assigned: adw)

References

()

Details

Attachments

(1 file, 1 obsolete file)

Aza, Myk, and I talked about first run after the meeting today and decided that since a feature's manifest should be declarative, it shouldn't define any first-run callbacks.  Instead, and as Aza proposed in JEP 30, expose an event handler-type method that features invoke to register a callback.  Where does it live?  Features aren't currently able to do any introspection on their dynamic state, which is what we're talking about here, so how about a jetpack.me namespace?  We can imagine other properties being added there over time, e.g., onEnabled(), onUpdated(), and memoryUsage when Electrolysis lands.

This bug will add jetpack.me and its first property, onFirstRun(callback).  Ideally callback would be invoked only when Jetpack has no previous knowledge of the feature (i.e., on the very first install and subsequent installs after purge), but I'm not sure whether it's possible to distinguish this type of install from a soft "reinstall", as about:jetpack calls it.
Attached patch v1 (obsolete) — Splinter Review
Pretty simple patch.  onFirstRun()'s callback is called at the end of context creation, after the first-run page is opened but before it's completely loaded.  i.e., there's no relationship between the first-run page and the callback.

I thought about making it so that the callback is called immediately if the context is already created.  Like, if the feature were to call onFirstRun() in a timeout or XHR or something.  But I don't think it's worth the small amount of additional complexity in our code.  1) My guess is, not a common use case.  2) If we decide it's a good idea, we can add it later.  3) Instead of calling onFirstRun() in async code, the feature could call the async code in onFirstRun().

It would be easy to add other properties to jetpack.me, but we should probably use new bugs and bikeshed appropriately.  (Other than the ones we already want for sure, like onEnabled and onUpdated.)
Attachment #418127 - Flags: review?(myk)
jetpack.me is *not* from the future, to be clear.  Let me know if it should be.
Comment on attachment 418127 [details] [diff] [review]
v1

I'm concerned about the potential for Jetpack developers to mistakenly call a callback unsafely, although I'm not sure how to improve our chances... perhaps wrap firstRunCallback in a function that calls doUnsafeCallback itself within the "me" module, so callers are always protected, and the security-guarding code lives within the module itself, right next to your IMPORTANT comment, and under the aegis of the module, rather than outside the module, away from that comment, and the responsibility of the caller?

Regarding whether or not the "me" API should be from the future, my inclination would be to make it from the future, but I'm not that familiar with its genesis and status; Aza's the better person to make that call.

Otherwise this looks good, r=myk, although it'd be good to have a test for it.
Attachment #418127 - Flags: review?(myk) → review+
Aza, is jetpack.me from the future?
New patch with test and changes re: comment 3, "me" future'd, coming up real soon.
Attached patch v2Splinter Review
* Futures jetpack.me, since it was only born yesterday.
* Includes an end-to-end test.  I had to modify how full jetpack tests are run.  I added an options JSON file in the jetpack tests directory.  One of the options is to simulate first run.  I think this simple options mechanism will come in handy down the road.
* Wraps the callback in the Me module.  I left doUnsafeCallback() as a member of JetpackRuntime.Context, because it will probably be useful for other Context consumers.
* Took the opportunity to fix a one-line nit in JetpackRuntime.showFirstRunPage().
Attachment #418127 - Attachment is obsolete: true
Attachment #418276 - Flags: review?(myk)
Comment on attachment 418276 [details] [diff] [review]
v2

Strangely, this patch fixes two unrelated test errors.  Everything looks good, r=myk.
Attachment #418276 - Flags: review?(myk) → review+
http://hg.mozilla.org/labs/jetpack/rev/7ff523f52fe6
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Awezumtastic! First-Run is...err..um...running!
Updated the JEP accordingly. https://wiki.mozilla.org/Labs/Jetpack/JEP/30
From the JEP: "If an extension is uninstalled and reinstalled (and the original data wasn't removed) then the callback(s) passed to onFirstRun aren't called."

This isn't true in the current implementation for the reason noted in comment 0, and only the last callback passed to onFirstRun() sticks.  I think we should keep proposals in JEPs and docs in docs, so the quote is fine, but I just wanted to make sure you were aware.  I'll add a page to MDC describing first run as it's currently implemented.
Most excellent!
jetpack.me should not be from the future.
Too late, jetpack.me is totally from the future. :O
Ah well, that's good too.
Atul, is it too late for 0.7 to push the small change of un-future'ing jetpack.me?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: