641254 - file.open() and text-streams/byte-streams docs are confusing

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: Documentation, defect)

Description

[Nickolay_Ponomarev]

https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/docs/file.md says:

  > @returns {stream} A stream that can be used to access or modify
  > the contents of the file. See text-streams and byte-streams for
  > more information.

while the linked text-streams https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/docs/text-streams.md says:

  > The text-streams module provides streams for reading and writing text 

and then goes on to describe two classes TextReader/TextWriter, which are documented to take an 'inputStream' in their constructor.


The fact that a module claiming to provide "streams" only has constructors taking streams as parameters is interesting, but I didn't pay much attention to it.

The logical thing to do seemed to call TextWriter(file.open(.., "w")), since file.open() returns a stream, and TextWriter takes a stream. That fails in very non-obvious ways:
- Error: The stream is closed and cannot be used.' when calling method: [nsIOutputStream::close]"  nsresult: "0x8057001c (NS_ERROR_XPC_JS_THREW_JS_OBJECT
- Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIBufferedOutputStream.close]"
- [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIBufferedOutputStream.flush]"

I think that:
- file docs should specify the exact types of objects that are returned (TextReader/TextWriter/ByteReader/ByteWriter) and not call them streams.
- text-streams / byte-streams should probably not call themselves streams to avoid confusion with nsI streams. The docs should say that other APIs usually return the Text/ByteReader/Writer object and not the low-level (nsI...) streams.

Comment 1

[Nickolay_Ponomarev]

Apparently I'm not alone in interpreting docs like this: http://groups.google.com/group/mozilla-labs-jetpack/browse_thread/thread/92a0de7a8aca05e9

Comment 2

[Drew Willcoxon :adw]

Attachment: patch

I think this is less confusing than you and the other guy make it out to be.  The text-streams and byte-streams docs specifically say their constructors take nsI streams.  There's no handwaving about it.

I agree that file.open() should describe the types of objects it returns though.  That's what this patch does.

(In reply to comment #0)
> The fact that a module claiming to provide "streams" only has constructors
> taking streams as parameters is interesting, but I didn't pay much attention to
> it.

No, not really.  Streams encapsulating and consuming other streams is pretty commonplace.  Look no further than nsIBufferedInputStream, nsIBinaryOutputStream and other Gecko stream interfaces.

Comment 3

[Nickolay_Ponomarev]

Thanks, specifying the exact type of the object returned does make this much less confusing!

Comment 4

[Will Bamberg [:wbamberg]]

Comment on attachment 519590 [details] [diff] [review]
patch

Thanks Drew. That makes sense to me.

Comment 5

[Drew Willcoxon :adw]

https://github.com/mozilla/addon-sdk/commit/36af513a23cd6796e5c50fabc3c190829919e9ae