556940 - incorporate Atul's 'errors' and 'window-utils' modules and merge 'unload-2' into 'unload'

Status: RESOLVED FIXED

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

Description

[Myk Melez [:myk] [@mykmelez]]

We should incorporate the commonly-used helper modules that Atul has built and is hosting in a separate repo (like the window tracker) into the SDK.

Atul: is this something you can take on?

Comment 1

[Drew Willcoxon :adw]

The context menu module needs the window tracker.  I pulled it in but noticed it requires the "errors" and "unload-2" modules, and I don't know what their statuses are, or what packages they all should belong to.  Alternatively and at least during development I can use the tracker I wrote for the prototype's menu module.

Comment 2

[Atul Varma [:atul]]

Yep, just assigned this to myself.

Comment 3

[Atul Varma [:atul]]

Attachment: Added errors, window-utils, and merged unload-2 into unload. Memory leaks are likely present.

Comment 4

[Atul Varma [:atul]]

Drew, it looks like window-utils.js, errors, and unload-2 (which I merged into unload) are the bare minimum that we need here, which I've added an initial in-progress patch for as attachment 437455 [details] [diff] [review].  If there are more modules to add, we should probably make separate bugs for them (we might even need to separate this one out into 3 separate bugs that are each independently code reviewed, for window-utils, errors, and unload/unload-2).

window-utils seems like a really bad name for a module though, maybe call it just "window" instead, or is that *too* generic?

This patch also needs actual documentation for the new modules and functions.

Comment 5

[Atul Varma [:atul]]

Renamed the bug to reflect its current objectives more precisely.

Comment 6

[Atul Varma [:atul]]

Also, note that the newly-added unload.ensure() method in this patch may be enough to satisfy the needs of bug 551824.

Comment 7

[Drew Willcoxon :adw]

Comment on attachment 437455 [details] [diff] [review]
Added errors, window-utils, and merged unload-2 into unload. Memory leaks are likely present.

(In reply to comment #4)
> we should probably make separate bugs for them (we might even need to separate
> this one out into 3 separate bugs that are each independently code reviewed,
> for window-utils, errors, and unload/unload-2).

Depends on who reviews it I guess.  If we ask different people to review them all then yeah, but I don't think it's a problem otherwise.

> window-utils seems like a really bad name for a module though, maybe call it
> just "window" instead, or is that *too* generic?

Right now window-utils seems like an OK name, since windowIterator, WindowTracker, and closeOnUnload are all window-related utilities.  If it were called only "window", I would expect to find a class that models windows.  One thing is that "window" always means content windows to a lot of people, so maybe chrome-window-utils or chrome-windowS (plural)?

Also, since tabs and windows are analogous, it would be nice if window-utils's interface were similar to the SDK's tabs interface.

The patch looks good to me, just some nits:

>diff --git a/packages/jetpack-core/lib/errors.js b/packages/jetpack-core/lib/errors.js
>+var catchAndLog = exports.catchAndLog = function(callback,
>+                                                 defaultResponse,
>+                                                 logException) {
>+  if (!logException)
>+    logException = logToConsole;
>+
>+  return function() {
>+    try {
>+      return callback.apply(this, arguments);
>+    } catch (e) {
>+      logException(e);
>+      return defaultResponse;
>+    }
>+  };
>+};

try-catch-log is a common idiom in the code I've been writing, so it would be useful to provide a method that does only that, and then build your catchAndLog on top.  Except I would call the first method catchAndLog, and this one "wrapCatchLog" or something to denote that it returns a wrapper function.

>+exports.catchAndLogProps = function catchAndLogProps(object,
>+                                                     props,
>+                                                     defaultResponse,
>+                                                     logException) {

Again, I would use the word "wrap" or something for this, since "catch and log" implies that the callbacks will be called immediately.

>diff --git a/packages/jetpack-core/lib/unload.js b/packages/jetpack-core/lib/unload.js
>+var addMethod = exports.addMethod = function addMethod(obj, unloader) {
>+  var called = false;
>+
>+  function unloadWrapper() {
>+    if (!called) {
>+      called = true;
>+      var index = unloaders.indexOf(unloadWrapper);
>+      if (index == -1)
>+        throw new Error("assertion failure");

Could you make this msg more descriptive?  Also, it would be good to try-catch-log all callers of unloadWrapper, e.g., the when() call in this module, so one exception doesn't ruin everybody else.

>+when(
>+  function() {
>+    unloaders.slice().forEach(

Hmm, don't you need to pass 0 to slice?  If not, cool, I didn't know that.

>diff --git a/packages/jetpack-core/lib/window-utils.js b/packages/jetpack-core/lib/window-utils.js
>+var gWindowWatcher = Cc["@mozilla.org/embedcomp/window-watcher;1"]
>+                     .getService(Ci.nsIWindowWatcher);

What would you think about lazy-instantiating this, and globals like this in general?  I've been thinking about that in my own modules, since it seems wasteful to create it if no one ever require()s the module.  Maybe not a big deal.

>+WindowTracker.prototype = {

Awhile ago we talked about security concerns when referring to |this| in methods of exported classes.  (i.e., you can't rely on |this| being what you expect it to be.)  Is that still a concern?

>+function onDocUnload(event) {
>+  var index = gDocsToClose.indexOf(event.target);
>+  if (index == -1)
>+    throw new Error("assertion failure");

More descriptive message.

>+  gDocsToClose.splice(index, 1);
>+}

Could you also remove the unload event listener here?  To be on the safe side.

>diff --git a/packages/jetpack-core/tests/test-errors.js b/packages/jetpack-core/tests/test-errors.js
>+  // TODO: Add test to ensure that an array of props can be
>+  // passed in, rather than just a string.

Since you use this, would be good to test.

Comment 8

[Dietrich Ayala (:dietrich)]

All three modules are becoming quickly necessary, as we dive into JEP implementation. Can we fast-track these modules into the core, and iterate further on them there?

Comment 9

[Atul Varma [:atul]]

Sure, I will push it pronto.

Comment 10

[Atul Varma [:atul]]

(In reply to comment #7)
> Right now window-utils seems like an OK name, since windowIterator,
> WindowTracker, and closeOnUnload are all window-related utilities.  If it were
> called only "window", I would expect to find a class that models windows.  One
> thing is that "window" always means content windows to a lot of people, so
> maybe chrome-window-utils or chrome-windowS (plural)?

Good idea. Also, we should probably namespace all of these helper modules into "jetpack/chrome/" or something, so we don't pollute the module namespace with it.

> try-catch-log is a common idiom in the code I've been writing, so it would be
> useful to provide a method that does only that, and then build your catchAndLog
> on top.  Except I would call the first method catchAndLog, and this one
> "wrapCatchLog" or something to denote that it returns a wrapper function.

Good idea, yo. I like the convention of prefixing the wrappery functions with 'wrap' or a similar word, to imply that they're not immediate.

> >+  function unloadWrapper() {
> >+    if (!called) {
> >+      called = true;
> >+      var index = unloaders.indexOf(unloadWrapper);
> >+      if (index == -1)
> >+        throw new Error("assertion failure");
> 
> Could you make this msg more descriptive?

Sure, it really should never be raised--if it is, it means there's basically a bug in the unload module--not in the client developer's code--so it's basically like saying "assert index != -1". Would it be more helpful to just put a comment above the line of code? I sort of expect this only to be reported in a bug report, since if a developer sees the exception raised, there isn't much they can do about it themselves. Unless they want to help patch the platform.

If that makes sense. 

> Also, it would be good to
> try-catch-log all callers of unloadWrapper, e.g., the when() call in this
> module, so one exception doesn't ruin everybody else.

Mmm, good idea.

> >+when(
> >+  function() {
> >+    unloaders.slice().forEach(
> 
> Hmm, don't you need to pass 0 to slice?  If not, cool, I didn't know that.

Yep, slice() works. I kind of wish the method was called copy() or something.

> >diff --git a/packages/jetpack-core/lib/window-utils.js b/packages/jetpack-core/lib/window-utils.js
> >+var gWindowWatcher = Cc["@mozilla.org/embedcomp/window-watcher;1"]
> >+                     .getService(Ci.nsIWindowWatcher);
> 
> What would you think about lazy-instantiating this, and globals like this in
> general?  I've been thinking about that in my own modules, since it seems
> wasteful to create it if no one ever require()s the module.  Maybe not a big
> deal.

Wait... um... if no one ever require()s the module, then none of the code in the module will ever be executed. It's only at the time that someone require()s the module for the first time that its code read from disk, evaluated, and its exports object returned to the caller of require(). The only reason to bust a lazy loader for gWindowWatcher would be if the module was require()'d but only a few of its rarely-used functions used gWindowWatcher. Does that make sense?

> 
> >+WindowTracker.prototype = {
> 
> Awhile ago we talked about security concerns when referring to |this| in
> methods of exported classes.  (i.e., you can't rely on |this| being what you
> expect it to be.)  Is that still a concern?

Oh, since WindowTracker will only really be used by low-level jetpack APIs--our equivalent of "kernel modules"--security isn't much of an issue here, as the module will effectively only be "require()-able" by modules that have chrome privileges.

That said, I forget what the current status of 'this' in COWs is... Will need to check with Blake on that.

> >+  gDocsToClose.splice(index, 1);
> >+}
> 
> Could you also remove the unload event listener here?  To be on the safe side.

Good idea.

> >diff --git a/packages/jetpack-core/tests/test-errors.js b/packages/jetpack-core/tests/test-errors.js
> >+  // TODO: Add test to ensure that an array of props can be
> >+  // passed in, rather than just a string.
> 
> Since you use this, would be good to test.

Will do!

Comment 11

[Atul Varma [:atul]]

Pushed as per Dietrich's suggestion for fast-tracking this:

  Bug 556940 - incorporate Atul's 'errors' and 'window-utils' modules and merge 'unload-2' into 'unload'
  Atul Varma [:atul]
  http://hg.mozilla.org/labs/jetpack-sdk/rev/0d780a5ec0a9b31b40aa046fc1d2e0ace7e3f556

Will commit Drew's suggestions in the repository itself.

Should we create bugs to formally code-review these modules? I suppose this doesn't need to be done for unload.js--bug 551360 already covers this--but what about 'errors' and 'window-utils'?

Comment 12

[Drew Willcoxon :adw]

(In reply to comment #10)
> > Could you make this msg more descriptive?
> 
> Sure, it really should never be raised--if it is, it means there's basically a
> bug in the unload module--not in the client developer's code--so it's basically
> like saying "assert index != -1". Would it be more helpful to just put a
> comment above the line of code? I sort of expect this only to be reported in a
> bug report, since if a developer sees the exception raised, there isn't much
> they can do about it themselves. Unless they want to help patch the platform.
> 
> If that makes sense. 

Totally, but a couple of things:  If I'm a developer and I see that error, I have no idea what's gone wrong.  Did I do something?  Plus, it would help us if when people see this rare and elusive error we know immediately what it means, where it comes from.  Sure, we can look at a traceback, but every bit helps.  (I've been prefixing my unexpected errors with "Internal error".)

> Yep, slice() works. I kind of wish the method was called copy() or something.

Cool!  Yeah.

> Wait... um... if no one ever require()s the module, then none of the code in
> the module will ever be executed.

Oh.  OK, that plugs a giant hole in my understanding of the loader.

Comment 13

[Drew Willcoxon :adw]

Something else I just noticed is that windowIterator() isn't tested.  That function is really small and looks correct to me, but it would be good to test it for completeness.

Comment 14

[Drew Willcoxon :adw]

Also, since on instantiation WindowTracker tracks currently opened windows, that should be tested.  The current test only checks that windows opened after instantiation are tracked.

Comment 15

[Myk Melez [:myk] [@mykmelez]]

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