It should be possible to open a popup panel containing rich content from a statusbarpanel, so statusbarpanels that provide a bit of information can show the user more details in a popup panel on mouseover.

Reopening, as we need a bug for implementation that is separate from the bug for specification, and this bug is the most appropriate, as its description is implementation-focused, even though this capability is not specific to statusbar panels.

cc:ing roc for his thoughts, as this is proving tricky because of platform limitations. The goal is to enable jetpack to create panel into which they can securely load arbitrary content. My first thought was to use a XUL panel containing a content iframe into which we load a jetpack's content, but, per the note in the XUL panel docs <https://developer.mozilla.org/en/XUL%3apanel>, which I subsequently confirmed by testing on the latest 1.9.2 nightly, bug 130078 prevents mouse interaction with content loaded into content iframes in XUL panels. My second thought was to make my own panel using a fixed position content iframe containing another content iframe, with the inner iframe loading the content while the outer iframe draws the panel borders. That's not quite as good as using a XUL panel, since the panel wouldn't be able to extend beyond the borders of the window, but it would probably be ok. And it works until some other code adds a node to the DOM (f.e. when the user opens a new tab), at which point the panel gets hidden by the new node where they overlap. XUL ignores z-index, and the behavior of overlapping elements in XUL is unspecified, so this is not unexpected, although it is unfortunate. The XUL way to layer elements is via a stack, and I could use a XBL binding to stick all chrome into a stack on top of which I place the aforementioned homegrown panel, but that seems brittle, likely to break any other extension or core code that depends on the regular hierarchy of elements in the window. mconnor also suggested it might have noticeable performance implications. However, Jetpack's slidebar implementation already places the tabbrowser in a stack in order to put slidebars on top of it, so I could reuse that stack. But that would mean that panels can't appear outside of the tabbrowser content area. My last thought was to create a real window that emulates a panel by stripping it of its window decorations, removing it from OS lists of windows (f.e. the taskbar, the Alt-Tab list), and moving it whenever the browser window to which it is attached is moved or resized. I have no idea if that's possible, however, and it certainly sounds impractical. roc: do you have any ideas about how best to accomplish this goal besides the long-term solution of fixing bug 130078?

One thing we could do is to give you a XUL attribute that "fixes" 130078 for just that IFRAME. Not ideal, since it would expose chrome to the regressions in bug 130078 with regard to that IFRAME, but if the content there is not too complex and doesn't reload much, it should be OK.

I did some testing with a XUL stack, and it turns out to be problematic too... In theory, it seems like it should be possible to unconditionally layer a content iframe on top of both the browser chrome and any content in the tabbrowser by wrapping the browser chrome in a stack and placing the content iframe on top of the stack via this XBL binding: <binding id="window"> <!-- gets bound to XUL window element --> <content> <xul:stack flex="1"> <xul:vbox flex="1"> <children/> </xul:vbox> <xul:iframe type="content" src="http://www.mozilla.org/" left="20" top="20" style="width: 400px; height: 300px"/> </xul:stack> </content> </binding> And initially it seems to work: https://people.mozilla.com/~myk/bug494238/panel-one-tab.png But further testing reveals two significant problems: 1. When multiple tabs are open, the iframe only appears on top of the content of one of them: https://people.mozilla.com/~myk/bug494238/panel-other-tab.png 2. When you scroll the content that the iframe is on top of, the view gets corrupted: https://people.mozilla.com/~myk/bug494238/panel-scroll.png (Triggering a paint event, f.e. by resizing the window, fixes the corruption, although it returns as soon as you scroll again.)

(In reply to comment #4) > One thing we could do is to give you a XUL attribute that "fixes" 130078 for > just that IFRAME. Not ideal, since it would expose chrome to the regressions in > bug 130078 with regard to that IFRAME, but if the content there is not too > complex and doesn't reload much, it should be OK. It's arbitrary content loaded by jetpacks, so it's impossible to know how complex it'll be or how often it'll get reloaded. But I'd be willing to try out this solution against a set of common testcases. If it works for enough of those, it might be good enough until we can get a better long-term solution in place!

I'm kinda busy but anyone could implement it really, it's just updating the patch in bug 130078 and making it conditional on the presence of an attribute in the container element.

The current status of this is that it's blocked by platform bug 130078, although the fix for bug 533845 might turn out to be independent of that bug, in which case we would only be blocked by bug 533845. I also filed bug 532569 on the workaround suggested in comment 4. And I've been working on a workaround (based on initial work by Anant, who based his work on Fennec, I think) that uses a hidden iframe painted onto a canvas in a XUL panel (like in Fennec), with events being forwarded back from the canvas to the iframe <https://hg.mozilla.org/users/myk_mozilla.com/jetpack-panel>. I have that workaround partly working, in that I'm able to hide an iframe in the chrome browser window and paint it to a canvas in a XUL panel, but there's there's still a lot to work out before I'll be able to say whether or not it'll be sufficient. Among other issues: 1. A mouseover event in the canvas (which Fennec doesn't handle, probably because it doesn't expect a mouse pointer) triggers a mouseout event in the canvas when the mouseover is forwarded to the iframe, which then triggers another mouseover in the canvas, ad infinitum. 2. Mouseover/out and click events don't seem to fire on any link below the first line of text. 3. Key events trigger an infinite loop.

I have exactly the same problem and wanted to ask if you already have a workaround (I need to display an iframe in a panel/popup) ? Or can maybe someone point me to more information about the fix mentioned in Comment 7 ? Any hint is highly appreciated !

I don't have a workaround, but bug 533845 has been fixed on the trunk, and the fix has been backported to the 1.9.2 branch for inclusion in the next dot release of Firefox, so it's no longer necessary to work around this problem (unless you need to support older versions of Firefox).

Yes, but that's the problem, I have to support also older versions of Firefox ... Thanks !

I've made progress (latest version is in https://hg.mozilla.org/users/myk_mozilla.com/jetpack-packages/), but this isn't going to land before the freeze for 0.3 tonight. -> 0.4

Here's a work in progress that implements a "panel" module with a Panel constructor for making panels and displaying panels. It also adds a "panel" property to Widget to which you can assign Panel objects that then appear when you click the widget. It mostly follows the JEP, except that "anchor" is now a parameter of the |show| method rather than a property of the Panel object, since I was able to implement long-lived panels that can be shown/hidden multiple times rather than short-lived panels that can only be shown/hidden once, and for a long-lived panel the anchor (if any) is best determined when the panel is being shown. Some notes: * Like context-menu, page-worker, and widget, you have to |add| a panel to activate it (and should |remove| it once you're done with it). * Per the note above about long-lived panels, content loads on add/remove, not show/hide. Thus you can load a panel in the background and leave it around, updating it (or letting it update itself) as appropriate, until it's time to show it. * Panel positioning/styling needs work, most of which is dependent on platform work to implement "arrow panels" (a.k.a. speech balloons) in bug 554937. * The implementation reuses page-worker, splitting it into two modules, page-worker and promiscuous-page-worker, the latter of which exposes its containing iframe element, which Panel needs. This required mostly just refactoring along with some minimal API changes (page.window/document is null rather than undefined when the page is inactive) and some lexical scope hackaroundery. I'm not entirely sure this is the optimal approach (or that promiscuous-page-worker is the optimal name), but it works and seems reasonable enough. Still to do: * unit tests; * documentation; * refinements to the example addon; * pending the outcome of the discussion about the Page Mods API, changes to the API such that access to the panel's DOM is provided to a special context rather than to the main module context.

This WIP updates the API to work the E10S-compatible way we've been discussing for Page Mods and other APIs that provide access to content documents: via a separate context that lives alongside the page and has access to its DOM and JS environment. This actually updates Page Worker to have such an API; Panel, which is implemented on top of Page Worker, then inherits it. There's still some work to do: * make the page's "window" object be the prototype for the global object in the separate context, so global variables in the page's window are accessible from the separate context the same way they're accessible from the web page; * provide a postMessage-like mechanism for the separate context and the addon context to communicate.

This WIP adds a postMessage-like API for communication between the main module and the content script.

The WIP makes the postMessage-like message sending API accept a callback via which the recipient of a message can respond to it.

Tests, misc refactoring.

A bunch of bug fixes. In particular, handles the case where a consumer shows a panel immediately after creating it, but the hidden frame isn't ready yet.

This version of the work-in-progress patch factors out the code for providing an E10S-compatible content script API into a separate content-script module that page-worker, panel, and hidden-frame depend upon. (Other modules, like widget, page-mods, and perhaps context-menu, could also use the content-script module to provide an E10S-compatible API, but this patch doesn't address that.) The patch is still a work in progress (the missing pieces are primarily additional tests and documentation), but I think it's about at a point where it could use some feedback. Overall, the patch implements a panel API by refactoring the code for creating a hidden frame out of the page-worker module into a new hidden-frame module. Both the page-worker and panel modules then use the hidden-frame module to create and load content into a frame in the hidden window when an add-on creates a page or panel. Then, when an add-on shows a panel, the code creates a XUL panel with an embedded XUL iframe and swaps the frame loaders of the hidden frame and the panel frame, which causes the panel content to appear in the XUL panel. When the add-on hides the panel, the code swaps the frame loaders again, returning the panel content to the hidden frame, and then destroys the XUL panel. Swapping the frame loaders like this makes it possible for an add-on to pre-load a panel's content and then hold it in reserve until it's time to show it, which is particularly useful for remote content that takes time to load. It also makes it easier to preserve the state of a panel's content across multiple showings of it. However, it might encourage add-ons to preload panels unnecessarily, impacting performance, so it's worth thinking about ways to mitigate that risk (without optimizing prematurely). The patch also implements an E10S-compatible API for accessing the content of both panel and page-worker. Instead of giving add-ons access to the window and document objects of the content, those APIs now solicit a separate "content script" that gets loaded in a separate JS context that has access to the document and window objects as well as a postMessage-like mechanism for communicating with the add-on. Dietrich: does this seem like a reasonable approach overall, and does the refactoring of much of page-worker into hidden-frame make sense? Also, can you see how content-script would be used to provide an E10S-compatible API for widget and other APIs that access content?

This version waits until popuphidden to destroy the panel to fix a bug where it wouldn't disappear on Firefox 3.6 on Mac.

Myk, regarding the 'script' option, as in script: require("self").data.load("handle-reddit-clicks.js"), - you pass a string to the panel/page constructor. For error reporting purposes we'll need to pass the URL down to sandbox.evaluate() as well. In Page Mods I was passing the string parameter to data.load() to the PageMod constructor, which is forces the user to load the scripts from files in data/, which I don't like. Perhaps accept a {contents, filename} object (similar to sandbox.evaluate()) in the API (except 'filename' could be sourceURL instead - bug 577948). The "self" module could be taught to return such an object.

(In reply to comment #21) > - you pass a string to the panel/page constructor. For error reporting purposes > we'll need to pass the URL down to sandbox.evaluate() as well. Hmm, indeed! I actually intended the string to be temporary but forgot to change it. > Perhaps accept a {contents, filename} object (similar to sandbox.evaluate()) in > the API (except 'filename' could be sourceURL instead - bug 577948). The "self" > module could be taught to return such an object. Since the self module already returns URLs to scripts, it seems like it'd be simpler to just accept such URLs as values for the script property and then have the receiving module be responsible for loading them.

Lately I've been trying out using a Mercurial clone to iterate on this patch: https://hg.mozilla.org/users/myk_mozilla.com/jetpack-sdk-panel/

Comment on attachment 456558 [details] [diff] [review] work in progress v8 feedback+, general approach looks good. at first i thought it was awkward that using panels requires three steps (create, add, show), but most of the time it'll happen in the context of another api (eg, widget), so it's probably fine. >+ * >+ * Contributor(s): >+ * Myk Melez <myk@mozilla.org> original author! (here and in your other new files) >+exports.makeContentScript = function makeContentScript(frame, browser) { >+ let contentScript = >+ new (require("securable-module").SandboxFactory)().createSandbox({ >+ principal: Cc["@mozilla.org/systemprincipal;1"].createInstance() >+ }); >+ >+ // Wait until the global object is created before configuring the script. >+ function onGlobalCreated(window) { stylistic comment, ignore at will: these embedded functions are accessing variables declared outside of their scope (declared in the enclosing function's scope), which i find makes debugging difficult, as it's not immediately clear where a variable came from. i'd prefer that they get passed in as parameters to the functions. i'm also a little concerned about how, usually because of caller-scope carefulness, Jetpack modules tend toward mega-functions containing many functions containing many functions. i think that style of coding is difficult to navigate and debug. >+ if (window == browser.contentWindow) { >+ require("observer-service").remove("content-document-global-created", >+ onGlobalCreated); >+ >+ // XXX At this point, we should actually check if the document element >+ // has been created and wait for it to be created if it hasn't. But I >+ // haven't figured out how to do that. I tried listening for DOM mutation >+ // events on the window and document, but my handler was never notified >+ // about them. I have added this to the list of platform dependencies >+ // at https://wiki.mozilla.org/Labs/Jetpack/Dependencies. >+ >+ configureContentScript(); >+ } >+ } >+ require("observer-service").add("content-document-global-created", >+ onGlobalCreated); does this mean there's an observer per content-script per package per addon that uses content scripts? also, i'm not clear on why content scripts are automatically loaded at browser window load. why not let callers control when the scripts are loaded? >+ >+if (!hiddenWindow) { >+ throw new Error([ >+ "The page-worker module needs an app that supports a hidden window. ", >+ "We would like it to support other applications, however. Please see ", >+ "https://bugzilla.mozilla.org/show_bug.cgi?id=546740 for more information." >+ ].join("")); >+} s/page-worker/hidden-frame/ >+ >+exports.Frame = apiUtils.publicConstructor(Frame); i prefer hiddenFrame for clarity, especially if someone requires just that ctor into scope. >+ if (entryPos != -1) >+ frameCache.splice(entryPos, 1); >+ } >+ }; >+ frameCache.push(cacheEntry); >+ require("unload").ensure(cacheEntry); would it be more efficient to unload the whole cache at module unload instead? >+ >+ let cacheEntry = findInCache(frame); >+ >+ if (cacheEntry != -1) { >+ frameCache[cacheEntry].unload(); >+ } should splice it out of the cache as well? >+ >+ function show(anchor) { can you add a comment wherever there's a public api defined? you did this in hidden-frame (i think), and i thought it was a great idea. regarding anchor - i'm not sure it's a good idea to have a high-level api that takes a xul element as an argument. there's a general problem of mixing high and low level apis where old-school conventions (xul dom access, raw events, xpcom) bleed into the new world. i'd like for it to be clearer when those lines are crossed, else we can end up with Jetpacks that are just as fragile and Fx-version-dependent as add-ons are now. maybe the best thing we can do right now is to come up with some general rules of thumb, like: - high level apis don't expose or require xul windows/documents/elements - high level apis don't expose or require xpcom and low level apis can do whatever they need to do, as they're the building blocks for high-level apis. >+ show: show, >+ hide: hide, >+ unload: unload >+ }; >+ cache.push(entry); >+ require("unload").ensure(entry); >+ >+ return panel; >+} ditto about unloading the whole cache at module unload time. >@@ -239,17 +247,17 @@ function BrowserWindow(window) { > > BrowserWindow.prototype = { > > _init: function BW__init() { > // Array of objects: > // { > // widget: widget object, > // node: dom node, >- // eventListeners: hash of event listeners >+ // eventListeners: array of event listeners is this to support multiple handlers per event? if so, try to break changes like this out into a separate bug. much easier to track/review/land smaller patches. >+ // Special case for click events: if the widget has been assigned a panel, >+ // display the panel. >+ function panelClickListener() { >+ if (item.widget.panel) >+ item.widget.panel.show(item.node); >+ } >+ iframe.addEventListener("click", panelClickListener, true, true); >+ item.eventListeners.push(["click", panelClickListener]); this should only occur if the caller hasn't registered click handlers?

(In reply to comment #24) > Comment on attachment 456558 [details] [diff] [review] > work in progress v8 > > feedback+, general approach looks good. at first i thought it was awkward that > using panels requires three steps (create, add, show), but most of the time > it'll happen in the context of another api (eg, widget), so it's probably fine. > > >+ * > >+ * Contributor(s): > >+ * Myk Melez <myk@mozilla.org> > > original author! (here and in your other new files) Blech, ok, done. > >+exports.makeContentScript = function makeContentScript(frame, browser) { > >+ let contentScript = > >+ new (require("securable-module").SandboxFactory)().createSandbox({ > >+ principal: Cc["@mozilla.org/systemprincipal;1"].createInstance() > >+ }); > >+ > >+ // Wait until the global object is created before configuring the script. > >+ function onGlobalCreated(window) { > > stylistic comment, ignore at will: these embedded functions are accessing > variables declared outside of their scope (declared in the enclosing function's > scope), which i find makes debugging difficult, as it's not immediately clear > where a variable came from. i'd prefer that they get passed in as parameters to > the functions. Yup, fixed for configureContentScript, addSendMessageProperty, and validateMessage. Not fixed for onGlobalCreated, though, which needs to close around makeContentScript's scope because it's called by the observer service. > i'm also a little concerned about how, usually because of caller-scope > carefulness, Jetpack modules tend toward mega-functions containing many > functions containing many functions. i think that style of coding is difficult > to navigate and debug. I couldn't agree more. However, I don't see a better option. I was hoping ES5 would provide private property hiding (as ES4 may have), but Irakli recently reviewed the spec and reported that it doesn't. Perhaps our JS engine hackers will spec out such functionality in ECMAScript Harmony and implement it in SpiderMonkey. We should encourage them to do so. > does this mean there's an observer per content-script per package per addon > that uses content scripts? Yes, but observers have very short lifespans. makeContentScript adds the observer, then the consumer initiates the page load, and finally onGlobalCreated removes the observer as soon as it has observed the creation of the window object for the page. For page-mods, we may need to modify this behavior to have the consumer observe the creation of the window object, since page-mods doesn't initiate its own page loads, in which case we will have fewer observers, but some of them will be longer-lived. > also, i'm not clear on why content scripts are automatically loaded at browser > window load. why not let callers control when the scripts are loaded? Loading content scripts when the window is first created satisfies the most use cases. Giving callers this control adds complexity. It might make sense anyway, though. Chrome Extensions does it. I'd love to collect more use cases first to inform the decision, though. > >+if (!hiddenWindow) { > >+ throw new Error([ > >+ "The page-worker module needs an app that supports a hidden window. ", > >+ "We would like it to support other applications, however. Please see ", > >+ "https://bugzilla.mozilla.org/show_bug.cgi?id=546740 for more information." > >+ ].join("")); > >+} > > s/page-worker/hidden-frame/ Fixed! > >+ > >+exports.Frame = apiUtils.publicConstructor(Frame); > > i prefer hiddenFrame for clarity, especially if someone requires just that ctor > into scope. Fixed! > >+ if (entryPos != -1) > >+ frameCache.splice(entryPos, 1); > >+ } > >+ }; > >+ frameCache.push(cacheEntry); > >+ require("unload").ensure(cacheEntry); > > would it be more efficient to unload the whole cache at module unload instead? Probably, although I don't think the difference would be significant in the common case, and I'd rather make that change in a separate patch to minimize the already significant number of changes from the original version of page-worker, especially as cache entry unload functions close around the scope of createPageWorker element to remove their browsers from the hidden window. Also, note that cache unloading requires more than just truncating the array, as DOM elements have to be removed as well. So we still have to iterate array elements, whether we do so ourselves or the unload module does so for us. > >+ let cacheEntry = findInCache(frame); > >+ > >+ if (cacheEntry != -1) { > >+ frameCache[cacheEntry].unload(); > >+ } > > should splice it out of the cache as well? The unload function handles that! > >+ function show(anchor) { > > can you add a comment wherever there's a public api defined? you did this in > hidden-frame (i think), and i thought it was a great idea. Yup, done. (Actually, Felipe is responsible for the comments in hidden-frame, whose code is largely refactored from page-worker.) > regarding anchor - i'm not sure it's a good idea to have a high-level api that > takes a xul element as an argument. there's a general problem of mixing high > and low level apis where old-school conventions (xul dom access, raw events, > xpcom) bleed into the new world. i'd like for it to be clearer when those lines > are crossed, else we can end up with Jetpacks that are just as fragile and > Fx-version-dependent as add-ons are now. The widget module should be able to anchor panels to widgets, for which addons themselves don't need to provide a DOM element to the panel module. But there are use cases for addons to anchor panels to elements in a page, for which addons would need to provide such an element. E10S presumably prevents addons from obtaining references to actual DOM elements in content processes, although, as I understand it, it will be possible to obtain handles to those elements, which addon code could pass to the panel module, which could then convert the handle back to a DOM element and anchor itself to it. > >+ show: show, > >+ hide: hide, > >+ unload: unload > >+ }; > >+ cache.push(entry); > >+ require("unload").ensure(entry); > >+ > >+ return panel; > >+} > > ditto about unloading the whole cache at module unload time. This case seems similar to the one in hidden-frame, and I'd prefer to put off this optimization for similar reasons. > >@@ -239,17 +247,17 @@ function BrowserWindow(window) { > > > > BrowserWindow.prototype = { > > > > _init: function BW__init() { > > // Array of objects: > > // { > > // widget: widget object, > > // node: dom node, > >- // eventListeners: hash of event listeners > >+ // eventListeners: array of event listeners > > is this to support multiple handlers per event? if so, try to break changes > like this out into a separate bug. much easier to track/review/land smaller > patches. Roger that. Filed as bug 579417, patch attached, review requested. But then I cancelled review and wontfixed the bug in favor of a better way to open panels on click events that doesn't require registering an extra click handler. > >+ // Special case for click events: if the widget has been assigned a panel, > >+ // display the panel. > >+ function panelClickListener() { > >+ if (item.widget.panel) > >+ item.widget.panel.show(item.node); > >+ } > >+ iframe.addEventListener("click", panelClickListener, true, true); > >+ item.eventListeners.push(["click", panelClickListener]); > > this should only occur if the caller hasn't registered click handlers? Yes, good point, fixed.

This is still leaking, but I think it's worth getting another review while continuing to work on that issue (which I thought I figured out tonight, only to discover once more that I actually hadn't). In this version, I fixed a variety of issues, including squashing a potential leaker in hidden-frame (the hostFrame DOM element not being removed from the hidden window's DOM when the module is unloaded), improving the widget's panel API (no need to add the panel beforehand, widget now does it for you), and changing some documentation that referred to content symbionts (a low-level term) when it should have been referring to content scripts (the high-level name of the scripts that run alongside content). Drew: I haven't yet incorporated any of your thinking and our recent conversations regarding on e10s-compatible APIs into this work. I'm happy to get both code review here as well as your thoughts on the direction(s) in which this API could and should evolve. Regarding the leaks, note that they are intermittent, and it seems like they could be timing related, since differences in the time it takes for the browser to load (i.e. cold vs. warm load) seem to affect them, although the ways in which they affect them have been inconsistent in my testing. Also, when I am testing only page-worker, hidden-frame, and content-symbiont, I can squash all the leaks simply by changing |handler.call(recipient, message, wrapper)| to |handler(message, wrapper)| inside content-symbiont.js. If I add panel and widget to the list of modules being tested, however, the leaks return even with that change.

Comment on attachment 461159 [details] [diff] [review] work in progress v11 This is righteous Myk, great work. What do you think about breaking it apart into separate bugs or patches, one for panel, hidden-frame, and content-symbiont; one for widget changes; and one for page-worker changes? No big deal, but it might be easier for us (me) to fit smaller chunks in our heads. I've only really looked at the panel parts so far. General comments first: hidden-frame is very cool, but is it necessary for panel? Rather than recreating the XUL panel and iframe on each show() and destroying them on hide(), why not create them on add() and destroy them on remove() (or module unload)? While the XUL panel is hidden, its iframe is a "hidden frame", no? I don't think add() and remove() make sense for panel. We can say that panels are "added" to the browser or its interface, yes. That's true for every Jetpack interface API, but context menu items are naturally added to the context menu or a submenu; widgets are naturally added to the widget bar. What parent item or container are panels added to? I know it's very important to keep the APIs consistent -- but only insofar as it supports some natural behavior of any given API. I have this same critcism of page worker, and for panel I'd recommend what Felipe initially implemented there: the panel is fully constructed in its constructor (everything in add() is moved there), and the client can call a destroy() or unload() method to permanently clean it up if he wants. Small potatoes code comments for now: >diff --git a/packages/jetpack-core/docs/content-symbiont.md b/packages/jetpack-core/docs/content-symbiont.md >+The `content-symbiont` module creates JavaScript contexts that can access web >+content in host application frames and communicate with programs via >+an asynchronous JSON pipe. It is useful in the construction of APIs that are >+compatible with the execution model codenamed "electrolysis" in which programs >+run in separate processes from the processes in which the host application >+loads web content. "in which programs run in separate processes from web content"? >diff --git a/packages/jetpack-core/docs/panel.md b/packages/jetpack-core/docs/panel.md >+<api name="Panel"> >+@constructor >+Creates a panel. >+@param options {object} >+ Options for the panel, with the following keys: >+ @prop width {number} >+ The width of the panel in pixels. >+ @prop height {number} >+ The height of the panel in pixels. Optional param and prop names are set in square brackets: @param [foo] {type} Which renders them in italics. (Since the docs don't yet make it clear that italics imply optionality, I also like to mention that fact in descriptions.) >+ @prop allow {object} >+ Permissions for the panel, with the following keys: >+ @prop script {boolean} >+ Whether or not to execute script in the content. Defaults to false. Why default to false? If you stick a Google Map or some other script-heavy (i.e., modern) page in a panel, it should just work. >+ @prop scriptURL {string} {array} >+ The URLs of content scripts to load. >+ @prop script {string} {array} >+ The text of content scripts to load. Should mention how these two relate/interact (or their lack of interaction). Also, there's currently no good way to specify multiple types of params and props (bug 570850) except: @prop foo {string,array} >+ @prop scriptWhen {string} >+ When to load the content scripts. >+ Possible values are "start" (default), which loads them as soon as >+ the window object for the page has been created, and "ready", which loads >+ them once the DOM content of the page has been loaded. Hmm, my first thought was modifying the DOM is probably the common case, so "ready" should be default. But it's also nice that "start" means your script behaves like a "normal" script, like one included in the page. Yeah, I guess that's better. >+@returns {Panel} the new panel We haven't been marking constructors as returning anything, since that's implied, but maybe we should. >diff --git a/packages/jetpack-core/lib/panel.js b/packages/jetpack-core/lib/panel.js >+if (!require("xul-app").is("Firefox")) { >+ throw new Error([ >+ "The panel module currently supports only Firefox. In the future ", >+ "we would like it to support other applications, however. Please see ", >+ "https://bugzilla.mozilla.org/show_bug.cgi?id=jetpack-panel-apps ", I presume this bug ID will be valid by the time the version of the SDK that includes panel is released. >+exports.Panel = apiUtils.publicConstructor(Panel); >+ >+function Panel(options) { >+ options = options || {}; >+ let self = this; >+ >+ contentSymbionts.mixInto(this, options); >+ >+ // We define these independently of the main validateOptions call so we can >+ // reuse them when checking values passed to the setters after construction. >+ let widthReq = { >+ is: ["number", "undefined"], >+ msg: "The width option must be a number." When you define `is` you get a nice message by default. ("The option 'foo' must be one of the following types: ..." But if you want to keep all these error messages consistent, including ones that define further requirements, that's cool too.) >+let cache = []; Nit: This could be more descriptive of the entries it contains, although I'm not sure of a better name than panelCache, which is obvious. >+function getWindow(anchor) { Can you use the technique described here to get the top chrome window? I've used it before in similar situations: https://developer.mozilla.org/en/Working_with_windows_in_chrome_code#Accessing_the_elements_of_the_top-level_document_from_a_child_window

Myk, do you have any comments on our e10s emails or the first several paragraphs in comment 28? I'm finding it hard to move on with review (and context-menu) without them.

(In reply to comment #28) > What do you think about breaking it apart into separate bugs or patches, one > for panel, hidden-frame, and content-symbiont; one for widget changes; and one > for page-worker changes? No big deal, but it might be easier for us (me) to > fit smaller chunks in our heads. That's a great idea! There are many interdependencies, but the patch could still be broken up into at least those pieces and possibly further. I'll work on that (and thus am not attaching another patch to this bug) right after I review our previous conversations and your latest thoughts on content scripts in the wiki. > I've only really looked at the panel parts so far. General comments first: > > hidden-frame is very cool, but is it necessary for panel? Rather than > recreating the XUL panel and iframe on each show() and destroying them on > hide(), why not create them on add() and destroy them on remove() (or module > unload)? While the XUL panel is hidden, its iframe is a "hidden frame", no? XUL panels are specific to windows, while Jetpack panels aren't (one defines a single panel which can then get shown in any window). The module could create the XUL panel on add() in some window and then recreate it if the panel is opened in another one (or if the original window is closed), but that seems more complicated than caching the content in the hidden window, which is always present. > I don't think add() and remove() make sense for panel. We can say that panels > are "added" to the browser or its interface, yes. That's true for every > Jetpack interface API, but context menu items are naturally added to the > context menu or a submenu; widgets are naturally added to the widget bar. What > parent item or container are panels added to? I know it's very important to > keep the APIs consistent -- but only insofar as it supports some natural > behavior of any given API. > > I have this same critcism of page worker, and for panel I'd recommend what > Felipe initially implemented there: the panel is fully constructed in its > constructor (everything in add() is moved there), and the client can call a > destroy() or unload() method to permanently clean it up if he wants. If I recall correctly, the primary reason we required both page workers and context menu items to be added in addition to being constructed is that doing so conforms to JavaScript programmers' mental model of constructed objects being GCed when they go out of scope (or immediately, if they are not assigned to a symbol) unless a persistent reference to them is explicitly declared. add() was our way of declaring that persistent reference. One might also see this as analogous to the createElement/appendChild two step procedure for adding elements to the DOM. I'm not particularly keen to model our API after the DOM (nor to make any of these APIs more complicated than they need to be), but this procedure does clarify the difference between merely creating an element, which doesn't cause it to persist when the scope is exited, and adding it to the document, which does. > >diff --git a/packages/jetpack-core/docs/content-symbiont.md b/packages/jetpack-core/docs/content-symbiont.md > > >+The `content-symbiont` module creates JavaScript contexts that can access web > >+content in host application frames and communicate with programs via > >+an asynchronous JSON pipe. It is useful in the construction of APIs that are > >+compatible with the execution model codenamed "electrolysis" in which programs > >+run in separate processes from the processes in which the host application > >+loads web content. > > "in which programs run in separate processes from web content"? So much better! > >diff --git a/packages/jetpack-core/docs/panel.md b/packages/jetpack-core/docs/panel.md > Optional param and prop names are set in square brackets: > > @param [foo] {type} > > Which renders them in italics. (Since the docs don't yet make it clear that > italics imply optionality, I also like to mention that fact in descriptions.) Deal! I've set the names in square brackets and mentioned their optionality in descriptions. > >+ @prop allow {object} > >+ Permissions for the panel, with the following keys: > >+ @prop script {boolean} > >+ Whether or not to execute script in the content. Defaults to false. > > Why default to false? If you stick a Google Map or some other script-heavy > (i.e., modern) page in a panel, it should just work. Brian suggested this default for Page Worker and other content-loading APIs, even though the common case is to allow script, to ensure developers are aware of the presence of script and the risk of interacting with content for which script is enabled. > >+ @prop scriptURL {string} {array} > >+ The URLs of content scripts to load. > >+ @prop script {string} {array} > >+ The text of content scripts to load. > > Should mention how these two relate/interact (or their lack of interaction). Good point! I've noted in both descriptions the order in which the content scripts are loaded. > Also, there's currently no good way to specify multiple types of params and > props (bug 570850) except: > > @prop foo {string,array} Ah, cool, I've updated the docs to specify multiple types this way. Also, I made these changes in all the doc files added by this patch where I found similar antipatterns. > >+ @prop scriptWhen {string} > >+ When to load the content scripts. > >+ Possible values are "start" (default), which loads them as soon as > >+ the window object for the page has been created, and "ready", which loads > >+ them once the DOM content of the page has been loaded. > > Hmm, my first thought was modifying the DOM is probably the common case, so > "ready" should be default. But it's also nice that "start" means your script > behaves like a "normal" script, like one included in the page. Yeah, I guess > that's better. Originally, I didn't want to provide this option at all in the first pass. My plan was to always load content scripts on "start" (and perhaps later add this option with a "ready" possible value after more research on its use cases, usability, and performance implications). But "start" currently happens before the document element is created, so it can't be used to load libraries like jQuery that depend on that element's existence. And since loading libraries like jQuery to modify the DOM is a very common case, we need to support it; hence this option and its "ready" possible value. Once Jonas fixes bug 579764, however, "start" will happen right before <script> elements in the <head> element are loaded, so it'll satisfy both the DOM manipulation use case and the use case of adding a web platform API (f.e. geolocation or camera access) to web pages before their script is loaded. Thus it seems like the most flexible default in the long term. > >+@returns {Panel} the new panel > > We haven't been marking constructors as returning anything, since that's > implied, but maybe we should. Hmm, good catch, we should be able to assume developers understand what a constructor returns (unless for some reason it returns a different type of object than the one implied by the constructor, which isn't currently the case for any of our constructors AFAIK). > >diff --git a/packages/jetpack-core/lib/panel.js b/packages/jetpack-core/lib/panel.js > >+if (!require("xul-app").is("Firefox")) { > >+ throw new Error([ > >+ "The panel module currently supports only Firefox. In the future ", > >+ "we would like it to support other applications, however. Please see ", > >+ "https://bugzilla.mozilla.org/show_bug.cgi?id=jetpack-panel-apps ", > > I presume this bug ID will be valid by the time the version of the SDK that > includes panel is released. Yes! I'm making it this bug right now, but using an alias means we can switch it to another bug if we file one for another app (or a meta bug to track individual bugs for each app). > >+ let widthReq = { > >+ is: ["number", "undefined"], > >+ msg: "The width option must be a number." > > When you define `is` you get a nice message by default. ("The option 'foo' > must be one of the following types: ..." But if you want to keep all these > error messages consistent, including ones that define further requirements, > that's cool too.) Nope, the default message is fine, I just didn't realize there was one. I've removed the custom messages from the items whose custom messages don't provide any additional information. > >+let cache = []; > > Nit: This could be more descriptive of the entries it contains, although I'm > not sure of a better name than panelCache, which is obvious. Yeah, I figured it would be obvious enough (after learning it once, anyway) given that there is only a single cache in the module and the cache is used regularly. > >+function getWindow(anchor) { > > Can you use the technique described here to get the top chrome window? I've > used it before in similar situations: > > https://developer.mozilla.org/en/Working_with_windows_in_chrome_code#Accessing_the_elements_of_the_top-level_document_from_a_child_window It looks like that's useful for code running in a subwindow that wants to access its parent window, but in this case the code is running outside of any window, and the anchor element could be in any window (or subwindow), so it's not clear how it would be helpful.

(In reply to comment #30) > XUL panels are specific to windows, while Jetpack panels aren't (one defines a > single panel which can then get shown in any window). Ah, OK. > If I recall correctly, the primary reason we required both page workers and > context menu items to be added in addition to being constructed is that doing > so conforms to JavaScript programmers' mental model of constructed objects > being GCed when they go out of scope (or immediately, if they are not assigned > to a symbol) unless a persistent reference to them is explicitly declared. > add() was our way of declaring that persistent reference. I didn't realize that was the justification. The reason I argued for add() in context-menu was that constructors shouldn't have side effects external to the object being constructed. Making a new Item shouldn't also insert it into the menu. I wanted to call the separate insertion function "add" because menu items are naturally added to menus. So now I have two problems with add and remove: they're implementation details that leak through to the API, and their names don't lend an understanding of their purpose to the client. The fact that we keep a hidden iframe around in some window and it won't go away when the panel JS object does is an implementation detail that we shouldn't force the client to stumble over every time he wants to use a new panel. "add" makes sense only if you understand the implementation. It co-opts the terminology of APIs that really do have parent-child relationships like context-menu and widget. In documentation, you end up having to explain weird things like add()ing panels to the interface and then show()ing them to *really* add them to the interface. > One might also see this as analogous to the createElement/appendChild two step > procedure for adding elements to the DOM. Again, in the DOM case there is a clear parent-child relationship to the client, as there also is in context-menu and widget. Not so here, unless you understand the implementation. > > >+ @prop allow {object} > > >+ Permissions for the panel, with the following keys: > > >+ @prop script {boolean} > > >+ Whether or not to execute script in the content. Defaults to false. > > > > Why default to false? If you stick a Google Map or some other script-heavy > > (i.e., modern) page in a panel, it should just work. > > Brian suggested this default for Page Worker and other content-loading APIs, > even though the common case is to allow script, to ensure developers are aware > of the presence of script and the risk of interacting with content for which > script is enabled. That's a noble goal I guess, but I doubt most people, especially the casual developers Jetpack targets, will pause and reflect on why script is disabled by default. They'll shake their fists when the map in their panel doesn't look right, google or post about the problem, add the one line fix, and move on. > > >+function getWindow(anchor) { > > > > Can you use the technique described here to get the top chrome window? I've > > used it before in similar situations: > > > > https://developer.mozilla.org/en/Working_with_windows_in_chrome_code#Accessing_the_elements_of_the_top-level_document_from_a_child_window > > It looks like that's useful for code running in a subwindow that wants to > access its parent window, but in this case the code is running outside of any > window, and the anchor element could be in any window (or subwindow), so it's > not clear how it would be helpful. The doc is incorrect on that point; you can start at a handle to any arbitrary content window, not just a (sub)window that your code is running inside. Here's the example I was looking for earlier, from the menu code of the Prototype: http://mxr.mozilla.org/labs-central/source/jetpack/extension/modules/menu.js#1395 (That particular function returns the browser.xul doc, not the window.)

(In reply to https://wiki.mozilla.org/User:Adw/e10s-context-menu) I agree that writing code in strings sucks, especially since JavaScript doesn't support here documents (and despite the variety of interesting workarounds that folks have cooked up over the years). However, it feels like there are enough valid use cases for both inline script and script files (including using the two in conjunction) to justify supporting both, even at the cost of additional complexity. Nevertheless, the way we have supported both inline content and content URLs via a single "content" property seems problematic. So Brian and I brainstormed on alternative approaches, and it seemed like the least bad one would be to have two properties, one for file URLs and one for script text, so that's what I have implemented here. In terms of putting content scripts into separate files, I agree with the goal of minimizing the number of files people have to make. It certainly shouldn't be necessary to have one file for specifying the a context item's context and another for specifying its click handler. I'm not so sure about sharing content scripts between APIs, though (f.e. having one content script to implement both a context menu item and a page mod). I wouldn't arbitrarily prevent that, but I'm not sure it wins us enough to encourage that approach and explicitly support it. I'm open to the idea, though, and API modifications that enable it. It looks like your suggestion would be to load the content script in a CommonJS environment. I actually proposed that a few months ago, thinking that it would simplify the development of basic addons that could do all work in a content script rather than having to do some work in the main program and communicate between the two. But bsmedberg was against it, presumably because it undermines the primary motivation for running main programs in separate processes in the first place, which is to move most addon processing to a separate process. We could limit the APIs that "require" could access when used in a content script to those content scripts need, but that seems like it'd be confusing to developers expecting a regular CommonJS environment. Another approach would be to use the "exports" side of the CommonJS module spec rather than the "require" side, i.e. something like this: // content script exports.contextMenu.Item({ id: "remove-image", context: "img", onClick: function (contextObj) { return contextObj.node.src; } }); Folks might be less inclined to think they can require modules in that case, not sure. This feels like it needs more thought. Finally, regarding naming of the content script-related properties, I've been using "script", "scriptURL", and "scriptWhen" in panel patches, but I noticed your examples use contentScript instead of "script". contentScript seems a bit better, since it's more explicit (although it's also longer). Adopting this naming scheme would mean the other content script properties would also get a "content" prefix, i.e. contentScriptURL and contentScriptWhen. (In reply to comment #31) > So now I have two problems with add and remove: they're implementation details > that leak through to the API, and their names don't lend an understanding of > their purpose to the client. > > The fact that we keep a hidden iframe around in some window and it won't go > away when the panel JS object does is an implementation detail that we > shouldn't force the client to stumble over every time he wants to use a new > panel. > > "add" makes sense only if you understand the implementation. It co-opts the > terminology of APIs that really do have parent-child relationships like > context-menu and widget. In documentation, you end up having to explain weird > things like add()ing panels to the interface and then show()ing them to > *really* add them to the interface. That's not quite right, since showing a panel is fundamentally different from adding a panel. It is about changing the visibility of a panel that has already been added to the interface, and it is something that potentially happens many times over the life of a panel. The Panel API's show() is more like the Context Menu API's Item.context, in the sense that it controls visibility (although imperatively rather than declaratively), than either API's add(). I do see your point about the leaky abstraction, however, although I think it's a relatively minor problem. Nevertheless, I'm open to ideas for addressing it. I don't think we can do so by giving construction the side-effect of persistence. But one mitigation (not quite a solution) would be to make pre-construction optional for simple cases by allowing add() to accept property bags of options: var panels = require("panel"); var panel = panels.add({ ... }); var widgets = require("widget"); widgets.add({ label: "My Widget", panel: { ... } }); In these cases, the APIs would construct the objects themselves using the specified options and return the constructed objects. For APIs like Context Menu, which lets you add multiple types of objects to the menu, raw options would construct the most likely type of object (a menu item), and constructors would still be needed for others (menus and separators): var contextMenu = require("context-menu"); contextMenu.add({ label: "My Menu Item", context: "img", onClick: function() { ... } }); var menu = contextMenu.add(contextMenu.Menu({ ... }));

Comment on attachment 461159 [details] [diff] [review] work in progress v11 (In reply to comment #32) > That's not quite right, since showing a panel is fundamentally different from > adding a panel. Sure, but how is the client supposed to know that, and why should he? To him, he's "adding the panel to the interface" twice. > It is about changing the visibility of a panel that has already been added to > the interface, and it is something that potentially happens many times over > the life of a panel. ... and how many times does add get called over the life of the panel? Why would anyone but the most pedantic of clients who understand the implementation behind add and remove ever call add more than once, or anytime other than immediately after construction? > I don't think we can do so by giving construction the side-effect of > persistence. But one mitigation (not quite a solution) would be to make > pre-construction optional for simple cases by allowing add() to accept property > bags of options: > > var panels = require("panel"); > var panel = panels.add({ ... }); > > var widgets = require("widget"); > widgets.add({ > label: "My Widget", > panel: { ... } > }); That sounds great, except replace "add" with "Panel" and "Widget". I can't r+ this patch, so someone else should review.

(In reply to comment #33) > (In reply to comment #32) > > showing a panel is fundamentally different from adding a panel. > > Sure, but how is the client supposed to know that, and why should he? To him, > he's "adding the panel to the interface" twice. My presumption is that developers will learn the difference between add() and show() when they read the documentation, which should explain the purpose of these two functions, or see a code example that calls show() each time a panel is to be shown while calling add() just once. But in any case, if I understand correctly, I think your issue is not that there is both add() and show() but rather that there is both a constructor and add(). > > It is about changing the visibility of a panel that has already been added to > > the interface, and it is something that potentially happens many times over > > the life of a panel. > > ... and how many times does add get called over the life of the panel? Why > would anyone but the most pedantic of clients who understand the implementation > behind add and remove ever call add more than once, or anytime other than > immediately after construction? They wouldn't, and I understand how that makes it feel like construction and add() are redundant steps. > > var panels = require("panel"); > > var panel = panels.add({ ... }); > > > > var widgets = require("widget"); > > widgets.add({ > > label: "My Widget", > > panel: { ... } > > }); > > That sounds great, except replace "add" with "Panel" and "Widget". I definitely do see your point. And providing such objects with a destroy() method (in lieu of remove()) is certainly a reasonable alternative that I have previously considered (and which jQuery UI uses, albeit in a somewhat different context). My concern is just that doing that would break the mental model of JavaScript developers that object construction doesn't have side-effects, and objects are garbage collected when the last references to them go out of scope. Do you not share this concern, or is it just that you think avoiding the appearance of redundancy and leakiness of the abstraction is more important? > I can't r+ this patch, so someone else should review. *sigh*, ok. Here's a version of the patch that only includes the panel, hidden-frame, and content-symbiont changes. The rest of the all-in-one patch has been split up into patches on different bugs. There is still intermittent leakage happening, but, with a heavy heart, I am increasingly of the opinion that we shouldn't block changes on leak bustage, given our uncertainty about the reliability of our leak detection and reporting infrastructure.

> My concern is just that doing that would break the mental model of JavaScript > developers that object construction doesn't have side-effects, In this case the side effect both pertains to the object being constructed and is an implementation detail invisible and irrelevant to the client. > and objects are garbage collected when the last references to them go out of > scope. That's still true. I don't think that clients are guaranteed that their references to new objects are the only references to those objects, and I'm not sure why such a guarantee is important.

Comment on attachment 463875 [details] [diff] [review] patch v1: panel, hidden-frame, and content-symbiont >+++ b/examples/reddit-panel/data/panel.js >@@ -0,0 +1,17 @@ >+$(window).click(function (event) { >+ var t = event.target; >+ >+ // Don't intercept the click if it isn't on a link. >+ if (t.nodeName != "A") >+ return; >+ >+ // Don't intercept the click if it was on one of the links in the header >+ // or next/previous footer, since those links should load in the panel itself. >+ if ($(t).parents('#header').length || $(t).parents('.nextprev').length) >+ return; >+ >+ // Intercept the click, passing it to the addon, which will load it in a tab. >+ event.stopPropagation(); >+ event.preventDefault(); >+ program.sendMessage(t.toString()); >+}); Should add a comment at the top saying something like "this is a content script. it executes in the context of the loaded Reddit page, and is injected there via the Panel module. see lib/main.js for details." >+++ b/packages/jetpack-core/docs/content-symbiont.md >@@ -0,0 +1,82 @@ >+The `content-symbiont` module creates JavaScript contexts that can access web >+content in host application frames and communicate with programs via >+an asynchronous JSON pipe. It is useful in the construction of APIs that are >+compatible with the execution model codenamed "electrolysis" in which programs >+run in separate processes from web content. >+ >+This module is not intended to be used directly by programs. Rather, it is >+intended to be used by other modules that provide APIs to programs. Need code examples of both APIs in this module, as well as hidden-frame and panel. >+<api name="remove"> >+@function >+Unregister a hidden frame, unloading any content that was loaded in it. >+@param hiddenFrame {HiddenFrame} the frame to remove >+</api> How do onBeforeUnload prompts behave in the hidden window? do they show up? or are they suppressed? >+The `panel` module creates floating modal "popup dialogs" that appear on top of >+web content and browser chrome and persist until dismissed by users or programs. >+Panels are useful for presenting temporary interfaces to users in a way that is >+easier for users to ignore and dismiss than a modal dialog, since panels are >+hidden the moment users interact with parts of the application interface outside >+them. >+ >+Introduction >+------------ >+ >+The module exports a constructor function, `Panel`, and two other functions, >+`add` and `remove`. >+ >+`Panel` constructs a new panel. `add` adds a panel to the interface of the host >+application. `remove` removes a panel from the interface of the host >+application. Note that `add` doesn't show a panel. To show a panel, call its >+`show` method after adding it. Per the previous conversation with Drew, you probably shouldn't use "to the interface", since it actually doesn't. More on this problem below. >+Panels are created, and their content is loaded, as soon as they are added, >+and they continue to exist after they have been hidden, so it is possible >+to leave a panel around in the background, updating its content as needed >+until the next time it is shown. Are we sure consumers actually *want* the content of panels to persist for the lifetime of the application, post add()? Going back to your first paragraph in the doc, there's a distinct emphasis on these being temporary interfaces. If I saw web content in there, from both a Jetpack developer and a user's perspective, I'm not sure I'd expect that content to stay live, and definitely wouldn't have expected it to have been live before I even opened the panel!). I'd expect it to act like a tab - loaded when the panel loads, and destroyed when the panel is destroyed. This approach would resolve the leaky-abstraction problem - could look something like: let panels = require("panel"); let myPanel = panels.Panel({}); // Content is *not* loaded yet. panel.show({}); // Content is loaded now. and content is unloaded when the panel is dismissed. >+Panels can be anchored to a particular DOM element in a DOM window (including >+both chrome elements, i.e. parts of the host application interface, and content >+elements, i.e. parts of a web page in an application tab). Panels that are >+anchored to an element should have a "tail" that points from the panel to the >+element, but that has not yet been implemented. The work to implement it is >+tracked in [bug 554937](https://bugzilla.mozilla.org/show_bug.cgi?id=554937). The first bit in parentheses doesn't need to be, I think. And s/tail/arrow/. I'm still not comfortable with the DOM node requirement here. Is that even possible when Jetpack module code is running in a separate process? >+Panels have associated content scripts, which are JavaScript scripts that have >+access to the content loaded into the panels. A program can specify scripts >+to load for a panel, and the program can communicate with those scripts over >+an asynchronous JSON pipe. "pipe" sounds old skool Unix-y to me. Maybe change it to something more akin to what webdevs talk about? Maybe "API" or just "via JSON."

(In reply to comment #36) > Should add a comment at the top saying something like "this is a content > script. it executes in the context of the loaded Reddit page, and is injected > there via the Panel module. see lib/main.js for details." Good point. I added a comment to this effect. > >+++ b/packages/jetpack-core/docs/content-symbiont.md > >+The `content-symbiont` module creates JavaScript contexts that can access web > >+content in host application frames and communicate with programs via > >+an asynchronous JSON pipe. It is useful in the construction of APIs that are > >+compatible with the execution model codenamed "electrolysis" in which programs > >+run in separate processes from web content. > >+ > >+This module is not intended to be used directly by programs. Rather, it is > >+intended to be used by other modules that provide APIs to programs. > > Need code examples of both APIs in this module, as well as hidden-frame and > panel. Yup, added for all three modules. > >+<api name="remove"> > >+@function > >+Unregister a hidden frame, unloading any content that was loaded in it. > >+@param hiddenFrame {HiddenFrame} the frame to remove > >+</api> > > How do onBeforeUnload prompts behave in the hidden window? do they show up? or > are they suppressed? They are suppressed (as are alert() and prompt() dialogs). > >+`Panel` constructs a new panel. `add` adds a panel to the interface of the host > >+application. `remove` removes a panel from the interface of the host > >+application. Note that `add` doesn't show a panel. To show a panel, call its > >+`show` method after adding it. > > Per the previous conversation with Drew, you probably shouldn't use "to the > interface", since it actually doesn't. More on this problem below. Ok, I have updated this language to use the more neutral language in the hidden-frame documentation. > Are we sure consumers actually *want* the content of panels to persist for the > lifetime of the application, post add()? Going back to your first paragraph in > the doc, there's a distinct emphasis on these being temporary interfaces. If I > saw web content in there, from both a Jetpack developer and a user's > perspective, I'm not sure I'd expect that content to stay live, and definitely > wouldn't have expected it to have been live before I even opened the panel!). > I'd expect it to act like a tab - loaded when the panel loads, and destroyed > when the panel is destroyed. My sense is that panels are different from tabs (and sidebars), in that they are more chrome-like rather than content-like (even though implemented as web content iframes), where "chrome-like" means, among other things, that they are immediately available for interaction (like the "larry" and "edit bookmark" popups), and not something that loads only once you activate it (like a tab or sidebar). Also, for addons that expose updates from Facebook, Reddit, Twitter, webmail, and other lifestreaming/news gathering/update generating services, it is useful to have a persistent frame for a panel into which updates are periodically pushed about which ambient interfaces (notifications, widgets) can then notify users, such that when the user activates the panel, it already contains those updates. It's possible that I'm wrong though, or that this model has undesirable side-effects (like performance implications from the presence of persistent background frames). But I think it's worth a try. And note that it's possible to use this model to implement non-persistent panels that only load after being shown, if a developer prefers one of those. The addon just needs to add the panel right before showing it and remove it right after it is hidden. I have added an onHide callback property to this version of the patch to make that easier. > This approach would resolve the leaky-abstraction problem - could look > something like: According to the Law of Leaky Abstractions <http://www.joelonsoftware.com/articles/LeakyAbstractions.html>, "all non-trivial abstractions, to some degree, are leaky"! Nevertheless, that doesn't mean this one should leak unnecessarily. The problem is just that there are competing concerns to balance. I have asked John Resig for his thoughts in case he has any insight from his years of building JavaScript interfaces for web developers. He's on vacation, though, and may not respond for some time. In the meantime, I think we should move forward with the current interface, but the door is very much open to improving this interface based not only on Drew's suggestions and John's thoughts but also observations of developer behavior and feedback from developers using the interface. > >+Panels can be anchored to a particular DOM element in a DOM window (including > >+both chrome elements, i.e. parts of the host application interface, and content > >+elements, i.e. parts of a web page in an application tab). Panels that are > >+anchored to an element should have a "tail" that points from the panel to the > >+element, but that has not yet been implemented. The work to implement it is > >+tracked in [bug 554937](https://bugzilla.mozilla.org/show_bug.cgi?id=554937). > > The first bit in parentheses doesn't need to be, I think. Good call, removed. > And s/tail/arrow/. Yeah, I know that's what the XUL panel implementation is calling them, I just don't understand why, since they seem to be modeled after speech bubbles <http://en.wikipedia.org/wiki/Speech_balloon#Speech_bubbles>, which have tails, not arrows. But ok, wording "fixed". > I'm still not comfortable with the DOM node requirement here. Is that even > possible when Jetpack module code is running in a separate process? Yes. As I understand it, the way it will work is that the code that identifies the anchor node, which will be running in the chrome/content process, will pass a handle <https://wiki.mozilla.org/Electrolysis/Jetpack#Messages_and_Handles> representing the anchor node to the code that maintains a reference to the panel object, which will be running in the addon process. That code will then call panel.show(), passing it the handle, and the part of the panel implementation running in the addon process will pass the handle to the part of the panel implementation that actually shows panels, which will be running in the chrome/content process. That code will then show the panel, anchoring it to the node to which the handle refers. > "pipe" sounds old skool Unix-y to me. Well I did not think the nit could be so cruel And I'm never going back to my old skool > Maybe change it to something more akin to > what webdevs talk about? Maybe "API" or just "via JSON." Yup, good point. I left the "pipe" terminology in the content-symbiont docs, since that is a low-level API intended to be used by other API developers, but I updated it in the panel docs, which are intended to be used by addon developers, to describe this as an "asynchronous message passing API". Here are responses to the review comments in bug 585390, comment 1 for the widget changes (which I pulled back into this patch, since the reddit-panel example depends on them): (In reply to bug 585390, comment #1) > >+ Note: if you also specify an `onClick` callback function, it will be > >+ called instead of the panel being opened. > >+ </td> > >+ </tr> > > maybe add a note explaining that it's still possible to open the panel manually > if you have a custom click handler. Yup, good point, I have expanded the existing note to include this information. > >+ panel: { > >+ is: ["null", "undefined", "object"], > >+ }, > > should check that it's actually a panel object. Yup, done. > > addItem: function browserManager_addItem(item) { > > let idx = this.items.indexOf(item); > > if (idx > -1) > > throw new Error("The widget " + item + " has already been added."); > > this.items.push(item); > > this.windows.forEach(function (w) w.addItems([item])); > >+ if (item.panel) > >+ panels.add(item.panel); > > will the panel module add panels to each open window? if so, this could be done > just once, in the widget ctor. No, it adds a single panel to the hidden window, then it moves it to a particular window when the panel is shown. The reason for doing adding the panel here is that this is the function that gets called when the widget is added, and I wanted to align the creation of DOM elements for the widget (which takes place when the widget is added) with the creation of DOM elements for the panel (which takes place when the panel is added), as that seems less prone to violate assumptions about the effects these two APIs will have on the browser DOM and when they will have those effects. > should add a test for when there's both a panel and a click handler defined. Yup, added. Other changes: * added attribution to the new documentation files per the convention for it; * enabled script in content frames by default; (In conversations with Brian, we agreed that it would make sense for allow.script to default to true, given that it is what web developers expect and would want in the vast majority of cases.) * removed some unused, commented-out code in hidden-frame.js; * renamed the "frame" property of HiddenFrame objects to "element" in order to better describe it and eliminate confusing constructs like frame.frame and hiddenFrame.frame; * made ContentSymbiont instantiators specify the name of the global object that is injected into the content symbiont context for communication with the instantiator, and made the panel and page-worker modules name their objects "panel" and "pageWorker", respectively, rather than "program", in order to make those names more descriptive and accurate, and also as a step towards potentially making it possible for multiple objects to share a content script; * fixed a timing-sensitive bug that would occasionally trigger an exception when a panel was removed while it was in the process of being shown; * renamed script, scriptURL, and scriptWhen to contentScript, contentScriptURL, and contentScriptWhen to clarify their functions; * made misc minor formatting changes for clarity and readability.

Comment on attachment 465583 [details] [diff] [review] patch v2: addresses review comments, other issues >+<api name="add"> >+@function >+Add a panel to the interface of the host application. >+@param panel {Panel} the panel to add >+</api> nit: s/the interface of// >+ >+<api name="remove"> >+@function >+Remove a panel from the interface of the host application. >+@param panel {Panel} the panel to remove >+</api> ditto >+ options.globalName = apiUtils.validateOptions( >+ { globalName: options.globalName }, >+ { >+ globalName: { >+ is: ["string"], >+ // The value cannot be an empty string, >+ // since we name a property with it. >+ ok: function(v) v != "" >+ } >+ } >+ ).globalName; accidentally left the first one there? >+ function configureSandbox() { can you pass the sandbox to this, instead of modifying vars from outside the function scope? IMO the locals-only approach is easier to read, and less bug-prone. >+ // XXX I think the principal should be self.frame.contentWindow, but script >+ // doesn't work correctly when I set it to that value. Events don't get >+ // registered; even dump() fails. >+ // >+ // FIXME: figure out the problem and resolve it, so we can restrict >+ // the sandbox to the same set of privileges the page has (plus any >+ // others it gets to access through the object that created it). >+ // >+ // XXX when testing self.frame.contentWindow, I found that setting >+ // the principal to its self.frame.contentWindow.wrappedJSObject resolved >+ // some test leaks; that was before I started clearing the principal >+ // of the sandbox on unload, though, so perhaps it is no longer a problem. file followup bugs for these? >+ // >+ //principal: self.frame.contentWindow.wrappedJSObject remove >+ // Chain the global object for the sandbox to the global object for >+ // the frame. This supports JavaScript libraries like jQuery that depend >+ // on the presence of certain properties in the global object, like window, >+ // document, location, and navigator. >+ sandbox.__proto__ = self.frame.contentWindow.wrappedJSObject; >+ >+ // Alternate approach: >+ // Define each individual global on which JavaScript libraries depend >+ // in the global object of the sandbox. This is hard to get right, >+ // since it requires a priori knowledge of the libraries developers use, >+ // and exceptions in those libraries aren't always reported. It's also >+ // brittle, prone to breaking when those libraries change. But it might >+ // make it easier to avoid namespace conflicts. yeah, that sounds like it'll cause no end of problems. let's stick with the former. >+ for each (let contentScriptURL in self.contentScriptURL) { >+ let filename = url.toFilename(contentScriptURL); >+ errors.catchAndLog( >+ function () Cu.evalInSandbox(file.read(filename), sandbox, "1.8", >+ filename, 1) >+ )(); >+ } >+ for each (let contentScript in self.contentScript) { >+ errors.catchAndLog( >+ function () Cu.evalInSandbox(contentScript, sandbox, "1.8", '<string>', >+ 1) >+ )(); >+ } >+ } can you put the js version in a global const so we an easily find and change in the future? >+ // We define these independently of the main validateOptions call so we can >+ // reuse them when checking values passed to the setters after construction. >+ let widthReq = { >+ is: ["number", "undefined"] >+ }; super useful. we should add a whole bit on the "implementing a module" wiki page about how to use validateOptions. >+ // Validate panel-specific options without filtering those validated by >+ // contentSymbionts.mixInto. >+ for each (let [key, val] in Iterator(apiUtils.validateOptions(options, >+ { width: widthReq, height: heightReq, >+ content: contentReq, onShow: onShowReq, >+ onHide: onHideReq }))) { >+ if (typeof(val) != "undefined") >+ options[key] = val; >+ }; that's awkward. not sure what to do about it though. >+ // XXX Sometimes I get "TypeError: xulPanel.hidePopup is not a function" >+ // when quitting the host application while a panel is visible. To suppress >+ // them, this now checks for "hidePopup" in xulPanel before calling it. >+ // It's not clear if there's an actual issue or the error is just normal. >+ if (xulPanel && "hidePopup" in xulPanel) >+ xulPanel.hidePopup(); >+ } file followup r=me!

(In reply to comment #38) > >+<api name="add"> > >+@function > >+Add a panel to the interface of the host application. > >+@param panel {Panel} the panel to add > >+</api> > > nit: s/the interface of// > > >+ > >+<api name="remove"> > >+@function > >+Remove a panel from the interface of the host application. > >+@param panel {Panel} the panel to remove > >+</api> > > ditto Both fixed by replacing the descriptions with those being used in the Introduction section. > >+ options.globalName = apiUtils.validateOptions( > >+ { globalName: options.globalName }, > >+ { > >+ globalName: { > >+ is: ["string"], > >+ // The value cannot be an empty string, > >+ // since we name a property with it. > >+ ok: function(v) v != "" > >+ } > >+ } > >+ ).globalName; > > accidentally left the first one there? This is actually correct, if unwieldy. The first argument to validateOptions is the configuration options object to validate, the second is the requirements object that specifies how to validate the first parameter. And the return value is a new object that contains only validated options. However, I just realized that the first argument can just be the |options| variable; it doesn't have to be a new object constructed from that variable. I had been under the impression that the argument to that parameter would get modified, and a new object was needed to protect other properties in |options| against that modification, but looking at validateOptions again, I realize that isn't the case. So this can be the (slightly) simpler: options.globalName = apiUtils.validateOptions( options, { globalName: { is: ["string"], // The value cannot be an empty string, // since we name a property with it. ok: function(v) v != "" } } ).globalName; I have made it so. > >+ function configureSandbox() { > > can you pass the sandbox to this, instead of modifying vars from outside the > function scope? IMO the locals-only approach is easier to read, and less > bug-prone. Yup, agreed. I originally tried to do this when you suggested it earlier, and I succeeded with addSendMessageProperty and validateMessage, but configureSandbox seemed to need to close around the constructor's scope for some reason. I just revisited it, though, and could indeed move configureSandbox to the global scope and pass the values it needs to access into it as arguments. In the process, I made the globalName option a public read-only property of ContentSymbiont to make it easier for configureSandbox to access it (and because it is provided by the instantiator, so there is no reason for it to be a private property). > >+ // XXX I think the principal should be self.frame.contentWindow, but script > >+ // doesn't work correctly when I set it to that value. Events don't get > >+ // registered; even dump() fails. > >+ // > >+ // FIXME: figure out the problem and resolve it, so we can restrict > >+ // the sandbox to the same set of privileges the page has (plus any > >+ // others it gets to access through the object that created it). > >+ // > >+ // XXX when testing self.frame.contentWindow, I found that setting > >+ // the principal to its self.frame.contentWindow.wrappedJSObject resolved > >+ // some test leaks; that was before I started clearing the principal > >+ // of the sandbox on unload, though, so perhaps it is no longer a problem. > > file followup bugs for these? Yup, I will file the following two bugs after checking in the fix for this bug: 1. restrict principal of content scripts to principal of page they access; 2. ContentSymbiont objects leak in the content-symbiont and panel tests. > >+ //principal: self.frame.contentWindow.wrappedJSObject > > remove Done. > >+ // Alternate approach: > >+ // Define each individual global on which JavaScript libraries depend > >+ // in the global object of the sandbox. This is hard to get right, > >+ // since it requires a priori knowledge of the libraries developers use, > >+ // and exceptions in those libraries aren't always reported. It's also > >+ // brittle, prone to breaking when those libraries change. But it might > >+ // make it easier to avoid namespace conflicts. > > yeah, that sounds like it'll cause no end of problems. let's stick with the > former. Done! But I left the comment in the code for historical reference. > >+ for each (let contentScriptURL in self.contentScriptURL) { > >+ let filename = url.toFilename(contentScriptURL); > >+ errors.catchAndLog( > >+ function () Cu.evalInSandbox(file.read(filename), sandbox, "1.8", > >+ filename, 1) > >+ )(); > >+ } > >+ for each (let contentScript in self.contentScript) { > >+ errors.catchAndLog( > >+ function () Cu.evalInSandbox(contentScript, sandbox, "1.8", '<string>', > >+ 1) > >+ )(); > >+ } > >+ } > > can you put the js version in a global const so we an easily find and change in > the future? I sure can and have! > >+ // We define these independently of the main validateOptions call so we can > >+ // reuse them when checking values passed to the setters after construction. > >+ let widthReq = { > >+ is: ["number", "undefined"] > >+ }; > > super useful. we should add a whole bit on the "implementing a module" wiki > page about how to use validateOptions. Yupski. > >+ // Validate panel-specific options without filtering those validated by > >+ // contentSymbionts.mixInto. > >+ for each (let [key, val] in Iterator(apiUtils.validateOptions(options, > >+ { width: widthReq, height: heightReq, > >+ content: contentReq, onShow: onShowReq, > >+ onHide: onHideReq }))) { > >+ if (typeof(val) != "undefined") > >+ options[key] = val; > >+ }; > > that's awkward. not sure what to do about it though. Bug 569122 is all over it (but also dormant--zpao?!). > >+ // XXX Sometimes I get "TypeError: xulPanel.hidePopup is not a function" > >+ // when quitting the host application while a panel is visible. To suppress > >+ // them, this now checks for "hidePopup" in xulPanel before calling it. > >+ // It's not clear if there's an actual issue or the error is just normal. > >+ if (xulPanel && "hidePopup" in xulPanel) > >+ xulPanel.hidePopup(); > >+ } > > file followup Will do, although reducing the test case and making it reproducable will be painful, and it's possible that it's actually a bug in the panel code rather than the XUL panel implementation (perhaps even the one I fixed recently). :-/ In addition to these changes, I also noticed that there were some missing pieces and inconsistencies between panel, page-worker (not included in this patch), hidden-frame, and content-symbiont docs wrt. content script properties and the wording of various descriptions. I have added the missing pieces and consistentified the inconsistencies.

Comment on attachment 465763 [details] [diff] [review] patch v3: addresses review comments, improves docs interdiff: https://bugzilla.mozilla.org/attachment.cgi?oldid=465583&action=interdiff&newid=465763&headers=1 looks good, r=me.

Fixed by changeset https://hg.mozilla.org/labs/jetpack-sdk/rev/fddb5092774b.

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".

I see this error in DEBUG BUILD of comm-central TB run, am I missing a TB-specific patch or something. I refreshed the source a day ago or so. error: geckoprofiler: An exception occurred. Error: The panel module currently supports only Firefox. In the future we would like it to support other applications, however. Please see https://bugzilla.mozilla.org/show_bug.cgi?id=jetpack-panel-apps for more information. resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/panel.js 12 Traceback (most recent call last): File "resource://gre/modules/NetUtil.jsm:123", line 17, in NetUtil_asyncOpen/<.onStopRequest File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/net/url.js:49", line 9, in readAsync/< File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/core/promise.js:142", line 32, in resolve File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/core/promise.js:36", line 43, in then File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/core/promise.js:116", line 34, in resolved File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/core/promise.js:142", line 32, in resolve File "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/core/promise.js:36", line 43, in then ...[omitted] TIA

(In reply to ISHIKAWA, Chiaki from comment #43) > I see this error in DEBUG BUILD of comm-central TB run, > am I missing a TB-specific patch or something. > I refreshed the source a day ago or so. > > error: geckoprofiler: An exception occurred. > Error: The panel module currently supports only Firefox. In the future we > would like it to support other applications, however. Please see > https://bugzilla.mozilla.org/show_bug.cgi?id=jetpack-panel-apps for more > information. > resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/ > panel.js 12 > Traceback (most recent call last): > File "resource://gre/modules/NetUtil.jsm:123", line 17, in > NetUtil_asyncOpen/<.onStopRequest > File > "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/ > net/url.js:49", line 9, in readAsync/< > File > "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/ > core/promise.js:142", line 32, in resolve > File > "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/ > core/promise.js:36", line 43, in then > File > "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/ > core/promise.js:116", line 34, in resolved > File > "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/ > core/promise.js:142", line 32, in resolve > File > "resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/sdk/ > core/promise.js:36", line 43, in then > ...[omitted] > > TIA ishikawa, do you have a version of Gecko Profiler installed?

ignore comment 44, obviously you do have Gecko Profiler installed. I noticed this also starting with profiler 11.12.19. But I don't see it when using a *current* version of the profiler, which BTW doesn't work for me in Thunderbird. I don't see the profiler panels in TB status bar.

(In reply to Wayne Mery (:wsmwk) from comment #44) > .... > .... > > ishikawa, do you have a version of Gecko Profiler installed? As you correctly guessed, I do have Gecko profiler installed (however, I never seem to be able to use it effectively from thunderbird at all. Maybe it works in Firefox.) TIA

(In reply to ISHIKAWA, Chiaki from comment #46) > I never seem to be able to use it effectively from thunderbird at all. Current versions of profiler don't work with TB. But I've created https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Reporting_a_Thunderbird_Performance_Problem_with_G and the version it points to should work for yom

(In reply to Wayne Mery (:wsmwk) from comment #47) > (In reply to ISHIKAWA, Chiaki from comment #46) > > I never seem to be able to use it effectively from thunderbird at all. > > Current versions of profiler don't work with TB. But I've created > https://developer.mozilla.org/en-US/docs/Mozilla/Performance/ > Reporting_a_Thunderbird_Performance_Problem_with_G and the version it points > to should work for yom Thank you. It works now! It could have saved me the work of finding which function calls short |write| calls repeatedly when we copy a message with large binary attachment. (I used gdb and breakpoint, but profiler seems to capture the stack backtrace nicely.) I will report more profiling issues once I get the hang of using the profiler. TIA

Is there any chance to get this working on Android?