Last Comment Bug 380813 - Add better scripting support for files and streams
: Add better scripting support for files and streams
: dev-doc-complete
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: mozilla1.9alpha8
Assigned To: Neil Deakin
: Nathan Froyd [:froydnj]
Depends on: 403439 379140 382034 396051 396055 399242 414901
  Show dependency treegraph
Reported: 2007-05-15 16:02 PDT by Neil Deakin
Modified: 2011-10-04 08:50 PDT (History)
19 users (show)
enndeakin: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Implement scriptable IO (92.54 KB, patch)
2007-05-16 12:46 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
updated patch (93.88 KB, patch)
2007-06-04 12:11 PDT, Neil Deakin
mark.finkle: review+
benjamin: superreview+
Details | Diff | Splinter Review
patch to checkin (97.32 KB, patch)
2007-06-26 11:19 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
fix some minor issues (99.38 KB, patch)
2007-06-27 08:14 PDT, Neil Deakin
benjamin: review+
benjamin: superreview+
Details | Diff | Splinter Review
add installer files for scriptable io for browser and suite (2.84 KB, patch)
2007-07-31 10:43 PDT, Neil Deakin review+
neil: superreview+
dsicore: approval1.9+
Details | Diff | Splinter Review

Description Neil Deakin 2007-05-15 16:02:29 PDT
Not necessarily related to bug 380168, but add some better APIs for handing files and streams from script. The current plan is:

An IO global property accesible only from chrome can be used to create new file and stream references. Files will just be the nIFile implementations with class info. Streams will be a C++ implementation which just builds on top of the existing scriptableinputstream component which extra APIs for convenience. Similarly, a related scriptableoutputstream component will be created. See for more details.

Patch will be available shortly after I've finished testing.
Comment 1 Neil Deakin 2007-05-15 16:03:16 PDT
Also, I currently plan on putting the code in xpcom/io with the other IO code.
Comment 2 Neil Deakin 2007-05-16 12:46:54 PDT
Created attachment 265032 [details] [diff] [review]
Implement scriptable IO

Located in xpcom/io, but netwerk/base may be a better place. This code will need to be changed slightly for bug 379140. For streams, it is based on existing interfaces, nsISeekableStream, nsIBinaryInputStream, nsIConverterInputStream, etc. Unfortunately, when combined together, they don't make for a spectacular API. Suggestions welcome.
Comment 3 Alex Vincent [:WeirdAl] 2007-05-29 09:59:34 PDT
Comment on attachment 265032 [details] [diff] [review]
Implement scriptable IO

>+ok(IO, "IO is avaialble");


Just as a side thought, is it possible to rewrite the testcase as a xpcshell test?  I've found out the hard way xpcshell tests can run dramatically faster than mochitests.  The only thing you would need a window for would be to have IO available globally.  (Note peterv objected to me making XPathGenerator globally available because it wasn't standardized or anything.)

Can content pages access the IO object?  I know using the JS global property category, web pages could use my XPathGenerator, and I'd really hate to expose file access to remote sites.
Comment 4 Alex Vincent [:WeirdAl] 2007-05-29 10:02:10 PDT
Disregard my worry about content pages accessing IO.  I hadn't read bug 379140, which provides a way to prevent that.
Comment 5 Jason Barnabe (np) 2007-05-29 10:23:28 PDT
To me, "newFile" and friends sound like they're actually going to create a file, possibly overwriting an existing file. What about calling them "getFile" instead?
Comment 6 Nickolay_Ponomarev 2007-05-31 02:04:19 PDT
> The only thing you would need a window for would be to have
> IO available globally.

Speaking of which (haven't checked the patch yet), will this be available to JS-implemented XPCOM components?
Comment 7 Neil Deakin 2007-06-03 06:52:54 PDT
(In reply to comment #6)
> Speaking of which (haven't checked the patch yet), will this be available to
> JS-implemented XPCOM components?

Yes, but the IO object would be accessed via the usual Components.classes mechanism instead:

var sio = Components.classes[";1"].getService();
Comment 8 Neil Deakin 2007-06-04 12:11:24 PDT
Created attachment 267175 [details] [diff] [review]
updated patch 

Not sure really who should review this.
Comment 9 Benjamin Smedberg [:bsmedberg] 2007-06-12 13:40:15 PDT
Comment on attachment 267175 [details] [diff] [review]
updated patch 

I've lost review comments on this twice do to some update bug :-(. I've asked mfinkle to do an API review of this.

>Index: xpcom/io/nsIScriptableIO.idl

>+  /**
>+   * Creates a URI object which implements nsIURI. The url argument may either
>+   * be a string or a file.
>+   *
>+   * @param aUri the url to create
>+   * @returns a new nsIURI object
>+   * @throws NS_ERROR_INVALID_ARG when aUri is null
>+   */
>+  nsIURI newURI(in nsIVariant aUri);

It feels a little funny to have this in xpcom/. I'd prefer that this were in netwerk/, though I'm not going to be anal about it if there's a good reason to put it here.

>+  /**
>+   * Read aCount bytes from the stream and fill the aBytes array with
>+   * the bytes.
>+   *
>+   * @param aCount the number of bytes to read
>+   * @param aBytes [out] set to the array of read bytes
>+   */
>+  void readByteArray(in unsigned long aCount,
>+                     [array, size_is(aCount), retval] out octet aBytes);

I thought we preferred ACString over octet-arrays for binary data. 

>Index: xpcom/io/nsScriptableInputStream.cpp

>+  buffer = (char*)nsMemory::Alloc(count+1); // make room for '\0'

nit, please use NS_Alloc and NS_Free for all new code.

>+nsScriptableInputStream::Read(char* aData,
>+                              PRUint32 aCount,
>+                              PRUint32 *aReadCount)
>+  if (mUnicharInputStream) {
>+    // XXXndeakin implement this

This doesn't look good.

>+  // just call Read and convert to UTF-16
>+  char* cstr;
>+  nsresult rv = Read(aCount, &cstr);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  aString.Assign(NS_ConvertASCIItoUTF16(cstr));
>+  nsMemory::Free(cstr);
>+  return NS_OK;

You want nsXPIDLCString and getter_Copies here.

>+nsScriptableInputStream::ReadByteArray(PRUint32 aCount, PRUint8 **aBytes)
>+  char* s = NS_REINTERPRET_CAST(char*, nsMemory::Alloc(aCount));

Boy, I wish we had a smart-class for this, but I can't find one.

>+nsScriptableInputStream::Create(nsISupports *aOuter, REFNSIID aIID, void **aResult)
>+  if (aOuter) return NS_ERROR_NO_AGGREGATION;
>+  nsScriptableInputStream *sis = new nsScriptableInputStream();
>+  if (!sis) return NS_ERROR_OUT_OF_MEMORY;
>+  NS_ADDREF(sis);
>+  nsresult rv = sis->QueryInterface(aIID, aResult);
>+  NS_RELEASE(sis);
>+  return rv;
> }

Please use nsRefPtr to avoid the manual refcounting:

nsRefPTr<nsScriptableInputStream> sis = new nsScriptableInputStream();
if (!sis) return NS_ERROR_OUT_OF_MEMORY;
return sis->QueryInterface(aIID, aResult);

>Index: xpcom/io/nsScriptableOutputStream.cpp

>+nsScriptableOutputStream::Create(nsISupports *aOuter, REFNSIID aIID, void **aResult)

ditto re nsRefPtr
Comment 10 Mark Finkle (:mfinkle) (use needinfo?) 2007-06-17 23:09:16 PDT
Comment on attachment 267175 [details] [diff] [review]
updated patch 

The interface defined in "nsIScriptableIO.idl" looks good to me. r=mfinkle

I have some questions about the implementation:

>+// when using the directory service, map some extra keys that are
>+// easier to understand for common directories
>+var mappedDirKeys = {
>+  Working: "CurWorkD",
>+  Profile: "ProfD",
>+  Desktop: "Desk",
>+  Components: "ComsD",
>+  Temp: "TmpD",

Can we add | Application: "resource:app" | for the benefit of xulrunner apps? The question comes up from time to time as to how to get that folder.

>+function ScriptableIO() {
>+ScriptableIO.prototype =
>+  wrapBaseWithInputStream : function ScriptableIO_wrapBaseWithInputStream

Should these private methods start with an underscore?

>+var ScriptableIOFactory = {
>+  io: null,
>+  QueryInterface: function ScriptableIOFactory_QueryInterface(iid)
>+  {
>+    if (iid.equals(Ci.nsIFactory) ||
>+        iid.equals(Ci.nsISupports))
>+      return this;
>+  },
>+  createInstance: function ScriptableIOFactory_createInstance(aOuter, aIID)
>+  {
>+    if (aOuter != null)
>+      throw Components.results.NS_ERROR_NO_AGGREGATION;
>+    if (!
>+ = new ScriptableIO();
>+    return;
>+  }

In Bug 378618, we currently make the singleton ("io" in this case) a global variable to reduce the number of leaks.

>+    categoryManager.addCategoryEntry("JavaScript global property", "IO",
>+                                     SCRIPTABLEIO_CONTRACT_ID, true, true);
>+  },

Are you planning on making this be available to privileged code only using your patch in bug 379140 ?
Comment 11 Neil Deakin 2007-06-25 07:31:27 PDT
(In reply to comment #10)
> Are you planning on making this be available to privileged code only using your
> patch in bug 379140 ?


I've fixed the other issues.
Comment 12 Sergey «Mithgol the Webmaster» Sokoloff 2007-06-26 10:54:08 PDT
If you feel really convinced now that it's better to avoid calling something "newFile" if the file is not necessary new, and "getFile" looks better (i.e. less counterintuitive), then please update accordingly. I (myself) can see getFile in well enough, but I can imagine developers who can't discover these getXXXX in raw C++ and would rely on wiki newXXXX names instead.
Comment 13 Neil Deakin 2007-06-26 11:19:14 PDT
Created attachment 269883 [details] [diff] [review]
patch to checkin

Address issues above
Comment 14 Neil Deakin 2007-06-27 08:14:12 PDT
Created attachment 270011 [details] [diff] [review]
fix some minor issues

Moved some files into netwerk, add used the category names from 379140 

Found some issues in the test on Windows
 - added classinfo to QI code in nsLocalFileWin/nsLocalFileUnix
 - file size isn't updated so those tests are Mac only
 - disabled the permissions test for now
 - Home doesn't work on Windows without a environment variable set, so change the test to check the Desktop folder
Comment 15 Neil Deakin 2007-06-27 18:38:31 PDT
Not blocking but would be good to get this into beta1
Comment 16 Adam Guthrie 2007-07-25 10:22:51 PDT
This caused a leak on the Linux and OS X leak boxes:

nsLocalFile                                     248       3.33%
Comment 17 Adam Guthrie 2007-07-25 15:25:21 PDT
Looks like the leak was just a false positive, as Benjamin's patch for bug 388833 fixed this. (The nsLocalFile leak didn't appear, it just increased the number of bytes leaked probably due to adding a member or two to the class.)
Comment 18 Frank Wein [:mcsmurf] 2007-07-27 13:43:38 PDT
nsScriptableIO.js still needs to be added to the installer packages files right?
Comment 19 Neil Deakin 2007-07-27 16:29:45 PDT
(In reply to comment #18)
> nsScriptableIO.js still needs to be added to the installer packages files
> right?

No idea. What would need to happen here?
Comment 20 :Gavin Sharp [email:] 2007-07-27 16:36:50 PDT
nsScriptableIO.js needs to be added to browser/installer/unix/packages-static and browser/installer/windows/packages-static for Firefox. There are equivalent files for most other apps too, you might want to let them know about this addition at the very least (SeaMonkey, TB, Calendar, etc).
Comment 21 Frank Wein [:mcsmurf] 2007-07-27 16:38:32 PDT
Someone needs to create a patch which adds that file name to browser/installer/windows/packages-static and mail/installer/windows/packages-static and if he/she is nice also to calendar/installer/windows/packages-static and suite/installer/windows/packages.
Comment 22 Neil Deakin 2007-07-31 10:43:49 PDT
Created attachment 274646 [details] [diff] [review]
add installer files for scriptable io for browser and suite
Comment 23 Neil Deakin 2007-08-11 04:53:35 PDT
Checked in the installer files
Comment 24 Boris Zbarsky [:bz] (still a bit busy) 2007-09-13 10:16:31 PDT
> The interface defined in "nsIScriptableIO.idl" looks good to me. r=mfinkle

It does?  See bug 396051
Comment 25 Mark Finkle (:mfinkle) (use needinfo?) 2007-09-13 19:38:42 PDT
It does.

bug 396051 raises an API issue on inheriting the stream API from the binary stream API which is valid. I will say that recently I used ScriptableIO in a xulapp and did feel the documentation could be improved, but it certainly isn't a flawed API.
Comment 26 Eric Shepherd [:sheppy] 2007-10-04 11:12:43 PDT
I've documented nsIScriptableIO now, although I need to add an example.  What other interfaces need documenting before I can call this one fully documented?
Comment 27 Eric Shepherd [:sheppy] 2007-10-04 11:25:59 PDT
Now with sample courtesy of mfinkle and the knowledge that nsIFile is already documented, tagging this dev-doc-complete.
Comment 28 Neil Deakin 2007-10-09 08:06:16 PDT
(In reply to comment #26)
> I've documented nsIScriptableIO now

Comment 29 Mark Finkle (:mfinkle) (use needinfo?) 2007-10-09 08:10:04 PDT
(In reply to comment #28)
> (In reply to comment #26)
> > I've documented nsIScriptableIO now
> Where?
Comment 30 [:Aleksej] 2007-10-10 09:02:16 PDT
The fix may have caused bug 399242.
Comment 31 Jesse Ruderman 2008-01-31 12:06:41 PST
This was removed for Gecko 1.9.  See bug 414901.
Comment 32 Bill Gianopoulos [:WG9s] 2008-02-02 04:52:29 PST
Added bug 413439 mochitest for this fails on AMD-64 processor under Linux to blocker list for reference so it can be looked at when this is redone for next time.
Comment 33 David Baron :dbaron: ⌚️UTC-8 2008-02-05 13:34:25 PST
Should this be reopened so it can be done again later?

If so, note bug 399242 comment 2.
Comment 34 Eric Shepherd [:sheppy] 2008-02-05 15:19:16 PST
I've added a warning to the documentation for nsIScriptableIO that it has been removed from Firefox.

Note You need to log in before you can comment on or make changes to this bug.