Closed Bug 571449 Opened 14 years ago Closed 14 years ago

implement a high-level window api

Categories

(Add-on SDK Graveyard :: General, defect, P1)

Product:
Add-on SDK Graveyard
Component:
General
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: dietrich, Assigned: Felipe)

Details

Attachments

(2 files, 9 obsolete files)

Parts 1 - 4 merged
50.22 KB, patch
dietrich
: review+
Details | Diff | Splinter Review
Part 5 - Implement high-level window API - v4
26.52 KB, patch
myk
: review+
Details | Diff | Splinter Review 
high-level modules such as the tabs module need to be able to do things like open tabs in specific windows, or move tabs between windows, and therefore need a way to reference open browser windows.
Assignee: nobody → felipc
My take on this API is that the initial version should be focused on satisfying the common use cases for tab access and management.  I don't have a complete API worked out yet, but here are my first thoughts on the kind of API I'm thinking about:

windows = require("window");

for (let window in windows) {
  window.tabs; // the tabs in a window; should behave like the tabs module
  window.tabs.activeTab; // the active tab in a window

  let tab = window.tabs.activeTab;
  tab.move(position /* zero-based index of location to which to move tab */,
           window   /* the window to which to move the tab; optional; if not
                     * specified, moves the tab within its current window */);
}
Ok, I did some more research, and I still don't have a good sense of the important use cases to support besides moving tabs between windows and activating windows, so let's start with that for a first pass and then add additional properties and methods as we identify common use cases.

Here's a doc that specifies the proposed API via example code:

http://gist.github.com/509053
The only omission I can see is a way to open a new window. I know you can do that with the tabs module, but it's counter-intuitive to require users to do it there, and not be able to do it here.
Dietrich, Myk, here's an update on the approach I'm taking:

Since the window api have a tabs property, I want to reuse the tabs api for that purpose. As the tabs API currently tracks tabs across all windows, I'm modifying it to be able to generate a module that tracks all tabs or only tabs from a certain window.

To do that, I'm modifying the tabs.js as tabs-helper.js. This file, instead of directly exporting a module, will export a module constructor called TabModule, which is basically everything that used to go to _exports_ on tabs.js, but now going to a regular object.

this constructor is like this:
function TabModule(window) {
  if (window)
    // generate module for a single window
  else
    // generate module for all windows
}

And thus, the proper, user-facing tabs.js becomes simply:

    const TabModule = require("tab-helper").TabModule();
    exports = new TabModule();


while windows.js will have:

    exports.window = {
      tabs: new TabModule(thisWindow);
    }

(This is a simplification of course)


So I'm interested in hearing what you think about this. Do you think this direction sound good, or should I try something else?
I was not sure of adding a helper lib file that in theory will be of no use externally, but I guess there are other files like this as well (and I was looking at myk's implementation of e10s frames, and something similar goes on for the common parts of content-frames/page-worker)


At the moment I have 40 of 41 of tabs tests passing (just favicon test fails due to use of btoa function), and I've started writing the modifications required to create the single-window-tracking module. After that is complete, implementing the other parts of window api will be straightforward.


I'm also thinking on the patch queue strategy to avoid re-reviewing code from the tabs module that is already reviewed but will just be moved around (as most parts of it need to move inside the object closure). I've got a pretty good idea of what to do (hg rename, then patch to move around, then patch with adjustments, then patch with new parts), but I have yet to see how that will work out in practice.
Status: NEW → ASSIGNED
Alright, patches incoming, using the approach above (as there was no opposition and I didn't find any showstopper during impl).

I've split the patches in 5 parts to make it easier to manage and understand the changes. I'll explain each one in more detail.

We haven't talked about who should review this, so tentatively asking review from Dietrich.
This patch just reorder blocks of code on tabs.js to position them on the right places to wrap them around a constructor on a later patch (blocks of code that will be window-specific are together, and functions that are used for any window are together).

I did this part first because the patch basically touches on the entire file, but if there's some easy way to see code moves it can be seen that there are no real code changes.
(Also, every patch except the file rename should leave the code on a fully working state)

previous order:
---------------
tabConstructor
function getThumbnailForTabs
function getChromeURLContents
exports.activeTab
function open
function openURLInNewWindow
function openURLInNewTab
events.forEach
function tabsIterator
function TabTracker
eventsTabDelegate
eventsTabTracker
exports.iterator
function unload

new order:
----------
tabConstructor
exports.activeTab
events.forEach
eventsTabDelegate
eventsTabTracker
exports.iterator
function unload
function open
function openURLInNewWindow
function openURLInNewTab
function tabsIterator
function TabTracker
function getThumbnailForTabs
function getChromeURLContents
Attachment #466249 - Flags: review?(dietrich)
This renames the file tabs.js to tabs-helper.js. I wanted to do this in a separate commit to make sure there are no weird Mercurial bugs to affect this.

I did $ hg rename tabs.js tabs-helper.js

and it looks like the results from hg log --follow and hg blame are correct
Attachment #466250 - Flags: review?(dietrich)
hg diff -w

This takes the tabs-helper file and wraps most part of it in a constructor. This will be exported as the TabModule constructor, which can be used to create a module to handle all tabs, or only tabs from a single window (next patch).

The tabs.js file is recreated now using this exported module.

The patch posted here uses -w to ignore whitespace as half of the code will shift one indentation to go inside the module.

Here's what this patch does to the previous file:

function TabModule() {
  tabConstructor
  this.activeTab
  this.open <-- added
  events.forEach
  eventsTabDelegate
  eventsTabTracker
  this.iterator
  function unload
}

function open
function openURLInNewWindow
function openURLInNewTab
function tabsIterator
function TabTracker
function getThumbnailForTabs
function getChromeURLContents
Attachment #466252 - Flags: review?(dietrich)
Code-wise, this is the tricky part. This adds a window parameter to the TabModule constructor, and if given, the iterators/listeners/delegates will only follow the events related to tabs on that window.

There were some changes needed on tab-browser's Tracker and some other common code was put together in functions to avoid having big conditionals of "if (window was given) { ..... } else { ..... }"

Added tests to the new behavior here and to cover difficult parts of the impl like to make sure that events from one module doesn't fire in another one.
Attachment #466254 - Flags: review?(dietrich)
(oh, also picked up bug 586889 on that last patch, as I was touching code on tab-browser and it was a straightforward fix. Added a test to cover it)
Part 5 should be the patch to actually create the window api and add the activeWindow and openWindow methods. I haven't finished that part yet (mostly missing tests)
Attachment #466249 - Flags: review?(dietrich) → review+
So here is the window API. The final look of this api:

-----------
window.tabs
  implement the tabs module for each window

-----------
windows. (__iterator__ and length):
  returns all the navigator:browser windows, wrapped in a safe representation called safeWindowObj

Talking with Myk on IRC, we were discussing if this API should also handle non-browser windows. As there's a movement of getting rid of non-browser windows (e.g. the add-ons manager), and each different window will have its own unique characteristics, we thought of making this high-level API only about the browser windows. If someone has opinions on why that should or shouldn't be the case, fire away!

As such, the length property will correctly reflect the number of windows returned by the iterator (that is, it won't count non-browser windows). The activeWindow property also handles this (as explained below).

---------
windows.activeWindow

Returns an object representing the current activeWindow. If the active window is a non-browser window, this will return null. This is a getter and a setter, and the property can be set by assigning one of the safe-object represetations to it, which will find the xulwindow and call focus() on it.

---------
windows.onOpen windows.onClose

Fired when a new window is opened or closed.

-------
windows.openWindow

Function that takes an object with an URL and an onOpen handler. Like tabs.open with inNewWindow implied


-----------
window.close(callback)
To be able to to test the openWindow function, and to also add functionatily, each window obj has a close function that can be called, and a callback is accepted that is fired when the unload event is received.

--------
window.title

The title of the window. Usually the title of the active tab, + " - Firefox" or other app identifier



Implementation-wise, the interesting point here is the map that had to be done to be able to convert from native element <-> safe object and vise-versa in places like the activeWindow setter. I really wanted to use an approach like dietrich did on the tabs' module (iterating through the elements and comparing some known property, like contentDocument), but there was no such property here. The good thing is that with the cache, we can avoid creating more than one object per window. It's created when onLoad is fired, and is kept mapped. When the window goes away, it's deleted.  Also, the _tabs_ property is only lazily evaluated when first used, so creating it early shouldn't be a problem
Attachment #466793 - Flags: review?(myk)
Comment on attachment 466793 [details] [diff] [review]
Part 5 - Implement high-level window API

Looking good, just a few issues...

Note: I was unable to apply part 4 when I tried to apply these patches to a tip clone:

(jsdk)myk@myk:~/Projects/jsdk-windows$ hg import --no-commit ~/part*
applying /home/myk/part1-reorder-blocks.diff
applying /home/myk/part2-rename-file.diff
applying /home/myk/part3-make-tabs-module-w.diff
applying /home/myk/part4-handle-single-windows.diff
patching file packages/jetpack-core/lib/tabs-helper.js
Hunk #1 FAILED at 64
Hunk #2 FAILED at 126
Hunk #3 FAILED at 167
Hunk #4 FAILED at 234
4 out of 6 hunks FAILED -- saving rejects to file packages/jetpack-core/lib/tabs-helper.js.rej
abort: patch failed to apply


>diff --git a/packages/jetpack-core/docs/windows.md b/packages/jetpack-core/docs/windows.md

>+The `windows` module provides easy access to browser windows, its tabs, and open/close
>+related functions and events.

Nit: its tabs -> their tabs
Nit: wrap this line and others at 80 columns.


>+Listeners are passed the `window object that triggered the event.

Nit: `window -> `window`


>+<api name="onOpen">
>+@property {collection}
>+Fired when a new window is opened.
>+</api>
>+
>+<api name="onClose">
>+@property {collection}
>+Fired when a window is closed.
>+</api>

Nit: fired -> called


>+**Examples**
>+
>+    var windows = require("windows");
>+
>+    // listen for window openings via property assignment
>+    windows.onOpen = function(window) {
>+      myOpenWindows.push(window);
>+    }

Nit: over in bug 588250, we're going to remove the ability to set a collection property like this, so these docs should avoid mentioning this ability, even though it currently exists.


>+Window
>+----
>+
>+A `window` object represents a single open window. It contains the following
>+window properties and methos

Nit: methos -> methods:


>+<api name="tabs">
>+@property {object}
>+An object represeting all the open tabs on the window. This object
>+has all the properties and methods of the `tabs` module.
>+This property is read-only.
>+</api>

Nit: represeting -> representing



>+    //Print how many tabs the current window have
>+    console.log("The active window have " +
>+                windows.activeWindow.tabs.length +
>+                " tabs.");

Nit: have -> has


>+    // Print the title of all windows
>+    for (var window in windows) {
>+    console.log(window.title);
>+    }

Nit: indent the console.log line another two spaces.


>diff --git a/packages/jetpack-core/lib/window-utils.js b/packages/jetpack-core/lib/window-utils.js
>+exports.__defineGetter__("activeWindow", function() {
>+  return Cc["@mozilla.org/appshell/window-mediator;1"]
>+         .getService(Ci.nsIWindowMediator)
>+         .getMostRecentWindow("");

Shouldn't the argument to nsIWindowMediator::getMostRecentWindow be null?


>diff --git a/packages/jetpack-core/lib/windows.js b/packages/jetpack-core/lib/windows.js

>+/**
>+ * Window
>+ *
>+ * Safe object representing a window
>+ */
>+let windowConstructor = function (element) {

Instead of assigning this function to a variable, just give it a name, like BrowserWindow, i.e.:

  function BrowserWindow(element) {


>+  this.element = element;

Setting this.element exposes the element to consumers of the API, making this not so safe an object.  element should remain a private property that is only exposed to other code inside this module.  And as far as I can tell, it can be, since this is not used elsewhere in the module.


>+events.forEach(function(e) {
>+  // create a collection for each event
>+  collection.addCollectionProperty(exports, e);
>+  // make setter for each event, for adding via property assignment
>+  exports.__defineSetter__(e, function(val) exports[e].add(val));

addCollectionProperty actually handles setting, although the behavior is different (it removes existing items), and as mentioned earlier I'm going to remove the ability to set a collection property per bug 588250, so this should be left out.


>+  onTrack: function(window) {
>+    let safeWindowObj = windowMap.create(window);
>+    if (!safeWindowObj)
>+      return;
>+    for (let callback in exports.onOpen) {
>+      errors.catchAndLog(function(safeWindowObj) {
>+        callback(safeWindowObj);
>+      })(safeWindowObj);
>+    }
>+  },
>+  onUntrack: function(window) {
>+    let safeWindowObj = windowMap.getWindowObject(window);
>+    for (let callback in exports.onClose) {
>+      errors.catchAndLog(function(safeWindowObj) {
>+        callback(safeWindowObj);
>+      })(safeWindowObj);
>+    }

The "this" object for the onOpen and onClose callbacks needs to be set so they don't get access to the module's global scope.  Since the callbacks are defined on the exports object, that should be the "this" object in this case, i.e.:

  for (let callback in exports.onOpen) {
    errors.catchAndLog(function(safeWindowObj) {
      callback.call(exports, safeWindowObj);
    })(safeWindowObj);
  }


One more thing: I still think we should only expose browser windows.  However, I'd like to make it clearer that we are doing so and leave the door open to adding an API for accessing all windows if we determine that such an API would be useful.  So let's name this module "browser-windows" instead of just "windows".

I'm also still not completely sure about the behavior of activeWindow, since it means that a consumer can't get access to the topmost browser window if it isn't the active window.  On the other hand, I don't want to redefine the term "active" to mean the topmost browser window.

Nor is adding a topmostWindow accessor particularly palatable, although so far I haven't thought of anything better, so that may be the winner.  In any case, what we have here is fine for now, but we should noodle on the problem some more.
Attachment #466793 - Flags: review?(myk) → review-
(In reply to comment #13)
> Comment on attachment 466793 [details] [diff] [review]
> Part 5 - Implement high-level window API
> 
> Looking good, just a few issues...
> 
> Note: I was unable to apply part 4 when I tried to apply these patches to a tip
Oh, part 4 failed because part 3 was a diff ignoring whitespace (for easy reviewing). I'll post an updated patch with parts 1-4 merged per dietrich's suggestion  (also, tabs-helper.js won't be created. instead, all of its functions will move to tab-browser).


> 
> 
> >+**Examples**
> >+
> >+    var windows = require("windows");
> >+
> >+    // listen for window openings via property assignment
> >+    windows.onOpen = function(window) {
> >+      myOpenWindows.push(window);
> >+    }
> 
> Nit: over in bug 588250, we're going to remove the ability to set a collection
> property like this, so these docs should avoid mentioning this ability, even
> though it currently exists.

ok. will change that to windows.onOpen.add

> 
> >diff --git a/packages/jetpack-core/lib/window-utils.js b/packages/jetpack-core/lib/window-utils.js
> >+exports.__defineGetter__("activeWindow", function() {
> >+  return Cc["@mozilla.org/appshell/window-mediator;1"]
> >+         .getService(Ci.nsIWindowMediator)
> >+         .getMostRecentWindow("");
> 
> Shouldn't the argument to nsIWindowMediator::getMostRecentWindow be null?

Just checked on the impl, they mean the same. I'll change to null though to save a string :p

> 
> 
> >diff --git a/packages/jetpack-core/lib/windows.js b/packages/jetpack-core/lib/windows.js
> 
> >+/**
> >+ * Window
> >+ *
> >+ * Safe object representing a window
> >+ */
> >+let windowConstructor = function (element) {
> 
> Instead of assigning this function to a variable, just give it a name, like
> BrowserWindow, i.e.:
> 
>   function BrowserWindow(element) {
>
Ok, I'll change to BrowserWindow
 
> 
> >+  this.element = element;
> 
> Setting this.element exposes the element to consumers of the API, making this
> not so safe an object.  element should remain a private property that is only
> exposed to other code inside this module.  And as far as I can tell, it can be,
> since this is not used elsewhere in the module.

Oh yeah, exposing this defeats the whole purpose of the API. I was just using this for debugging, thanks for catching!

> 
> 
> >+events.forEach(function(e) {
> >+  // create a collection for each event
> >+  collection.addCollectionProperty(exports, e);
> >+  // make setter for each event, for adding via property assignment
> >+  exports.__defineSetter__(e, function(val) exports[e].add(val));
> 
> addCollectionProperty actually handles setting, although the behavior is
> different (it removes existing items), and as mentioned earlier I'm going to
> remove the ability to set a collection property per bug 588250, so this should
> be left out.

Ok, so I should just remove the setter, right? and collection.addCollectionProperty remains

> 
> 
> >+  onTrack: function(window) {
> >+    let safeWindowObj = windowMap.create(window);
> >+    if (!safeWindowObj)
> >+      return;
> >+    for (let callback in exports.onOpen) {
> >+      errors.catchAndLog(function(safeWindowObj) {
> >+        callback(safeWindowObj);
> >+      })(safeWindowObj);
> >+    }
> >+  },
> >+  onUntrack: function(window) {
> >+    let safeWindowObj = windowMap.getWindowObject(window);
> >+    for (let callback in exports.onClose) {
> >+      errors.catchAndLog(function(safeWindowObj) {
> >+        callback(safeWindowObj);
> >+      })(safeWindowObj);
> >+    }
> 
> The "this" object for the onOpen and onClose callbacks needs to be set so they
> don't get access to the module's global scope.  Since the callbacks are defined
> on the exports object, that should be the "this" object in this case, i.e.:
> 
>   for (let callback in exports.onOpen) {
>     errors.catchAndLog(function(safeWindowObj) {
>       callback.call(exports, safeWindowObj);
>     })(safeWindowObj);
>   }

Thanks, will do

> 
> 
> One more thing: I still think we should only expose browser windows.  However,
> I'd like to make it clearer that we are doing so and leave the door open to
> adding an API for accessing all windows if we determine that such an API would
> be useful.  So let's name this module "browser-windows" instead of just
> "windows".

browser-windows it is

> 
> I'm also still not completely sure about the behavior of activeWindow, since it
> means that a consumer can't get access to the topmost browser window if it
> isn't the active window.  On the other hand, I don't want to redefine the term
> "active" to mean the topmost browser window.
> 
> Nor is adding a topmostWindow accessor particularly palatable, although so far
> I haven't thought of anything better, so that may be the winner.  In any case,
> what we have here is fine for now, but we should noodle on the problem some
> more.

Another idea would be to follow the windowWatcher naming and use mostRecentWindow?

I added low-level apis to window-utils as a getter/setter for activeWindow, and a getter for activeBrowserWindow. Should I keep that like that for now?  Or rename activeBrowserWindow to mostRecentBrowserWindow?
Basically no code changes, just moved everything originally at tabs-helper to tab-browser. The tab module is exported as tabBrowser.TabModule, and the supported events as tabBrowser.tabEvents

The TabTracker function was renamed to ModuleTabTracker to avoid naming conflicts.
- Fixed all nits and comments

- Changed the module name to browser-windows and updated all tests and docs to reflect that.

- Removed listener setter and updated tests and docs

- I left the activeWindow untouched because I wasn't sure if you meant we should change it now or keep it in mind for a follow-up bug.
Attachment #466793 - Attachment is obsolete: true
Attachment #466900 - Flags: review?(myk)
(In reply to comment #15)
> Created attachment 466895 [details] [diff] [review]
> Parts 1 - 4 merged

Note: on part 4, I added the fix and test from bug 586889 (tabs' inBackground doesn't work because tab-browser expected openInBackground)
(In reply to comment #13)
> One more thing: I still think we should only expose browser windows.  However,
> I'd like to make it clearer that we are doing so and leave the door open to
> adding an API for accessing all windows if we determine that such an API would
> be useful.  So let's name this module "browser-windows" instead of just
> "windows".

I would like to formally contest the renaming! :)

I think there's a balance to be had between 100% descriptive naming, and adding unnecessary caveats and complexity through module explosion (window-utils, browser-windows, and later, windows).

I'd like to keep the core set of high-level modules as compact as possible. This was the driver behind asking Felipe to fold tab-helper into tab-browser (otherwise we would have had 3 tab related modules, only 1 of which is intended for add-on developers to use!).

In this case, I'd prefer to keep the module as "windows", and document that it only supports tabbed-browser windows for now. If we want to support more than that, we can add it later without having to either rename the module or add more modules.
Talked to Myk on IRC, switched back to windows, and moved the iterator to windows.browserWindows (and windows.browserWindows.length). This allows us to add a windows iterator (over all) and others in the future, and clarifies what windows are available from the api now.

However, the patch is failing testActiveWindow on Linux, even without any of my changes. Currently investigating.
Attachment #466249 - Attachment is obsolete: true
Attachment #466250 - Attachment is obsolete: true
Attachment #466252 - Attachment is obsolete: true
Attachment #466254 - Attachment is obsolete: true
Attachment #466900 - Attachment is obsolete: true
Attachment #466250 - Flags: review?(dietrich)
Attachment #466252 - Flags: review?(dietrich)
Attachment #466254 - Flags: review?(dietrich)
Attachment #466900 - Flags: review?(myk)
Looks like nonBrowserWindow.focus() doesn't actually focus the window - I added a focus event listener and it was never called.
Turns out in nextTest(), there was a while which should've been an if.
Tests fixed.
Attachment #466952 - Attachment is obsolete: true
Attachment #467012 - Flags: review?(myk)
Comment on attachment 467012 [details] [diff] [review]
Part 5 - Implement high-level window API v3.1 (windows.browserWindows)

This looks good, except that activeWindow, openWindow, onOpen, and onClose are all browser window-specific, so they should be attributes of the browserWindows object rather than the top-level exports object.
Attachment #467012 - Flags: review?(myk) → review-
Alright, I'll do that. What about leaving openWindow in the top-level exports and rename it to openBrowserWindow? I think it makes more sense there.

Still on naming, any opinions about
windows.browserWindows.activeWindow vs. windows.browserWindows.active ?
(In reply to comment #24)
> Alright, I'll do that. What about leaving openWindow in the top-level exports
> and rename it to openBrowserWindow? I think it makes more sense there.

That's not unreasonable, although it would be more consistent to put it in the browserWindows object, and doing so would mean that one wouldn't have to access two different objects to both open browser windows and access them, i.e.:

  let windows = require("windows").browserWindows;
  let window = windows.openWindow(...);
  if (window == windows.activeWindow)
    console.log("Hurrah.");
  for (let windoh in windows)
    if (window == windoh)
      console.log("D'oh!");

So putting it into browserWindows seems the better option.


> Still on naming, any opinions about
> windows.browserWindows.activeWindow vs. windows.browserWindows.active ?

I prefer activeWindow, because the property's object is a collection of windows, which makes "active" by itself slightly ambiguous, and for consistency with activeTab.


Note: I'm holding the first RC for the late landing of this patch, if we can get it ready in time (which is roughly defined as today, and the sooner, the better).
Yeah, accessing two different objects to call openWindow would be weird.

Moved onOpen/onClose/activeWindow/openWindow to exports.browserWindows, updated tests and docs.

Also fixed another dumb bug on my nextStep test function (the setTimeout should be inside the if).
Attachment #467012 - Attachment is obsolete: true
Attachment #467164 - Flags: review?(myk)
Comment on attachment 467164 [details] [diff] [review]
Part 5 - Implement high-level window API - v3

So close!


Note: the following hunk failed to apply when I applied the latest patch to a tip clone:

--- tab-browser.js
+++ tab-browser.js
@@ -502,20 +502,17 @@ function open(options, tabConstructor, w
     onOpen: {
       is: ["undefined", "function"]
     }
   });
 
   if (window)
     options.inNewWindow = false;
 
-  // TODO: remove me. maybe implement window-utils.activeWindow?
-  const wm = Cc["@mozilla.org/appshell/window-mediator;1"].
-             getService(Ci.nsIWindowMediator);
-  let win = window || wm.getMostRecentWindow("navigator:browser");
+  let win = window || require("window-utils").activeBrowserWindow;
 
   if (!win || options.inNewWindow)
     openURLInNewWindow(options, tabConstructor);
   else
     openURLInNewTab(options, win, tabConstructor);
 }
 
 function openURLInNewWindow(options, tabConstructor) {


>diff --git a/packages/jetpack-core/docs/windows.md b/packages/jetpack-core/docs/windows.md

>+This module currently only supports browser windows, and does not provide
>+access to non-browser windows such as the bookmarks Library, preferences
>+or other non-browser windows created via add-ons.

Nit: windows, and does not provide -> windows and does not provide
Nit: bookmarks Library -> Bookmarks Library


>+**Example**
>+
>+    var windows = require("windows");
>+    for each (window in windows.browserWindows) {
>+      console.log(window.title);
>+    }
>+
>+    console.log(windows.browserWindows.length);

Nit: these docs demonstrate the length property but seem to be missing a description of it.


>+If the only option being used is `url`, then a bare string URL can be passed to
>+`open` instead of adding at a property of the `options` object.

Nit: adding at -> specifying it as


>+@prop [url] {string}
>+String URL to be opened in the new window.
>+This is a required property.
>+
>+@prop [onOpen] {function}
>+A callback function that is called when the window has opened. This does not
>+mean that the URL content has loaded, only that the window itself is fully
>+functional and its properties can be acessed. This is an optional property.

Nit: acessed -> accessed


>+**Example**
>+
>+    var browserWindows = require("windows").browserWindows;
>+
>+    // open a new window
>+    browserWindows.openWindow("http://www.mysite.com");
>+
>+    // an onOpen listener
>+    browserWindows.openWindow({
>+      url: "http://www.mysite.com",
>+      onOpen: function(window) {
>+        // do stuff like listen for content
>+        // loading.
>+      }
>+    });

Nit: this example (and others) would be a bit simpler and easier to read if the module were required into a "windows" variable rather than a "browserWindows" variable.


>diff --git a/packages/jetpack-core/lib/windows.js b/packages/jetpack-core/lib/windows.js

>+  __iterator__: function() {
>+    let enum = Cc["@mozilla.org/appshell/window-mediator;1"]
>+               .getService(Ci.nsIWindowMediator)
>+               .getEnumerator("navigator:browser");
>+    while (enum.hasMoreElements())
>+      yield windowMap.getWindowObject(enum.getNext());
>+  },

Nit: apparently enum is a reserved identifier (making this generate a strict warning), so this should use a different name.


>+    if (options.onOpen) {
>+      addTabOptions.onLoad = function(e) {
>+        let win = e.target.defaultView;
>+        let safeWindowObj = windowMap.create(win);
>+        let tabEl = win.gBrowser.tabContainer.childNodes[0];
>+        let tabBrowser = win.gBrowser.getBrowserForTab(tabEl);
>+        tabBrowser.addEventListener("load", function(e) {
>+          tabBrowser.removeEventListener("load", arguments.callee, true);
>+          errors.catchAndLog(function(e) options.onOpen(e))(safeWindowObj);
>+        }, true);

options.onOpen should probably be called with safeWindowObj as the "this" object.


>+  onTrack: function(window) {
>+    let safeWindowObj = windowMap.create(window);
>+    if (!safeWindowObj)
>+      return;
>+    for (let callback in exports.browserWindows.onOpen) {
>+      errors.catchAndLog(function(safeWindowObj) {
>+        callback.call(exports, safeWindowObj);
>+      })(safeWindowObj);
>+    }
>+  },
>+  onUntrack: function(window) {
>+    let safeWindowObj = windowMap.getWindowObject(window);
>+    for (let callback in exports.browserWindows.onClose) {
>+      errors.catchAndLog(function(safeWindowObj) {
>+        callback.call(exports, safeWindowObj);
>+      })(safeWindowObj);
>+    }

Now that the object on which one specifies onOpen and onClose callback properties is exports.browserWindows, that should be the "this" object when executing those callbacks.


>diff --git a/packages/jetpack-core/tests/test-window-utils.js b/packages/jetpack-core/tests/test-window-utils.js

>+exports.testActiveWindow = function(test) {
>+  test.waitUntilDone(5000);
>+
>+  let testRunnerWindow = Cc["@mozilla.org/appshell/window-mediator;1"]
>+                         .getService(Ci.nsIWindowMediator)
>+                         .getMostRecentWindow("");

Nit: this should probably be "" -> null too in case it ever matters.
Attachment #467164 - Flags: review?(myk) → review-
(In reply to comment #27)
> Comment on attachment 467164 [details] [diff] [review]
> Part 5 - Implement high-level window API - v3
> 
> So close!
> 
> 
> Note: the following hunk failed to apply when I applied the latest patch to a
> tip clone:

Don't know why it failed to apply. In any case, I've pulled the latest jetpack tip and refreshed this patch.

> >+This module currently only supports browser windows, and does not provide
> >+access to non-browser windows such as the bookmarks Library, preferences
> >+or other non-browser windows created via add-ons.
> 
> Nit: windows, and does not provide -> windows and does not provide
> Nit: bookmarks Library -> Bookmarks Library

fixed

> 
> 
> >+**Example**
> >+
> >+    var windows = require("windows");
> >+    for each (window in windows.browserWindows) {
> >+      console.log(window.title);
> >+    }
> >+
> >+    console.log(windows.browserWindows.length);
> 
> Nit: these docs demonstrate the length property but seem to be missing a
> description of it.

As talked on IRC, this is because we call this property as an array, so length is implied. Something to think about..

> 
> 
> >+If the only option being used is `url`, then a bare string URL can be passed to
> >+`open` instead of adding at a property of the `options` object.
> 
> Nit: adding at -> specifying it as

fixed
> 
> 
> >+@prop [url] {string}
> >+String URL to be opened in the new window.
> >+This is a required property.
> >+
> >+@prop [onOpen] {function}
> >+A callback function that is called when the window has opened. This does not
> >+mean that the URL content has loaded, only that the window itself is fully
> >+functional and its properties can be acessed. This is an optional property.
> 
> Nit: acessed -> accessed

fixed


> Nit: this example (and others) would be a bit simpler and easier to read if the
> module were required into a "windows" variable rather than a "browserWindows"
> variable.

Changed the examples to use var windows = require("windows").browserWindows

> 
> 
> >diff --git a/packages/jetpack-core/lib/windows.js b/packages/jetpack-core/lib/windows.js
> 
> >+  __iterator__: function() {
> >+    let enum = Cc["@mozilla.org/appshell/window-mediator;1"]
> >+               .getService(Ci.nsIWindowMediator)
> >+               .getEnumerator("navigator:browser");
> >+    while (enum.hasMoreElements())
> >+      yield windowMap.getWindowObject(enum.getNext());
> >+  },
> 
> Nit: apparently enum is a reserved identifier (making this generate a strict
> warning), so this should use a different name.

That's interesting. Changed it to winEnum

> 
> 
> >+    if (options.onOpen) {
> >+      addTabOptions.onLoad = function(e) {
> >+        let win = e.target.defaultView;
> >+        let safeWindowObj = windowMap.create(win);
> >+        let tabEl = win.gBrowser.tabContainer.childNodes[0];
> >+        let tabBrowser = win.gBrowser.getBrowserForTab(tabEl);
> >+        tabBrowser.addEventListener("load", function(e) {
> >+          tabBrowser.removeEventListener("load", arguments.callee, true);
> >+          errors.catchAndLog(function(e) options.onOpen(e))(safeWindowObj);
> >+        }, true);
> 
> options.onOpen should probably be called with safeWindowObj as the "this"
> object.

So we end up not removing the safeWindowObj as the first parameter to keep consistency with the tabs module. (Another thing to keep in mind)
> 
> 
> >+  onTrack: function(window) {
> >+    let safeWindowObj = windowMap.create(window);
> >+    if (!safeWindowObj)
> >+      return;
> >+    for (let callback in exports.browserWindows.onOpen) {
> >+      errors.catchAndLog(function(safeWindowObj) {
> >+        callback.call(exports, safeWindowObj);
> >+      })(safeWindowObj);
> >+    }
> >+  },
> >+  onUntrack: function(window) {
> >+    let safeWindowObj = windowMap.getWindowObject(window);
> >+    for (let callback in exports.browserWindows.onClose) {
> >+      errors.catchAndLog(function(safeWindowObj) {
> >+        callback.call(exports, safeWindowObj);
> >+      })(safeWindowObj);
> >+    }
> 
> Now that the object on which one specifies onOpen and onClose callback
> properties is exports.browserWindows, that should be the "this" object when
> executing those callbacks.

Right, changed

> 
> 
> >diff --git a/packages/jetpack-core/tests/test-window-utils.js b/packages/jetpack-core/tests/test-window-utils.js
> 
> >+exports.testActiveWindow = function(test) {
> >+  test.waitUntilDone(5000);
> >+
> >+  let testRunnerWindow = Cc["@mozilla.org/appshell/window-mediator;1"]
> >+                         .getService(Ci.nsIWindowMediator)
> >+                         .getMostRecentWindow("");
> 
> Nit: this should probably be "" -> null too in case it ever matters.

done
Attachment #467164 - Attachment is obsolete: true
Attachment #467219 - Flags: review?(myk)
Comment on attachment 467219 [details] [diff] [review]
Part 5 - Implement high-level window API - v4

>+        for (var i in tabBrowser)
>+          console.log(i);
>+        console.log("^");

Debugging cruft?

r=myk with that removed!  And a0.7=myk for checkin today.  We're in the freeze, but in conversations with Dietrich last night, we agreed that it would be worth making an exception to our normal freeze rules for this one patch, for which I'm currently holding the first RC.

(Also for folks following along or who may refer back to this bug in the future: such exceptions are not typically granted and should not be counted on during future freeze cycles!)
Attachment #467219 - Flags: review?(myk) → review+
(In reply to comment #28)
> > >+@prop [url] {string}
> > >+String URL to be opened in the new window.
> > >+This is a required property.

Brackets mean a @param or @prop is optional, so this should be:

  @prop url {string}
> Brackets mean a @param or @prop is optional, so this should be:
> 
>   @prop url {string}

fixed nit
Comment on attachment 466895 [details] [diff] [review]
Parts 1 - 4 merged

Hey Dietrich, did you mean to still review these patches combined, or did you finish review on the separate patches?
Attachment #466895 - Flags: review?(dietrich)
Comment on attachment 466895 [details] [diff] [review]
Parts 1 - 4 merged

Sorry, meant to mark this before. The tab code-move changes look fine, r=me.
Attachment #466895 - Flags: review?(dietrich) → review+
Priority: -- → P1
Target Milestone: -- → 0.8
Landed!
http://hg.mozilla.org/labs/jetpack-sdk/rev/afe8f5d1333a

I had to increase the timeout for the activeWindow tests a bit because they would occasionally fail. I guess there's really no way to make the tests reliable unless we expose a onActivate event for windows too (like the tabs API has)
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
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".
Component: Jetpack SDK → General
Product: Mozilla Labs → Add-on SDK
QA Contact: jetpack-sdk → general
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: