541622 - Implement byte streams

Status: RESOLVED FIXED

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

Description

[Drew Willcoxon :adw]

Attachment: patch 1

This patch adds binary stream classes ByteReader and ByteWriter.  It also adds file.open(), file.exists(), and file.remove().  The latter two are needed by the new tests.  I tried to conform to the CommonJS IO/A and Filesystem/A proposals.  There is lots more to those proposals, but I leave it for later patches.

Comment 1

[Atul Varma [:atul]]

Comment on attachment 423125 [details] [diff] [review]
patch 1

Looks like we are using javadoc/jsdoc style comments to document functions.

Note that this is supported by MDC Coding Style:

  https://developer.mozilla.org/En/Developer_Guide/Coding_Style

>+/**
>+ * A binary input stream.  The stream is backed by a Mozilla platform stream
>+ * that provides the underlying data, such as an nsIFileInputStream.
>+ *
>+ * @param backingStream
>+ *        A Mozilla platform stream object.
>+ */

I wonder if it'd be helpful to just go ahead and add the function name
to this comment, as it'd make it easier to read and for jsdoc-esque-tools
to parse.

Below, I am used to the catch being on the same line as the previous
closing bracket before it. Is that weird?

>+    }
>+    catch (err) {
>+      wrapAndThrowXpcomError(err);
>+    }
>+
>+    return data;
>+  };
>+}

Is there any way to format javadoc/jsdoc text as being in monospaced
font? Can we borrow from Markdown here or anything? In particular when
you mention 'str' below I wasn't sure if you were referring to the
word "string" or a variable called "str".

Also, I really like that this code doesn't put 'a' in front of every
argument name. That is cool.

>+  /**
>+   * Writes to the stream.  If the stream is closed, an exception is thrown.
>+   * begin and end are optional and control the portion of str that is output.
>+   * If neither is specified, str is output in its entirety.  If only begin is
>+   * specified, the suffix begining at that index is output.  If both are
>+   * specified, the range [begin, end) is output.
>+   *
>+   * @param str
>+   *        The string to write.

This is cool but we should probably move it to Cuddlefish's 'xpcom' module,
mayhaps?

>+/**
>+ * If err is an nsIException, throws a friendly version of it.  If there is no
>+ * friendly version or err is not an nsIException, err itself is thrown.
>+ *
>+ * @param err
>+ *        An exception.
>+ */
>+function wrapAndThrowXpcomError(err) {
>+  if (err instanceof nsIException) {
>+    switch (err.result) {
>+    case Cr.NS_BASE_STREAM_CLOSED:
>+      throw new Error("The stream is closed and cannot be read or written.");
>+    case Cr.NS_ERROR_FILE_IS_DIRECTORY:
>+      throw new Error("The stream was opened on a directory, which cannot be " +
>+                      "read or written.");
>+    }
>+  }
>+  throw err;

Some code style guides strictly specify that developers should only use one form
of commenting, C-style /* */ or C++-style //. The MDC Coding Style page doesn't
seem to make any distinction here; what do we want to do?

>+   // Flags passed when opening a file.  See nsprpub/pr/include/prio.h.
>+   const OPEN_FLAGS = {
>+     RDONLY: 0x01,
>+     WRONLY: 0x02,
>+     CREATE_FILE: 0x08,
>+     APPEND: 0x10,
>+     TRUNCATE: 0x20,
>+     EXCL: 0x80
>+   };
>+

Neat. But similarly, we may want to move this wrapAndThrowXpcomError()
function out to the 'xpcom' module. This code was written to be
includeable as a JS Module (.jsm) or even a script src tag, but we
could always go back on that (we'd have to if we added dependencies to
other modules via require(), though I notice you already do this in this
patch, by having this file require byte streams).

>    function ensureExists(file) {
>      if (!file.exists())
>-       throw new Error("path does not exist: " + file.path);
>+       wrapAndThrowXpcomError(Cr.NS_ERROR_FILE_NOT_FOUND, file.path);
>    }
> 
>+   function wrapAndThrowXpcomError(errOrResult, filename) {
>+     var result = errOrResult instanceof Ci.nsIException ?
>+                  errOrResult.result :
>+                  errOrResult;
>+     switch (result) {
>+     case Cr.NS_ERROR_FILE_NOT_FOUND:
>+       throw new Error("path does not exist: " + filename);
>+     }
>+     throw errOrResult;
>+   }

Interesting that you put the dot at the end of the same line as the
CC[], rather than at the beginning of the call to createInstance().
I've seen both on MDC snippets and would like to standardize on one
or the other.

>+       var stream = Cc['@mozilla.org/network/file-output-stream;1'].
>+                    createInstance(Ci.nsIFileOutputStream);

Comment 2

[Drew Willcoxon :adw]

I started a coding style guide and addressed those comments there.  Let's iterate on it.

https://wiki.mozilla.org/Labs/Jetpack/Reboot/Style_guide

(In reply to comment #1)
> This is cool but we should probably move it to Cuddlefish's 'xpcom' module,
> mayhaps?

Sounds good, I'll update the patch.

Comment 3

[Drew Willcoxon :adw]

Attachment: patch 2

Requesting review again with the following changes.

(In reply to comment #1)
> Neat. But similarly, we may want to move this wrapAndThrowXpcomError()
> function out to the 'xpcom' module.

I added xpcom.throwFriendlyError().  I don't like how callers of that method need to know how it works internally -- which options which errors take.  Any ideas?

> This code was written to be
> includeable as a JS Module (.jsm) or even a script src tag, but we
> could always go back on that (we'd have to if we added dependencies to
> other modules via require(), though I notice you already do this in this
> patch, by having this file require byte streams).

I removed file.js's ability to be loaded as a jsm or in a script tag.  Doing that for every module in Cuddlefish/Jetpack seems overly burdensome.  And how would it work when you need to require() something?  require'ing other modules to build upon them is one of the tenets of CommonJS and therefore Cuddlefish, no?

Comment 4

[Atul Varma [:atul]]

That looks great, thanks!

I'm not sure what to do about throwFriendlyError; it seems like there isn't an easy way out of this, since the friendly function's only other option is to inspect the local scope of its caller's stack frame or something, which is kind of hard (and would still require local variables to be named properly, thus not entirely getting around the situation).

The nice thing, though, is that thanks to our unit tests, we can opt to fix this issue at a later point.

Re: file.js being loaded as a jsm or in a script tag, that's fine, and only securable-module.js, cuddlefish.js, and file.js had that special property; the former two need it to be able to be "bootstrapped" from normal extensions, and at the time I thought file.js would be nice for normal extensions to use too if they needed it, but as we're bringing in dependencies w/ require() it's ok to have it require a CommonJS environment.

My only other concern about this patch now is regarding unloading: if/when the 'byte-streams' module is unloaded, e.g. when a reloadable extension using it is unloaded, we probably want any open streams to be closed, right? For comparison, the xhr.js module currently keeps track of all open requests and aborts them when it's unloaded.

Comment 5

[Drew Willcoxon :adw]

Attachment: patch 3

(In reply to comment #4)
> Re: file.js being loaded as a jsm or in a script tag, that's fine, and only
> securable-module.js, cuddlefish.js, and file.js had that special property; the
> former two need it to be able to be "bootstrapped" from normal extensions, and
> at the time I thought file.js would be nice for normal extensions to use too if
> they needed it, but as we're bringing in dependencies w/ require() it's ok to
> have it require a CommonJS environment.

I added this question to the best practices guide.  If we're at all considering making these modules available outside of Cuddlefish, we should make explicit rules.  We should also make helpers that automate exporting and require'ing.

> My only other concern about this patch now is regarding unloading: if/when the
> 'byte-streams' module is unloaded, e.g. when a reloadable extension using it is
> unloaded, we probably want any open streams to be closed, right? For
> comparison, the xhr.js module currently keeps track of all open requests and
> aborts them when it's unloaded.

Oops, yeah.  This patch creates a streamRegistry data structure, basically just a smart list.  xhr.js does the same thing I noticed.  Maybe it's worth abstracting out into a common helper?  We could get smarter and use a hash table, though we'd need some scheme for computing hash values.

Comment 6

[Atul Varma [:atul]]

Sweet, I will take a look at this soon.

I agree that a hash table would be great to do this with, but the ideal "hash value" to me is just a pointer to the object--which JS unfortunately gives us no way of keeping track of. I guess it's ok to make our own hash values for objects that we "own", but for other objects, e.g. navigator:browser Windows, IMO we really need to discourage mutating them by e.g. adding a "__jetpack_id" attribute, in part because it creates the potential for namespace collision with other Jetpacks.

For the time being I figured that doing O(n) lookup on a list in native code was ok, but I agree that a real hashtable would be nice. I've made a basic dictionary object that allows object pointers to be used as keys, which we could move into cuddlefish if we want:

  http://hg.mozilla.org/users/avarma_mozilla.com/atul-packages/file/747631f216f3/packages/proto-jetpack-2/lib/dictionary.js

The implementation, as you can see, is pretty dumb, just uses two lists to keep track of keys and values. But if we use the interface then we can optimize the implementation later.

Comment 7

[Atul Varma [:atul]]

Comment on attachment 424380 [details] [diff] [review]
patch 3

>+// On unload, close all currently opened streams.
>+require("unload").when(function byteStreams_unload() {
>+  streamRegistry.streams.forEach(function (s) s.close());
>+});

Since s.close() calls streamRegistry.unregister(), which mutates streamRegistry, maybe forEach() be run on a copy of the stream list, e.g.:

  streamRegistry.streams.slice().forEach(function (s) s.close());

I added a unit test for this sort of thing in my code too, but like you said, we should just put a generic registry helper like this in cuddlefish so we don't have to constantly rewrite and test this sort of code.

Maybe we should just commit your patch and make a new bug for creating the generic helper?

- Atul

Comment 8

[Drew Willcoxon :adw]

Attachment: patch 3.1

(In reply to comment #6)
> I agree that a hash table would be great to do this with, but the ideal "hash
> value" to me is just a pointer to the object--which JS unfortunately gives us
> no way of keeping track of. I guess it's ok to make our own hash values for
> objects that we "own", but for other objects, e.g. navigator:browser Windows,
> IMO we really need to discourage mutating them by e.g. adding a "__jetpack_id"
> attribute, in part because it creates the potential for namespace collision
> with other Jetpacks.

Agree.  If we're talking about only the small-scoped use case here, we can probably come up with something.

> track of keys and values. But if we use the interface then we can optimize the
> implementation later.

Concur.

(In reply to comment #7)
> (From update of attachment 424380 [details] [diff] [review])
> >+// On unload, close all currently opened streams.
> >+require("unload").when(function byteStreams_unload() {
> >+  streamRegistry.streams.forEach(function (s) s.close());
> >+});
> 
> Since s.close() calls streamRegistry.unregister(), which mutates
> streamRegistry, maybe forEach() be run on a copy of the stream list, e.g.:

That is embarrassing, yes. :\  The new patch uses a while loop instead.

> Maybe we should just commit your patch and make a new bug for creating the
> generic helper?

Sure, it's up to you.  There's no big rush to commit it, so if you want to do the helper first, we should do that.  I'll let you r+ if you want to commit, assuming it's OK.

Comment 9

[Atul Varma [:atul]]

Ok, I've decided to go ahead and commit it here:

  http://hg.mozilla.org/users/avarma_mozilla.com/jep-28/rev/fec9c60e8e3e

Will make a separate bug for the generic helper, since more code than just this patch will need to be converted to use it.

Comment 10

[Drew Willcoxon :adw]

Atul, next time please let me commit my own patches.  I was just asking for an r+ on this one.  Actually though, after working on bug 547417, which I emailed you about a couple of days ago, I'm no longer happy with this patch, and I didn't want to have it checked in.  Can you back out this patch and look at bug 547417 please?

Comment 11

[Atul Varma [:atul]]

Oh, sorry, I read "I'll let you r+ if you want to commit" and assumed I could go ahead and commit it...

Is there any way we could just commit this and make future bugs based on whatever is wrong with the patch?  The thing is, I actually needed to write code over the weekend that used some of the functionality in your patch, which is why I went ahead and committed it... Knowing that your patch is going to modify a number of things in file.js is basically blocking me from making any changes to it myself, which is why I just wanted to go ahead and commit it, then resolve any problems with it afterwards.

I guess unlike Firefox, committing code that has bugs in it is OK as long as bugs are created for them, since just having basic functionality at this point is better than having slightly buggy functionality, if that makes sense. Plus, if you "break the build" there won't actually be lots of people inconvenienced since not many folks are working on this right now.

Comment 12

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