551348 - Review the file module, jetpack-core/lib/file.js

Status: RESOLVED FIXED

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

Description

[Drew Willcoxon :adw]

http://hg.mozilla.org/labs/jetpack-sdk/file/tip/packages/jetpack-core/lib/file.js

Module description from the docs:

The file module provides access to the local filesystem.

Comment 1

[Dietrich Ayala (:dietrich)]

Shaun, can you review this module?

Comment 2

[Dietrich Ayala (:dietrich)]

(In reply to comment #1)
> Shaun, can you review this module?

Er, that'd be *Shawn*.

Comment 3

[Shawn Wilsher :sdwilsh]

(In reply to comment #1)
> Shaun, can you review this module?
Yes, just dump it into my feedback/review queue.

Comment 4

[Dietrich Ayala (:dietrich)]

Attachment: snapshot

attachment is a snapshot. please review the code at the url in comment #0.

Comment 5

[Shawn Wilsher :sdwilsh]

Comment on attachment 439258 [details] [diff] [review]
snapshot

no vim/emacs modelines!  This should, in general, be following the style guide (it is new code, after all).

> * The Original Code is nsINarwhal.
Say what?

>// 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
>};
You don't have all the options that the file you mentioned contains.  Reasoning?

>
>var dirsvc = Cc["@mozilla.org/file/directory_service;1"]
>             .getService(Ci.nsIProperties);
>
>function MozFile(path) {
>  var file = Cc['@mozilla.org/file/local;1']
>             .createInstance(Ci.nsILocalFile);
>  file.initWithPath(path);
>  return file;
>}
Feels like you really want Components.constructor here:
var MozFile = Components.constructor("@mozilla.org/file/local;1",
                                     "nsILocalFile",
                                     "initWithPath");

then just do |new MozFile(path)| when you want to make one.
(see also https://developer.mozilla.org/en/Components.constructor)

Docs on what these exports are supposed to do would be super useful.  I'm having to guess what some of these methods are meant to do, which is no fun :(

>exports.read = function read(filename) {
>  var stream = exports.open(filename);
>  try {
>    var str = stream.read();
>  }
>  catch (err) {
>    throw err;
>  }
You really don't need to do this.  Just do the try-finally.  The exception will automatically be thrown anyway.

>exports.join = function join(base) {
>  if (arguments.length < 2)
>    throw new Error("need at least 2 args");
>  base = MozFile(base);
>  for (var i = 1; i < arguments.length; i++)
>    base.append(arguments[i]);
>  return base.path;
>};
I really don't know what this is trying to do.  What I think it's doing doesn't seem useful.

>exports.dirname = function dirname(path) {
>  return MozFile(path).parent.path;
>};
This could start to get expensive if they do dirname a bunch.

In general, the thing I hate most about this is all the fstats and other I/O that are being done synchronously :(  Can we make this API callback based?

Comment 6

[Atul Varma [:atul]]

(In reply to comment #5)
> > * The Original Code is nsINarwhal.
> Say what?

nsINarwhal is Irakli's Narwhal engine:

  http://narwhaljs.org/

I just copied the code and tidied it a bit.

> >// 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
> >};
> You don't have all the options that the file you mentioned contains. 
> Reasoning?

I think because they may be the only ones we need. I'm not even sure if I wrote that code myself or if Irakli or adw did, actually...

> Feels like you really want Components.constructor here:
> var MozFile = Components.constructor("@mozilla.org/file/local;1",
>                                      "nsILocalFile",
>                                      "initWithPath");
> 
> then just do |new MozFile(path)| when you want to make one.
> (see also https://developer.mozilla.org/en/Components.constructor)

Yeah, that would make sense; oddly I didn't even know about this way of doing things until yesterday.

> Docs on what these exports are supposed to do would be super useful.  I'm
> having to guess what some of these methods are meant to do, which is no fun :(

Sorry, those docs are actually in Markdown format, in '../docs/file.md' relative to the location of the JS file. Need to add that as another attachment to this bug so it can go through review too.

> >exports.join = function join(base) {
> >  if (arguments.length < 2)
> >    throw new Error("need at least 2 args");
> >  base = MozFile(base);
> >  for (var i = 1; i < arguments.length; i++)
> >    base.append(arguments[i]);
> >  return base.path;
> >};
> I really don't know what this is trying to do.  What I think it's doing doesn't
> seem useful.

It's just an equivalent of Python's os.path.join(), useful because the path separator is different on different platforms (windows is '\', unix is '/', etc).

> >exports.dirname = function dirname(path) {
> >  return MozFile(path).parent.path;
> >};
> This could start to get expensive if they do dirname a bunch.

If they do, could we profile and optimize later if needed, unless there's an easier fix?

> In general, the thing I hate most about this is all the fstats and otUher I/O
> that are being done synchronously :(  Can we make this API callback based?

Er, that would be nontrivial given the API's current consumers: for instance, the require() global needs to be synchronous and also needs to potentially load a file from the filesystem before returning. I'm wondering if it might be better to just have consumers of this API run in a separate thread, so they can just assume that file access is 'reasonably fast' without blocking the rest of Firefox's UI.

Comment 7

[Shawn Wilsher :sdwilsh]

(In reply to comment #6)
> Er, that would be nontrivial given the API's current consumers: for instance,
> the require() global needs to be synchronous and also needs to potentially load
> a file from the filesystem before returning. I'm wondering if it might be
> better to just have consumers of this API run in a separate thread, so they can
> just assume that file access is 'reasonably fast' without blocking the rest of
> Firefox's UI.
So, perhaps require() should be implemented with XPCOM sync stuff, but this publicly exported API not designed that way?  Using threads in JS is dangerous (I've tried it before but there are just too many issues with it in our platform still).

Comment 8

[Shawn Wilsher :sdwilsh]

(In reply to comment #7)
> So, perhaps require() should be implemented with XPCOM sync stuff, but this
> publicly exported API not designed that way?  Using threads in JS is dangerous
> (I've tried it before but there are just too many issues with it in our
> platform still).
To be clear, I'm advocating that we don't give developers an API that lets them easily shoot themselves in the foot (footguns are bad) in terms of performance.  We should give them the right API, even if that means we don't use it internally.

Comment 9

[Shawn Wilsher :sdwilsh]

As far as I can tell, this is done.

Comment 10

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