Talk:Io.js: Difference between revisions

From MozillaZine Knowledge Base
Jump to navigationJump to search
 
Line 219: Line 219:


:::::::Interesting. I didn't know JS had a GC (not all interpreters use GC, by any means--think of BASIC). Where did you learn about that? I'd like to learn about Mozilla's JS interpreter. --[[User:grimholtz|grimholtz]]
:::::::Interesting. I didn't know JS had a GC (not all interpreters use GC, by any means--think of BASIC). Where did you learn about that? I'd like to learn about Mozilla's JS interpreter. --[[User:grimholtz|grimholtz]]
::::::::I don't remember where I learned the fact that JS is GC'ed, probably from forums/newsgroups posts and bug comments. I haven't seen a great resource about JS yet. [[User:Asqueella|asqueella]]


===add to uninstall list===
===add to uninstall list===

Latest revision as of 02:25, 16 March 2005

asqueella's suggestions

I'd like to get point-by-point agree/disagree answer below from interested parties (grimholtz?), add long answers as subsections.

  • rename top-level ids -> FileIO2, DirIO2 to avoid conflicts with older code.
  • change Components.classes[this.dirservCID].createInstance(this.propsIID) -> getService (it is a service).
  • uncomment this ((?) I don't really see why that's commented): // data += uniConv.Finish();
  • remove DirIO.join/DirIO.split. File IO article has example for converting native path to URI and vice versa. I don't know how join/split can be used also, and this is not very clean XP code (hard-coding '\' for windows only doesn't seem smart).
  • rewrite FileIO.path()
  • move return false statements out of catch blocks
  • add dump() statements instead of // foobar!
  • personal: I would like the *CID/*IID members to be removed. I like the idea of creating wrappers for getService/createInstance, see below. This makes code much more readable IMO.
{
  createInstance: function(aCID, aIID) {
    return Components.classes["@mozilla.org/"+aCID]
          .createInstance(Components.interfaces[aIID]);
    },

  getService: function(aCID, aIID) {
    return Components.classes["@mozilla.org/"+aCID]
          .getService(Components.interfaces[aIID]);
  }
}
  • replace tab symbols with two-spaces. It is too wide for my 1024x768 screen now.
  • move examples from code to a separate section, add a link to io.js to the code.

me thinks about creating his own thin wrapper (like this for prefs) for file io...


@Nick - Why don't you add prefutils to as an article to KB like is.js? Also, do you want to use it for QuickNote? Might make things nicer and more module. --Jed 00:25, 13 Mar 2005 (PST)

Will think about it and open a bug or mail you about prefs in QN.
I wanted to write about that wrapper and about other wrappers too, but I haven't had time for that yet. asqueella

They're all great ideas, in my opinion, except "move return false statements out of catch blocks." I want to know when the operation has failed (but not through an exception). Also, don't forget to increment the version counter from 0.1 to 0.2, as well as placing your name in the header comments. In fact, it might be a good idea to have a version history section in the header. Up to you. I have a few other suggestions that I'll type here tonight (code fixes and enhancements).
--grimholtz 13:54, 10 Mar 2005 (EST).
I don't think the changelog should be in the reusable js file itself. We can create a changelog section/subpage though. asqueella


OK, but please include authors and contributors in the js file. --grimholtz 15:13 10 Mar 2005 (EST).

regarding returning false, I meant, change

try {
  return "useful value"
}
catch(e) {
  return false;
}

to

try {
  return "useful value"
} catch(e) {}
return false;
Sounds good! --grimholtz 15:14 10 Mar 2005 (EST).

Also I think returning false on failure isn't smart, null would be better. asqueella


hm, why? I like minimizing the number of types that can be returned because it simplifies code which uses the class --grimholtz 15:15 10 Mar 2005 (EST).
I mean return null in functions that return a string in case of success.
returning false for a function that returns a boolean value is of course correct.
asqueella
Sounds good --grimholtz 16:52 10 Mar 2005 (EST).

grimholtz's suggestions

These are the functions I would change and add to FileIO.

read/write taking path as string

First, I would add read() and write() function overloads which take a path/filename as a string. That way you don't have to continually write code this like:

var fileContents = DirIO.get("ProfD");
fileContents.append('extensions');
fileContents.append('{5872365E-67D1-4AFD-9480-FD293BEBD20D}');
fileContents.append('defaults');
fileContents.append(some-file.xml);
fileContents = FileIO.read(fileContents);

you could just write:

var fileContents = FileIO.read("ProfD/extensions/{5872365E-67D1-4AFD-9480-FD293BEBD20D}/defaults/some-file.xml");

Here's an example (this is untested):

  /**
   * readFiles takes a full path/filename as string and returns its contents.
   * First part of the path must be one of the special path names; e.g., ProfD.
   * You can separate paths with forward slash or backslash; it doesn't matter.
  readFile : function(fileAsString, charset) {

    fileAsString = fileAsString.replace("\\", "/", "g");
    var paths = fileAsString.split("/");
    // first part must be one of the "special" path names, like ProfD or TmpD
    // else DirIO.get() will fail.
    var f = DirIO.get(paths[0]);
    for (p in paths) {
      f.append(paths[p]);
    }
    return this.read(f, charset);
  },
Sounds great, but why do you want to include the support for forward slashes? Is it really useful to anybody? Also, wouldn't p in paths also append paths[0] to the file path?
Also any reason this have to be readFile, not getFile (ie. change that return with return f;)
asqueella
There are a few reasons why both forward and backslashes should be supported: (1) Some extension developers work on operating systems where they are accustomed to using forward slashes instead of backslashes for paths; we should be sensitive to that; (2) I think one of the main reasons the nsIFile interface prevents you from entering full paths (i.e., nsiFile.append() to drill down directories) is because "it is the only correct cross-platform way to specify a file. Strings are not such a way. If you grew up on windows or unix, you may think they are. Welcome to reality." With that in mind, we might try to accomodate *nix and Mac developers by permitting forward slashes.
You are right, p in paths would also append paths[0] to the file path. As I wrote above, this is untested code -- I wrote the function on-the-fly in the MediaWiki editor.
Returning f instead of this.read(f, charset) is fine with me, but I would like either (1) an option for this function to return the file contents or (2) another function called something like getFileContents() which returns this.read(f, charset). Come to think of it, if we have both of these functions, why do we need the original read() function at all? Similarly, we can get rid of the original write() function in place of the new one (which could take a full path/filename as a function argument, OR an nsILocalFile -- we can use typeof or some other means to determine the argument's type).
grimholtz
I meant backslashes, "\", of course.
So basically you're adding code bloat to help Windows/Linux developers feel like they are developing a Windows/Linux-only application. (Looks strange, in your comments below, you're arguing that replacing a="1"; if(cond) a="2" with a=cond ? "1":"2" gains performance win.)
I disagree, because I/O in mozilla is different from what Windows or Linux developers are accustomed to. I don't think we expect user input to be passed to this function, and devs can be taught to use forward slashes in string that is passed to this function.
Java supports both "\" and "/", and ever since my switch from C++ to Java, I learned to be grateful for that. It's little niceties like that which make day-to-day programming easier sometimes. It is a performance hit, but it makes the library more usable and user-friedly. If you are dead-set against it, I will yield and we can go with only "/".
grimholtz
Hm, I still don't agree with you here, but that's my preference vs. your preference, so if you add this, I won't be offended asqueella
If what you're suggesting is:
read: function(file, charset) {
  if(!(file instanceof C.i.nsIFile)) {
    file = ...getFile(file);
  }
   // current read code goes here
}
- then I like the idea. asqueella
Yes, I am. grimholtz

More changes to read/write

Now here are the changes I would make to existing functions:


  read   : function(file, charset) {
    var fiStream = null;
    var siStream = null;
    try {
      fiStream = Components.classes[this.finstreamCID]
                .createInstance(this.finstreamIID);
      siStream = Components.classes[this.sinstreamCID]
		.createInstance(this.sinstreamIID);
      fiStream.init(file, 1, 0, false);
      siStream.init(fiStream);
      var data = new String(siStream.read(-1));
      return charset ? this.toUnicode(charset, data) : data
    } 
    catch(e) {
      return false;
    }
    finally {
      if (siStream != null)
        siStream.close();
      if (fiStream != null)
        fiStream.close();
    }
},

write  : function(file, data, mode, charset) {
  var foStream = null;
  try {
    foStream = Components.classes[this.foutstreamCID]
	      .createInstance(this.foutstreamIID);
    if (charset) {
      data = this.fromUnicode(charset, data);
    }
    var flags = mode == 'a' ? 0x02 | 0x10 : // wronly | append
                              0x02 | 0x08 | 0x20; // wronly | create | truncate
				
    foStream.init(file, flags, 0664, 0);
    foStream.write(data, data.length);
    return true;
  }
  catch(e) {
    return false;
  }
  finally {
    if (foStream != null)
      foStream.close();
  }
},
In case anybody else is following this, the changes are stylistic plus added finally statement.
From my reading of the code though, calling close is not necessarily at all (the streams are closed in destructor). See nsFileStreams.cpp.
asqueella
The changes are not merely stylistic. They result in more efficient algorithms because there are less assignments, comparisons, and object allocations than the original code (although since I don't know anything about Mozilla's memory management, I don't know if the change in object allocations really matter; however, the change in assignments and comparisons certainly makes for a more efficient algorithm).
grimholtz
Well, no offence, but I don't really think changes like these in code that is called relatively rare will have any noticeable effect on CPU usage.
Could you look in the code and confirm/deny that calling close() is not needed? Then we won't need those try{}finally blocks. asqueella
I'd be glad to. Where should I look? In LXR? I'm not too familiar with the Mozilla source repository and how Mozilla maintains its source. grimholtz
I'll answer you by e-mail later. asqueella
Nevermind. Yes, you'd use LXR to look up nsFileStreams.cpp file name (as I've told you).
When did you tell me that? Anyway, I'm looking at it now. grimholtz
You're right; Close() is called in the dtor. We should at least write a comment about that in io.js, otherwise someone (like me) might think it's a bug to not use a finally block.
It 'is' a bug, see my comment below (about gc). Try it yourself - without close() the file is locked for some time.
OK --grimholtz
"When did you tell me that" - I meant I told you about nsFileStreams.cpp, not about lxr.
asqueella
But one thing I didn't think about is that GC may not happen immediately and the file will be unavailable until the filestream object is GD'ed. But anyways, we don't need to close both streams, since scriptableinputstream's close() closes the stream it wraps around.
Interesting. I didn't know JS had a GC (not all interpreters use GC, by any means--think of BASIC). Where did you learn about that? I'd like to learn about Mozilla's JS interpreter. --grimholtz
I don't remember where I learned the fact that JS is GC'ed, probably from forums/newsgroups posts and bug comments. I haven't seen a great resource about JS yet. asqueella

add to uninstall list

And here's another function I would add:

/**
 * Adds the given file to an extension's uninstall list
 * so the file doesn't linger after extension uninstallation.
 * The regular expression isn't correct (yet) so the function
 * doesn't prevent duplicate entries in the Uninstall log.
 * 
 */
addToUninstallList : function(file, extensionGUID) {

  var uninstallFile = DirIO.get("ProfD"); // %Profile% dir
  uninstallFile.append("extensions");
  uninstallFile.append(extensionGUID);
  uninstallFile.append("uninstall");
  uninstallFile.append("Uninstall");
  var data = FileIO.read(uninstallFile);
  // file.leafName actually isn't a sufficient check; we
  // should check the full path after 
  if (!data || new RegExp("^add\\s.*" + file.path, "m").test(data)) {
    // Uninstall file doesn't exist or the given file is
    // already listed in it.
    return;
  }
  // Add new file to the Uninstall list.
  // Note we're not using the FileIO.path() function
  // because it returns a URL-encoded path; e.g., ' ' = %20
  var newEntry = "add\t" + file.path + "\n";
  this.write(uninstallFile, newEntry, "a");
},
I'm not sure if it is a suitable addition to File I/O module. I think this functionality needs to be in the Extension manager service itself.
asqueella
Of course I agree with you, but it's not there and who knows when it will be? Maybe with QuickNote you don't want to delete the notes if someone uninstalls the extension, but in many other cases, any file which is created using write() should be deleted with the extension. We can leave it out if you like, and I'll just make it a KB article instead.
grimholtz
Yes put it in a separate article, please.
The reason I don't want to use techniques like this is that EM doesn't support any clean up procedures on extension uninstallation. That is, all prefs you set, all files you create, all entries you create in localstore.rdf - they all are left after uninstallation. The code above is a hack (because it modifies files managed by EM) and is still a partial solution, because it doesn't take care about prefs or localstore entries.
That's why I believe (and you seem to agree) that it should really be done by EM. Could you file a bug about this (a brief search hasn't found anything), note the bug # here, please.
btw, bug 258301 is about purging extension's prefs on uninstall asqueella
[update] looks like we may get "on-uninstall" notification in Firefox 1.1, see [1]
Done. Look here. Meanwhile, I'll write the article in the next couple days. grimholtz

even more on read/write methods

So do you think we should:

1. replace the original read() and write() with versions which takes strings?

2. leave the old methods and provide new ones which takes strings?

3. modify the original read() and write() so they take either nsiFile or strings?

I think we have agreed to go with #3 (see my code snippet above). asqueella

general js programming discussion

Also, can you explain two things? My lack of JS expertise is getting in the way. Firstly, it looks like this code:

if (typeof(File_IO_Module) != 'boolean') {
	var File_IO_Module = true;

Is a sort of javascript equivalent to the C/C++ pre-processor directives:

#ifndef __FILEIO_H
#define __FILEIO_H
#include <fileio.h>
#endif

am I right? If not, what is its purpose?

exactly that. asqueella
What happens in JS if the file is included more than once, besides the FileIO and DirIO variables being declaring more than once? grimholtz


Secondly, what is the advantage of doing the following:

var FileIO = {
...
...
}
var DirIO = {
...
...
}

instead of:

function FileIO() {}    // ctor
function FileIO_read(path) { ... }
function FileIO_wrote(path) { ... }
...
...
FileIO.prototype.read = FileIO_read;
FileIO.prototype.write = FileIO_write;

and then users of the class instantiate instances of it using new FileIO() just as they would any other JS object. Does one have benefit over the other, besides the latter being more object-oriented (no FileIO/DirIO static singleton).

--grimholtz

I don't understand your question. What is the advantage of your method over what's there now?
See, wrapping everything in objects is a common technique (which has nothing to do with "real" OOP), used as a substitution for namespaces. Since the methods of FileIO/DirIO are all static (operating on given in parameters nsIFile), it doesn't make any sense to allow creation of several instances of FileIO/DirIO. Think of it as of XPCOM services, if you like.
I really like the other way of writing thin wrappers, which I used in prefutils.js. (That's why I noted above that I may write Yet Another File Wrapper.) In that wrapper I define a constructor that (1) isntantiates an XPCOM component; (2) makes the created object inherit underlying XPCOM component's methods/properties (by modifying __proto__); (3) defines additional helper methods/properties on the object. If this wrapper was implemented that way, one would use it like this:
var file = new File("ProfD/extensions/extensions.rdf");
if(file.isFile()) { // call to nsIFile method
  alert(file.readContents("utf-8")); // call to wrapper's method
}
asqueella
I understand why functions and vars are wrapped in "objects" (to avoid naming collisions). My question was about the use of statics instead of non-statics.
In any case, I'd like to see your wrapper here instead of writing Yet Another File Wrapper...why don't we adopt that as the new io.js?
grimholtz
The main problem is - I don't know when that will happen. And when it will happen, the result won't have much in common with the original io.js. And I realize that not everybody *loves* that style of wrappers - so I don't think replacing MonkeeSage's module (while preserving name) is a good idea. asqueella

Next Step

So, is it time to put together our ideas and write version 0.2?

Go ahead. It's not my top priority to get my changes incorporated in the module, so I'd appreciate if you did it for me. asqueella