Bug 611763 - switch to construct/destroy model for Panel (r=myk)
authorAtul Varma <varmaa@gmail.com>
Tue, 30 Nov 2010 17:29:39 -0500
changeset 1149 86172a91aa6ab7146ff7beb47d8e2404b33dad06
parent 1136 a669c12d7f07f8c4d73fe9222f7304dd94bfe1ac (current diff)
parent 1148 4a3c87093ba3e61c10a970eb8fe58facd5fbb62a (diff)
child 1151 985136f1b156a4cca06ea44cd880401db7a1edf5
push id471
push userbwarner@mozilla.com
push dateTue, 30 Nov 2010 22:30:12 +0000
reviewersmyk
bugs611763
Bug 611763 - switch to construct/destroy model for Panel (r=myk)
examples/reddit-panel/lib/main.js
--- a/examples/reddit-panel/lib/main.js
+++ b/examples/reddit-panel/lib/main.js
@@ -1,17 +1,16 @@
 const widgets = require("widget");
-const panels = require("panel");
 const data = require("self").data;
 
 exports.main = function(options, callbacks) {
   widgets.Widget({
     label: "Reddit",
     contentURL: "http://www.reddit.com/static/favicon.ico",
-    panel: panels.Panel({
+    panel: require("panel").Panel({
       width: 240,
       height: 320,
       contentURL: "http://www.reddit.com/.mobile?keep_extension=True",
       contentScriptFile: [data.url("panel.js")],
       contentScriptWhen: "ready",
       onMessage: function(message) {
         require("tab-browser").addTab(message);
       }
--- a/packages/addon-kit/docs/panel.md
+++ b/packages/addon-kit/docs/panel.md
@@ -6,24 +6,19 @@ web content and browser chrome and persi
 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`.
+The module exports a single constructor function, `Panel`, which constructs a new panel.
 
-`Panel` constructs a new panel.  `add` registers a panel, loading its content
-and preparing it to be shown when its `show` method is invoked.  `remove`
-unregisters a panel, unloading the content that was loaded in it.
-
-A panel's content is loaded as soon as it is added, before the panel is shown,
+A panel's content is loaded as soon as it is created, before the panel is shown,
 and the content remains loaded when a panel is hidden, so it is possible
 to keep a panel around in the background, updating its content as appropriate
 in preparation for the next time it is shown.
 
 Panels can be anchored to a particular 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 an arrow that points from the panel to the
@@ -35,21 +30,20 @@ access to the content loaded into the pa
 content scripts to load for a panel, and the program can communicate with those
 scripts via an asynchronous message passing API.
 
 Examples
 --------
 
 Create and show a simple panel with content from the `data/` directory:
 
-    const panels = require("panel");
     const data = require("self").data;
-    let panel = panels.add(panels.Panel({
+    var panel = require("panel").Panel({
       contentURL: data.url("foo.html")
-    }));
+    });
 
     panel.show();
 
 The tutorial section on [web content](#guide/web-content) has a more complex
 example using panels.
 
 <api name="Panel">
 @class
@@ -142,16 +136,23 @@ property are loaded *after* those specif
 <api name="contentScriptWhen">
 @property {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.
 </api>
 
+<api name="destroy">
+@method
+Destroy the panel, unloading any content that was loaded in it. Once
+destroyed, the panel can no longer be used. If you just want to hide
+the panel and might show it later, use `hide` instead.
+</api>
+
 <api name="onMessage">
 @property {array}
 Functions to call when a content script sends the panel a message.
 </api>
 
 <api name="postMessage">
 @method
 Send a message to the content scripts.
--- a/packages/addon-kit/lib/panel.js
+++ b/packages/addon-kit/lib/panel.js
@@ -43,30 +43,27 @@ if (!require("xul-app").is("Firefox")) {
     "for more information."
   ].join(""));
 }
 
 const { Ci } = require("chrome");
 const { validateOptions: valid } = require("api-utils");
 const { Symbiont } = require("content");
 const { EventEmitter } = require('events');
-const { Registry } = require('utils/registry');
 
 require("xpcom").utils.defineLazyServiceGetter(
   this,
   "windowMediator",
   "@mozilla.org/appshell/window-mediator;1",
   "nsIWindowMediator"
 );
 
 const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
       ON_SHOW = 'popupshown',
       ON_HIDE = 'popuphidden',
-      ERR_ADD = "You have to add the panel via require('panel').add() " +
-                "before you can show it.",
       validNumber = { is: ['number', 'undefined', 'null'] };
 
 /**
  * Emits show and hide events.
  */
 const Panel = Symbiont.resolve({
   constructor: '_init',
   _onInit: '_onSymbiontInit',
@@ -96,18 +93,16 @@ const Panel = Symbiont.resolve({
       .swapFrameLoaders(this._viewFrame);
     this.__frameLoadersSwapped = value;
   },
   __frameLoadersSwapped: false,
 
   constructor: function Panel(options) {
     this._onShow = this._onShow.bind(this);
     this._onHide = this._onHide.bind(this);
-    PanelRegistry.on('add', this._onAdd.bind(this));
-    PanelRegistry.on('remove', this._onRemove.bind(this));
     this.on('inited', this._onSymbiontInit.bind(this));
 
     options = options || {};
     if ('onShow' in options)
       this.on('show', options.onShow);
     if ('onHide' in options)
       this.on('hide', options.onHide);
     if ('width' in options)
@@ -115,39 +110,38 @@ const Panel = Symbiont.resolve({
     if ('height' in options)
       this.height = options.height;
     if ('contentURL' in options)
       this.contentURL = options.contentURL;
 
     this._init(options);
   },
   _destructor: function _destructor() {
-    PanelRegistry.remove(this._public);
+    this.hide();
     this._removeAllListeners('show');
     // defer cleanup to be performed after panel gets hidden
     this._xulPanel = null;
     this._symbiontDestructor(this);
     this._removeAllListeners(this, 'hide');
   },
+  destroy: function destroy() {
+    this._destructor();
+  },
   /* Public API: Panel.width */
   get width() this._width,
   set width(value)
     this._width = valid({ $: value }, { $: validNumber }).$ || this._width,
   _width: 320,
   /* Public API: Panel.height */
   get height() this._height,
   set height(value)
     this._height =  valid({ $: value }, { $: validNumber }).$ || this._height,
   _height: 240,
   /* Public API: Panel.show */
   show: function show(anchor) {
-    // do nothing if already open
-    if (!PanelRegistry.has(this))
-      throw new Error(ERR_ADD);
-
     anchor = anchor || null;
     let document = getWindow(anchor).document;
     let xulPanel = this._xulPanel;
     if (!xulPanel) {
       xulPanel = this._xulPanel = document.createElementNS(XUL_NS, 'panel');
       let frame = document.createElementNS(XUL_NS, 'iframe');
       frame.setAttribute('type', 'content');
       frame.setAttribute('flex', '1');
@@ -166,19 +160,16 @@ const Panel = Symbiont.resolve({
       when = null;
     }
     xulPanel.sizeTo(width, height);
     xulPanel.openPopup(anchor, when, x, y);
     return this._public;
   },
   /* Public API: Panel.hide */
   hide: function hide() {
-    if (!PanelRegistry.has(this))
-      throw new Error(ERR_ADD);
-
     // The popuphiding handler takes care of swapping back the frame loaders
     // and removing the XUL panel from the application window, we just have to
     // trigger it by hiding the popup.
     // 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.
     let xulPanel = this._xulPanel;
@@ -238,51 +229,30 @@ const Panel = Symbiont.resolve({
         return this.on('inited', this._onShow.bind(this));
       this._frameLoadersSwapped = true;
       this._emit('show', this._public);
     } catch(e) {
       this._emit('error', e);
     }
   },
   /**
-   * Notification that panel was added.
-   */
-  _onAdd: function _onAdd(self) {
-    if (self == this._public && this._inited)
-      this._onInit();
-  },
-  /**
-   * Notification that panel was removed.
-   */
-  _onRemove: function _onRemove(self) {
-    if (self == this._public)
-        this.hide();
-  },
-  /**
    * Notification that panel was fully initialized.
-   * This will be called another time when panel was added if it
-   * was initialized before.
    */
   _onInit: function _onInit() {
     this._inited = true;
-    if (PanelRegistry.has(this)) {
-      // perform all deferred tasks like initSymbiont, show, hide ...
-      this._emit('inited');
-      this._removeAllListeners('inited');
-    }
+    // perform all deferred tasks like initSymbiont, show, hide ...
+    // TODO: We're publicly exposing a private event here; this
+    // 'inited' event should really be made private, somehow.
+    this._emit('inited');
+    this._removeAllListeners('inited');
   }
 });
 exports.Panel = function(options) Panel(options)
 exports.Panel.prototype = Panel.prototype;
 
-const PanelRegistry = Registry(Panel);
-
-exports.add = PanelRegistry.add;
-exports.remove = PanelRegistry.remove;
-
 function getWindow(anchor) {
   let window;
 
   if (anchor) {
     let anchorWindow = anchor.ownerDocument.defaultView.top;
     let anchorDocument = anchorWindow.document;
 
     let enumerator = windowMediator.getEnumerator("navigator:browser");
--- a/packages/addon-kit/lib/widget.js
+++ b/packages/addon-kit/lib/widget.js
@@ -254,18 +254,16 @@ let browserManager = {
   // all currently registered windows, and when new windows are registered it
   // will be added to them, too.
   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);
   },
 
   // Updates the content of an item registered with the manager,
   // propagating the change to all windows.
   updateItem: function browserManager_updateItem(item, property, value) {
     let idx = this.items.indexOf(item);
     if (idx != -1)
       this.windows.forEach(function (w) w.updateItem(item, property, value));
@@ -273,17 +271,17 @@ let browserManager = {
 
   // Unregisters an item from the manager.  It's removed from the addon-bar
   // of all windows that are currently registered.
   removeItem: function browserManager_removeItem(item) {
     let idx = this.items.indexOf(item);
     if (idx > -1) {
       this.items.splice(idx, 1);
       if (item.panel)
-        panels.remove(item.panel);
+        item.panel.destroy();
       this.windows.forEach(function (w) w.removeItems([item]));
     }
   },
 
   _isBrowserWindow: function browserManager__isBrowserWindow(win) {
     let winType = win.document.documentElement.getAttribute("windowtype");
     return winType === "navigator:browser";
   }
--- a/packages/addon-kit/tests/test-panel.js
+++ b/packages/addon-kit/tests/test-panel.js
@@ -1,52 +1,52 @@
 let { Cc, Ci } = require("chrome");
 let panels = require('panel');
 let URL = require("url").URL;
 let tests = {}, panels, Panel;
 
 tests.testPanel = function(test) {
   test.waitUntilDone();
-  let panel = panels.add(Panel({
+  let panel = Panel({
     contentURL: "about:buildconfig",
     contentScript: "postMessage(1); on('message', function() postMessage(2));",
     onMessage: function (message) {
       switch(message) {
         case 1:
           test.pass("The panel was loaded.");
           panel.postMessage('');
           break;
         case 2:
           test.pass("The panel posted a message and received a response.");
-          panels.remove(panel);
+          panel.destroy();
           test.done();
           break;
       }
     }
-  }));
+  });
 };
 
 tests.testShowHidePanel = function(test) {
   test.waitUntilDone();
-  let panel = panels.add(Panel({
+  let panel = Panel({
     contentScript: "postMessage('')",
     contentScriptWhen: "ready",
     onMessage: function (message) {
       panel.show();
     },
     onShow: function () {
       test.pass("The panel was shown.");
       panel.hide();
     },
     onHide: function () {
-      panels.remove(panel);
+      panel.destroy();
       test.pass("The panel was hidden.");
       test.done();
     }
-  }));
+  });
 };
 
 tests.testResizePanel = function(test) {
   test.waitUntilDone();
 
   // These tests fail on Linux if the browser window in which the panel
   // is displayed is not active.  And depending on what other tests have run
   // before this one, it might not be (the untitled window in which the test
@@ -60,17 +60,17 @@ tests.testResizePanel = function(test) {
                      getMostRecentWindow(null);
   let browserWindow = Cc["@mozilla.org/appshell/window-mediator;1"].
                       getService(Ci.nsIWindowMediator).
                       getMostRecentWindow("navigator:browser");
 
   function onFocus() {
     browserWindow.removeEventListener("focus", onFocus, true);
 
-    let panel = panels.add(Panel({
+    let panel = Panel({
       contentScript: "postMessage('')",
       contentScriptWhen: "ready",
       height: 10,
       width: 10,
       onMessage: function (message) {
         panel.show();
       },
       onShow: function () {
@@ -78,68 +78,114 @@ tests.testResizePanel = function(test) {
         panel.hide();
       },
       onHide: function () {
         test.assert((panel.width == 100) && (panel.height == 100),
           "The panel was resized.");
         activeWindow.focus();
         test.done();
       }
-    }));
+    });
   }
 
   if (browserWindow === activeWindow) {
     onFocus();
   }
   else {
     browserWindow.addEventListener("focus", onFocus, true);
     browserWindow.focus();
   }
 };
 
 tests.testHideBeforeShow = function(test) {
   test.waitUntilDone();
   let showCalled = false;
-  let panel = panels.add(Panel({
+  let panel = Panel({
     onShow: function () {
       showCalled = true;
     },
     onHide: function () {
       test.assert(!showCalled, 'must not emit show if was hidden before');
       test.done();
     }
-  }));
+  });
   panel.show();
   panel.hide();
 };
 
 tests.testSeveralShowHides = function(test) {
   test.waitUntilDone();
   let hideCalled = 0;
-  let panel = panels.add(panels.Panel({
+  let panel = panels.Panel({
     contentURL: "about:buildconfig",
     onShow: function () {
       panel.hide();
     },
     onHide: function () {
       hideCalled++;
       if (hideCalled < 3)
         panel.show();
       else {
         test.pass("onHide called three times as expected");
         test.done();
       }
     }
-  }));
+  });
   panel.on('error', function(e) {
     test.fail('error was emitted:' + e.message + '\n' + e.stack);
   });
   panel.show();
 };
 
+function makeEventOrderTest(options) {
+  let expectedEvents = [];
+
+  return function(test) {
+    let panel = panels.Panel({ contentURL: "about:buildconfig" });
+
+    function expect(event, cb) {
+      expectedEvents.push(event);
+      panel.on(event, function() {
+        test.assertEqual(event, expectedEvents.shift());
+        if (cb)
+          require("timer").setTimeout(cb, 1);
+      });
+      return {then: expect};
+    }
+
+    test.waitUntilDone();
+    options.test(test, expect, panel);
+  }
+}
+
+tests.testWaitForInitThenShowThenDestroy = makeEventOrderTest({
+  test: function(test, expect, panel) {
+    expect('inited', function() { panel.show(); }).
+      then('show', function() { panel.destroy(); }).
+      then('hide', function() { test.done(); });
+  }
+});
+
+tests.testShowThenWaitForInitThenDestroy = makeEventOrderTest({
+  test: function(test, expect, panel) {
+    panel.show();
+    expect('inited').
+      then('show', function() { panel.destroy(); }).
+      then('hide', function() { test.done(); });
+  }
+});
+
+tests.testShowThenHideThenDestroy = makeEventOrderTest({
+  test: function(test, expect, panel) {
+    panel.show();
+    expect('show', function() { panel.hide(); }).
+      then('hide', function() { panel.destroy(); test.done(); });
+  }
+});
+
 tests.testContentURLOption = function(test) {
   const URL_STRING = "about:buildconfig";
   const HTML_CONTENT = "<html><title>Test</title><p>This is a test.</p></html>";
 
   let (panel = Panel({ contentURL: URL_STRING })) {
     test.pass("contentURL accepts a string URL.");
     test.assert(panel.contentURL instanceof URL,
                 "contentURL is a URL object.");
--- a/packages/jetpack-core/docs/self.md
+++ b/packages/jetpack-core/docs/self.md
@@ -40,21 +40,19 @@ data that can be displayed directly in a
 <api name="data.url">
 @method
 The `data.url(NAME)` method returns a URL instance that points at an embedded
 data file. It is most useful for data that can be displayed directly in a
 content frame. The URL instance can be passed to a content frame constructor,
 such as the Panel:
 
     var self = require("self");
-    var panels = require("panel");
-    var myPanel = panels.Panel({
+    var myPanel = require("panel").Panel({
       contentURL: self.data.url("my-panel-content.html")
     });
-    panels.add(myPanel);
     myPanel.show();
 
 @param name {string} The filename to be read, relative to the
   package's `data` directory. Each package that uses the `self` module
   will see its own `data` directory.
 @returns {URL}
 </api>
 </api>
--- a/static-files/md/dev-guide/web-content.md
+++ b/static-files/md/dev-guide/web-content.md
@@ -171,25 +171,25 @@ a string, number, boolean, array of JSON
 whose property values are JSON-serializable.
 
 ###Message handling in the add-on script
 The worker API is not exposed to add-on code in quite the same way in all
 modules. The panel and page objects integrate the worker API directly. So to
 receive messages from a content script associated with a panel you can
 register as a listener in its constructor:
 
-    panel = panels.add(panels.Panel({
+    panel = require("panel").Panel({
       contentURL: "http://www.reddit.com/.mobile?keep_extension=True",
       contentScriptFile: data.url("panel.js"),
       contentScriptWhen: "ready",
       // Register the handleMessage function as a listener
       onMessage: function handleMessage(message) {
         // Handle the message
       }
-    }));
+    });
 
 To send messages to a content script from a panel you can just call
 `panel.postMessage()`.
 
 The panel and page objects only host a single page at a time, so each distinct
 page object only needs a single channel of communication to its content
 scripts. But some modules, such as page-mod, might need to handle multiple
 pages, each with its own context in which the content scripts are executing,