Closed Bug 532860 Opened 15 years ago Closed 15 years ago

Enable features to create a first-run page

Categories

(Mozilla Labs :: Jetpack Prototype, defect)

Product:
Mozilla Labs
Component:
Jetpack Prototype
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: adw, Assigned: adw)

References

(
URL
)

Details

Attachments

(3 files, 6 obsolete files)

screenshot v2: firstRun.content specified
492.66 KB, image/png
Details
screenshot v2: firstRun.content not specified, default embed used
162.78 KB, image/png
Details
patch v3.1
24.53 KB, patch
Details | Diff | Splinter Review
Attached patch patch v1 (obsolete) — — Splinter Review
Atul or Myk, could either of you review this? Atul would be best since it touches JetpackRuntime, JetpackRuntime.Context, and confirm-add-jetpack.{js,html}, but I know you're really busy with the reboot. Aza, I'll attach some screenshots. Replaces the current confirm-add-jetpack.html "Installation Successful" experience with a new tab tailored to the feature, per the "Creating a First-run Page" section of JEP30. When you click the install button on the warning page, that page closes, the "subscribe" event triggers the creation of a new context, and that triggers the opening of the first-run page in a new tab. A feature's manifest can look like this: var manifest = { firstRun: { html: "<html>...", url: "http://...", text: "Basic text...", img: "http://.../foo.png", command: function () ... } }; Any of those properties can be defined. |html| overrides |url| overrides |text| and |img|. Both |text| and |img| can be defined together. |text| is basically |html| with all HTML escaped. |command| is called after the new page has loaded. There's a simple default HTML file for embedding when features don't have a first-run manifest.
Attached image screenshot: url specified in manifest (obsolete) —
Attached patch patch v1.1 (obsolete) — — Splinter Review
Didn't handle the case where firstRun is empty or contains only invalid props.
Attachment #416049 - Attachment is obsolete: true
I find myself wondering why we use command: in some places and onReady in others to specify the ready load state of content...care to enlighten me?
Because different people are working on Jetpack without global guidelines that specify things like onReady vs. command. I choose command since that's what I used for .menu, and to me it seems more appropriate here than onReady, because what's ready?, and everywhere I've seen it, onReady is a function that you pass a callback to, not the callback itself.
Comment on attachment 416112 [details] [diff] [review] patch v1.1 >diff --git a/extension/content/first-run-default.html b/extension/content/first-run-default.html >+ You may now close this page. It would be useful for "close this page" to link to javascript:window.close(). >diff --git a/extension/content/js/confirm-add-jetpack.js b/extension/content/js/confirm-add-jetpack.js >@@ -121,18 +101,21 @@ function fetchSource(uri, onSuccess) { > > function onReady() { > var feedMgr = JetpackSetup.createServices().feedManager; > if (feedMgr.isSubscribedFeed(gCommandFeedInfo.url)) { > if (gCommandFeedInfo.updateCode) > // TODO: Also check to see if updateCode is different from > // the current code. > displayCode(gCommandFeedInfo.updateCode); >- else >- showConfirmation(); >+ else { >+ // Closing the window in the ready handler messes up jQuery(), so delay. >+ setTimeout(function () window.close()); >+ return; >+ } This feels wonky, since it means that installing a jetpack you've already installed will simply open and then quickly close a page. Perhaps we could display some message at this point notifying the user that they have already installed the jetpack? To be extra useful, we could check for updates at this point and update the jetpack if it's out of date, since that might be what the user was trying to do. Otherwise the patch looks good, although I'm a bit concerned about the API being presented, since it seems like we're overwhelming authors with options for populating the page. That is better addressed in a discussion on the API, however.
Attachment #416112 - Flags: review+
Thanks, Myk. (In reply to comment #6) > This feels wonky, since it means that installing a jetpack you've already > installed will simply open and then quickly close a page. Perhaps we could Yeah, worse yet is installing from the Gallery (or some other trusted source). The red warning page appears briefly, then it's immediately closed. Of course the current experience is that the red page appears briefly, then its content is replaced with the Installation Successful content. > Otherwise the patch looks good, although I'm a bit concerned about the API > being presented, since it seems like we're overwhelming authors with options > for populating the page. That is better addressed in a discussion on the API, > however. Might it be wise to have that discussion before we put it in? I'm concerned that changing things later, with features relying on those things, might be harder. We can always start small.
(In reply to comment #7) > Might it be wise to have that discussion before we put it in? I'm concerned > that changing things later, with features relying on those things, might be > harder. We can always start small. Yeah, that's a good idea. For example, we could start with a couple of concrete use cases, perhaps based on the dichotomy we've previously described of two main types of developers: one who wants the simplest possible API and another who wants the best possible user experience. I'll raise the issue in the discussion forum.
Attached patch patch v2 (obsolete) — — Splinter Review
Changes: * manifest.firstRun accepts two properties, "content" and "callback". If content is a string and valid URI, it's used. Otherwise, it's assumed to be HTML. "command" from the previous patches is renamed callback. * The red warning page is not opened at all unless it needs to be. * The default first-run embedded page links to window.close(). * A fade-in effect for the entire first-run page. * For features that supply a firstRun, there are helpful links to about:jetpack and window.close() under the main banner, outside the embedded content. * The first-run page is now more self-contained. It's responsible for updating itself given its URL, not jetpack-runtime.js as was previously the case. This also is more like how the red warning page works.
Attachment #416112 - Attachment is obsolete: true
Attachment #417982 - Flags: review?(myk)
Attachment #416050 - Attachment is obsolete: true
Attachment #417985 - Attachment description: screenshot v3: firstRun.content not specified, default embed used → screenshot v2: firstRun.content not specified, default embed used
Drew: This is really looking good. I like the simplification to "content" (have we updated the JEP accordingly?). I'm with Daniel that onReady is the label that I would be expecting, given that is what we use for statusBar.
Thanks, Aza. I'll update the JEP and add a page to MDC. I don't like onReady in this case because it's not clear what's ready -- the window? Which window? The first-run window? If I access window or document or |this| in my callback, will it work like it does with onReady for status bar items and elsewhere? It's really just a context-free callback, which is the word Myk suggested, and it makes sense to me. Did I convince you? :)
I'm with Drew here, since onReady makes sense in a context where some asynchcronous process is taking place (f.e. a resource is being loaded), and the extension wants to be notified when it completes, whereas in this case it seems like the goal is just to notify extensions that they are being run for the first time, not that their first run page has loaded.
Attached patch patch v3 (obsolete) — — Splinter Review
Not much different from v2. Changes after the meeting today: * manifest.firstRun is gone, replaced with manifest.firstRunPage, which is either a string or XML. * No mechanism for specifying a callback; that's to be handled by a new mechanism in a new bug.
Attachment #417982 - Attachment is obsolete: true
Attachment #418058 - Flags: review?(myk)
Attachment #417982 - Flags: review?(myk)
Comment on attachment 418058 [details] [diff] [review] patch v3 Cancelling this review as Drew, Aza, and I talked this through and agreed to change the way the callback is specified such that it hangs off a "me" object and is passed to an onFirstRun method that registers it as a first-run callback, whereupon it gets called at the appropriate time.
Attachment #418058 - Flags: review?(myk)
Comment on attachment 418058 [details] [diff] [review] patch v3 Erm, never mind, I cancelled the wrong review, as Drew updated the patch before I got to it!
Attachment #418058 - Flags: review?(myk)
(In reply to comment #16) > * No mechanism for specifying a callback; that's to be handled by a new > mechanism in a new bug. Bug 535464.
Comment on attachment 418058 [details] [diff] [review] patch v3 >+ // Get the context's first-run page from its manifest. >+ var page = context.manifest ? context.manifest.firstRunPage : null; As of a very recent checkin, contexts always have a manifest (we create a default empty object manifest if the jetpack doesn't provide one), so you don't need to do this check anymore. I also saw a couple spurious "Ci is not defined" errors in testing, but I can't reproduce them, and Ci is defined the only place this patch references it. And I sometimes see "Error: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsISelectionPrivate.removeSelectionListener] Source file: chrome://jetpack/content/js/selection.js Line: 324" when the first run page appears, although I can't reproduce consistently. It doesn't appear to affect functionality, however. Note that there's a trivial conflict with a nearby change to code in jetpack-runtime.js that you'll need to resolve before checking in. Otherwise, this looks great. r=myk
Attachment #418058 - Flags: review?(myk) → review+
> As of a very recent checkin, contexts always have a manifest (we create a > default empty object manifest if the jetpack doesn't provide one), so you don't > need to do this check anymore. Erm, actually that wasn't true, as the checkin hadn't happened yet, but it has now (I guess that makes it *very* recent!): http://hg.mozilla.org/labs/jetpack/rev/7b24d5e567a8
(In reply to comment #20) > I also saw a couple spurious "Ci is not defined" errors in testing, but I can't > reproduce them, and Ci is defined the only place this patch references it. > > And I sometimes see "Error: Component returned failure code: 0x80004005 > (NS_ERROR_FAILURE) [nsISelectionPrivate.removeSelectionListener] Source file: > chrome://jetpack/content/js/selection.js Line: 324" when the first run page > appears, although I can't reproduce consistently. It doesn't appear to affect > functionality, however. I see these too. Both come from selection. They look like what errors look like when callbacks are running after the underlying window has closed. The first one is: Error: Ci is not defined Source File: chrome://jetpack/content/js/selection.js Line: 177 I can trigger it after clicking the about:jetpack link on the first-run page a few times. I can trigger the second by restarting Firefox and letting session restore restore the first-run page. Hmm...
Well I've convinced myself the errors aren't first-run's fault. What's happening is, jetpack.selection listens for all page loads. It sees first-run's embedded iframe's load and adds a selection listener on its window. On page unload, jetpack.selection removes the selection listener. For some reason in some instances on unload the iframe passes all of Ed's checks and so the selection listener is removed, but the window's selection doesn't actually contain the listener T. Other than leaking selection listeners presumably, the errors don't seem to have any ill effect. I'll push this patch and file a separate bug if need be, but I'm not going down this rabbit hole tonight.
Attached patch patch v3.1 — — Splinter Review
Address comment 20.
Attachment #418058 - Attachment is obsolete: true
http://hg.mozilla.org/labs/jetpack/rev/8295a67625f4
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
I wonder if it has anything to do with that setTimeout wrap magic of onPageLoad.. Well, the problem comes from something (onUnload for BrowserWatcher or onPageUnload for DOMContent listener) /not/ calling removeSelection for the now-stale Selection, so the stale-notifySelectionChanged still manages to get called. Oh so it might be that wrap magic where it's in the process of unwrapping the wrapped addSelection, but stuff gets unloaded then the last unwrapping actually calls addSelection.. ?? .. magic?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: