Closed
Bug 380813
Opened 18 years ago
Closed 17 years ago
Add better scripting support for files and streams
Categories
(Core :: XPCOM, defect)
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)
(deleted),
patch
|
mfinkle
:
review+
benjamin
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
benjamin
:
review+
benjamin
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Gavin
:
review+
neil
:
superreview+
damons
:
approval1.9+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•18 years ago
|
||
Also, I currently plan on putting the code in xpcom/io with the other IO code.
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•18 years ago
|
||
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.
Assignee | ||
Updated•18 years ago
|
Target Milestone: --- → mozilla1.9alpha6
Comment 3•18 years ago
|
||
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.
Comment 4•18 years ago
|
||
Disregard my worry about content pages accessing IO. I hadn't read bug 379140, which provides a way to prevent that.
Comment 5•18 years ago
|
||
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•18 years ago
|
||
> 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?
Assignee | ||
Comment 7•18 years ago
|
||
(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();
Assignee | ||
Comment 8•18 years ago
|
||
Not sure really who should review this.
Attachment #265032 -
Attachment is obsolete: true
Attachment #267175 -
Flags: superreview?(benjamin)
Attachment #267175 -
Flags: review?(benjamin)
Comment 9•18 years ago
|
||
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 10•18 years ago
|
||
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+
Assignee | ||
Comment 11•18 years ago
|
||
(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.
Comment 12•18 years ago
|
||
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.
Assignee | ||
Comment 13•18 years ago
|
||
Address issues above
Assignee | ||
Comment 14•18 years ago
|
||
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)
Assignee | ||
Comment 15•18 years ago
|
||
Not blocking but would be good to get this into beta1
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
Updated•18 years ago
|
Attachment #270011 -
Flags: superreview?(benjamin)
Attachment #270011 -
Flags: superreview+
Attachment #270011 -
Flags: review?(benjamin)
Attachment #270011 -
Flags: review+
Comment 16•17 years ago
|
||
This caused a leak on the Linux and OS X leak boxes:
--NEW-LEAKS-----------------------------------leaks------leaks%
nsLocalFile 248 3.33%
Comment 17•17 years ago
|
||
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.)
Assignee | ||
Updated•17 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•17 years ago
|
Flags: in-testsuite+
Comment 18•17 years ago
|
||
nsScriptableIO.js still needs to be added to the installer packages files right?
Assignee | ||
Comment 19•17 years ago
|
||
(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•17 years ago
|
||
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•17 years ago
|
||
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.
Assignee | ||
Comment 22•17 years ago
|
||
Attachment #274646 -
Flags: superreview?(neil)
Attachment #274646 -
Flags: review?(gavin.sharp)
Updated•17 years ago
|
Attachment #274646 -
Flags: review?(gavin.sharp) → review+
Updated•17 years ago
|
Attachment #274646 -
Flags: superreview?(neil) → superreview+
Assignee | ||
Updated•17 years ago
|
Attachment #274646 -
Flags: approval1.9?
Updated•17 years ago
|
Attachment #274646 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 23•17 years ago
|
||
Checked in the installer files
Comment 24•17 years ago
|
||
> The interface defined in "nsIScriptableIO.idl" looks good to me. r=mfinkle
It does? See bug 396051
Comment 25•17 years ago
|
||
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.
Updated•17 years ago
|
Keywords: dev-doc-needed
Comment 26•17 years ago
|
||
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•17 years ago
|
||
Now with sample courtesy of mfinkle and the knowledge that nsIFile is already documented, tagging this dev-doc-complete.
Keywords: dev-doc-needed → dev-doc-complete
Assignee | ||
Comment 28•17 years ago
|
||
(In reply to comment #26)
> I've documented nsIScriptableIO now
Where?
Comment 29•17 years ago
|
||
(In reply to comment #28)
> (In reply to comment #26)
> > I've documented nsIScriptableIO now
>
> Where?
>
http://developer.mozilla.org/en/docs/nsIScriptableIO
Comment 30•17 years ago
|
||
The fix may have caused bug 399242.
Updated•17 years ago
|
Comment 31•17 years ago
|
||
This was removed for Gecko 1.9. See bug 414901.
Comment 32•17 years ago
|
||
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•17 years ago
|
||
Should this be reopened so it can be done again later?
If so, note bug 399242 comment 2.
Comment 34•17 years ago
|
||
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.
Description
•