Last Comment Bug 396051 - Issues with scriptable IO API
: Issues with scriptable IO API
Status: RESOLVED WONTFIX
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: x86 Linux
: -- normal with 2 votes (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks: 380813
  Show dependency treegraph
 
Reported: 2007-09-13 10:11 PDT by Boris Zbarsky [:bz]
Modified: 2012-09-19 11:16 PDT (History)
12 users (show)
sayrer: blocking1.9-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Highlight issues (9.04 KB, patch)
2007-09-13 10:11 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2007-09-13 10:11:32 PDT
Created attachment 280759 [details] [diff] [review]
Highlight issues

I just tried using the scriptable IO API, and ran into the following issues:

* The stream API duplicates bits of the binary stream API.  Why not just
  inherit from that and add whatever bits are needed and reuse binary stream
  for the non-unichar stuff?
* The initWithStreams functions are underdocumented for both input and output
  scriptable io streams.
* There is no documentation about what works and what doesn't with unichar
  streams (a lot of things don't work, and worse yet do so _silently_).
* readString truncates at the first null, but doesn't document that.  Unclear
  whether this is a bug in the API or the documentation.  Attached patch
  assumes the latter.
* The nsIVariant arg to newInputStream/newOutputStream can't actually be a
  variant.  The implementation won't deal if it is.
* The "string" base documented for newOutputStream doesn't make any sense and
  in fact isn't implemented.

That's after about an hour and a half of trying to use this API, after which I gave up and just used binary streams, which work as documented.

Attaching a patch with some XXX comments and the readString change above.
Comment 1 Boris Zbarsky [:bz] 2007-09-13 10:13:15 PDT
> That's after about an hour and a half of trying to use this API,

So the point is, I didn't do a thorough API or impl review here; I just tried to use it and wrote down the issues I ran into.  One item I forgot in comment 0:

* writeBoolean doesn't document how many bytes it writes (compare readBoolean).
Comment 2 Boris Zbarsky [:bz] 2007-09-13 10:37:08 PDT
Oh, and an impl nit:  the float/double sizeof asserts should use PR_STATIC_ASSERT instead of runtime asserts.
Comment 3 Neil Deakin 2007-09-13 10:38:24 PDT
(In reply to comment #0)
> * The nsIVariant arg to newInputStream/newOutputStream can't actually be a
>   variant.  The implementation won't deal if it is.

Can you explain why?

> * The "string" base documented for newOutputStream doesn't make any sense and
>   in fact isn't implemented.

That looks a leftover bit of comment that should just be removed.
Comment 4 Boris Zbarsky [:bz] 2007-09-13 10:55:49 PDT
> Can you explain why?

Uh...  "How woudl it deal, exactly?"

More precisely, the implementation of newInputStream, say, will test whether |base| is instanceof nsIFile, nsITransport, nsIInputStream, or whether
 typeof(base) == "string".  For an nsIVariant, all those are false, so _wrapBaseWithInputStream will throw.

Comment 5 Neil Deakin 2007-09-13 11:07:43 PDT
(In reply to comment #4)
> > Can you explain why?
> 
> Uh...  "How woudl it deal, exactly?"
> 
> More precisely, the implementation of newInputStream, say, will test whether
> |base| is instanceof nsIFile, nsITransport, nsIInputStream, or whether
>  typeof(base) == "string".  For an nsIVariant, all those are false, so
> _wrapBaseWithInputStream will throw.
> 

It is implemented in a JS component, so the variant is "unboxed" and becomes an nsISupports. Unless I'm misunderstanding the issue?
Comment 6 Boris Zbarsky [:bz] 2007-09-13 11:15:14 PDT
Ah, I see.  XPConnect unwraps variants when calling from C++, and we don't really care about consumers who manually create variants in JS.  OK, ignore that part.  ;)
Comment 7 Neil Deakin 2007-09-13 11:20:30 PDT
(In reply to comment #6)
> Ah, I see.  XPConnect unwraps variants when calling from C++, and we don't
> really care about consumers who manually create variants in JS.  OK, ignore
> that part.  ;)
> 

It does this for JS callers as well, or are you loading the scriptableIO.js script yourself within a component?
Comment 8 Boris Zbarsky [:bz] 2007-09-13 11:51:18 PDT
> It does this for JS callers as well

Ah, neat.  Didn't realize that.
Comment 9 Neil Deakin 2007-09-13 17:33:54 PDT
> The stream API duplicates bits of the binary stream API.  Why not just
> inherit from that and add whatever bits are needed and reuse binary stream
> for the non-unichar stuff?

I originally had done this, but then changed my mind so that the string reading/writing APIs would be simpler. But I'm not too concerned either way.

> The initWithStreams functions are underdocumented for both input and output
> scriptable io streams.

Sure, but the documentation should really also say that one shouldn't call them directly.

> There is no documentation about what works and what doesn't with unichar
> streams (a lot of things don't work, and worse yet do so _silently_).

I had hoped to be able to fix up the inconsistencies with text streams earlier. Unfortunately, I haven't come up with a feasible solution short of reimplementing the unichar stream code.
Comment 10 Boris Zbarsky [:bz] 2007-09-13 22:57:52 PDT
> Sure, but the documentation should really also say that one shouldn't call them
> directly.

So the only supported use of this api is through nsIScriptableIO?  For my use (have am input stream, just want to read bytes from it in script) doing a stream createInstance followed by init is a lot simpler than getting a scriptable IO and then figuring out what args to pass to newInputStream...  I guess if I had an IO object already there it would be simpler to use it.  That's bug 396055.

As for the unichar thing, it would even help if the non-implemented stuff threw NS_ERROR_NOT_IMPLEMENTED...
Comment 11 Neil Deakin 2007-09-14 04:38:38 PDT
(In reply to comment #10)
> So the only supported use of this api is through nsIScriptableIO?  For my use
> (have am input stream, just want to read bytes from it in script) doing a
> stream createInstance followed by init is a lot simpler than getting a
> scriptable IO

It sounds like you just want to do that then, which is what existing code does. Unfortunately our frozen stream apis are so poor for general scripting use that is is difficult to create a simple api for it.
Comment 12 Boris Zbarsky [:bz] 2007-09-14 09:28:49 PDT
> It sounds like you just want to do that then, which is what existing code does.

I did, but my point is that it took a lot longer to figure out that I should do that because there was this other API that seemed to be designed to do what I wanted.... but just didn't quite do it as expected.

That's what bothers me about the API duplication with binary stream: it increases developer confusion to have multiple ways of getting the same functionality.  And I'm generally not that easily confused, even.
Comment 13 Robert Sayre 2007-10-09 17:35:13 PDT
not clear why this should block. API aesthetics certainly don't qualify. renominate if there is a more serious issue.
Comment 14 Boris Zbarsky [:bz] 2007-10-09 18:02:35 PDT
Well.  The API comments don't match what actually happens, and the API is new in 1.9.  It's better to not add it at all, at that point, in my opinion.
Comment 15 Sergey «Mithgol the Webmaster» Sokoloff 2008-01-30 11:41:01 PST
Even in its current state, this API really helps newbies writing extensions and thus should not be delayed until circa 2009 and Firefox 4 just because it's underdocumented. Documentation is a wiki, easy to improve; and the browser is not.
Comment 16 Boris Zbarsky [:bz] 2008-01-30 11:57:50 PST
For what it's worth, I had the exact opposite experience.  This API actually made it _slower_ for me to write code because of the state it was in.

And API documentation is not a wiki.  It's the IDL file.  But if you think it's easily improved, please write the correct docs that actually match what this code is doing?
Comment 17 Reed Loden [:reed] (use needinfo?) 2008-01-30 12:01:55 PST
Bug 399242 wants to back this out.
Comment 18 Wladimir Palant 2012-09-19 11:10:13 PDT
I found some confusing documentation on MDN linking to this bug. As far as I can tell, nsScriptableIO has long been superseded by FileUtils/NetUtil modules. Should this be WONTFIX?
Comment 19 Neil Deakin 2012-09-19 11:16:21 PDT
There's also https://developer.mozilla.org/en-US/docs/JavaScript_OS.File. None of them are actually replacements though, but I don't think it's likely the original scriptable io will go anywhere.

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