Closed Bug 380813 Opened 13 years ago Closed 12 years ago

Add better scripting support for files and streams

Categories

(Core :: XPCOM, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: enndeakin, Assigned: enndeakin)

References

(Depends on 1 open bug, )

Details

(Keywords: dev-doc-complete)

Attachments

(3 files, 2 obsolete files)

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 http://wiki.mozilla.org/ScriptableIO for more details.

Patch will be available shortly after I've finished testing.
Also, I currently plan on putting the code in xpcom/io with the other IO code.
Status: NEW → ASSIGNED
Attached patch Implement scriptable IO (obsolete) — Splinter Review
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.
Target Milestone: --- → mozilla1.9alpha6
Depends on: 382034
Comment on attachment 265032 [details] [diff] [review]
Implement scriptable IO

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

Typo.

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.
Disregard my worry about content pages accessing IO.  I hadn't read bug 379140, which provides a way to prevent that.
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?
> 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?
(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["@mozilla.org/io/scriptable-io;1"].getService();
Attached patch updated patch Splinter Review
Not sure really who should review this.
Attachment #265032 - Attachment is obsolete: true
Attachment #267175 - Flags: superreview?(benjamin)
Attachment #267175 - Flags: review?(benjamin)
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.

>+NS_IMETHODIMP
>+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.


>+NS_IMETHODIMP
>+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.

>+NS_METHOD
>+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
Attachment #267175 - Flags: superreview?(benjamin)
Attachment #267175 - Flags: superreview+
Attachment #267175 - Flags: review?(mark.finkle)
Attachment #267175 - Flags: review?(benjamin)
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;
>+    throw Cr.NS_ERROR_NO_INTERFACE;
>+  },
>+   
>+  createInstance: function ScriptableIOFactory_createInstance(aOuter, aIID)
>+  {
>+    if (aOuter != null)
>+      throw Components.results.NS_ERROR_NO_AGGREGATION;
>+
>+    if (!this.io)
>+      this.io = new ScriptableIO();
>+
>+    return this.io.QueryInterface(aIID);
>+  }
>+};

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 ?
Attachment #267175 - Flags: review?(mark.finkle) → review+
(In reply to comment #10)
> 
> Are you planning on making this be available to privileged code only using your
> patch in bug 379140 ?
> 

Yes.

I've fixed the other issues.
Depends on: 379140
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 http://wiki.mozilla.org/ScriptableIO accordingly. I (myself) can see getFile in https://bugzilla.mozilla.org/attachment.cgi?id=267175 well enough, but I can imagine developers who can't discover these getXXXX in raw C++ and would rely on wiki newXXXX names instead.
Attached patch patch to checkin (obsolete) — Splinter Review
Address issues above
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
Attachment #269883 - Attachment is obsolete: true
Attachment #270011 - Flags: superreview?(benjamin)
Attachment #270011 - Flags: review?(benjamin)
Not blocking but would be good to get this into beta1
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
Attachment #270011 - Flags: superreview?(benjamin)
Attachment #270011 - Flags: superreview+
Attachment #270011 - Flags: review?(benjamin)
Attachment #270011 - Flags: review+
This caused a leak on the Linux and OS X leak boxes:

--NEW-LEAKS-----------------------------------leaks------leaks%
nsLocalFile                                     248       3.33%
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.)
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
nsScriptableIO.js still needs to be added to the installer packages files right?
(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?
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).
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.
Attachment #274646 - Flags: superreview?(neil)
Attachment #274646 - Flags: review?(gavin.sharp)
Attachment #274646 - Flags: review?(gavin.sharp) → review+
Attachment #274646 - Flags: superreview?(neil) → superreview+
Attachment #274646 - Flags: approval1.9?
Attachment #274646 - Flags: approval1.9? → approval1.9+
Checked in the installer files
> The interface defined in "nsIScriptableIO.idl" looks good to me. r=mfinkle

It does?  See bug 396051
Depends on: 396051, 396055
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.
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?
Now with sample courtesy of mfinkle and the knowledge that nsIFile is already documented, tagging this dev-doc-complete.
(In reply to comment #26)
> I've documented nsIScriptableIO now

Where?
(In reply to comment #28)
> (In reply to comment #26)
> > I've documented nsIScriptableIO now
> 
> Where?
> 

http://developer.mozilla.org/en/docs/nsIScriptableIO
The fix may have caused bug 399242.
Blocks: 399242
Depends on: 414901
No longer blocks: 399242
Depends on: 399242
This was removed for Gecko 1.9.  See bug 414901.
Depends on: 403439
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.
Should this be reopened so it can be done again later?

If so, note bug 399242 comment 2.
I've added a warning to the documentation for nsIScriptableIO that it has been removed from Firefox.
You need to log in before you can comment on or make changes to this bug.