Closed Bug 396051 Opened 17 years ago Closed 12 years ago

Issues with scriptable IO API

Categories

(Core :: XPCOM, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: bzbarsky, Unassigned)

References

Details

Attachments

(1 file)

Attached patch Highlight issuesSplinter Review
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.
Flags: blocking1.9?
> 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).
Blocks: 380813
Oh, and an impl nit:  the float/double sizeof asserts should use PR_STATIC_ASSERT instead of runtime asserts.
(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.
> 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.

(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?
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.  ;)
(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?
> It does this for JS callers as well

Ah, neat.  Didn't realize that.
> 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.
> 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...
(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.
> 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.
not clear why this should block. API aesthetics certainly don't qualify. renominate if there is a more serious issue.
Flags: blocking1.9? → blocking1.9-
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.
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.
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?
Bug 399242 wants to back this out.
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?
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.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.