[FIXr]Implement nsBinaryOutputStream::WriteUtf8Z

RESOLVED FIXED in mozilla1.8beta2

Status

()

Core
XPCOM
P2
normal
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
mozilla1.8beta2
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

It seems to be well-documented and not very implemented.
Created attachment 181113 [details] [diff] [review]
Patch
Assignee: dougt → bzbarsky
Status: NEW → ASSIGNED
Attachment #181113 - Flags: superreview?(darin)
Attachment #181113 - Flags: review?(darin)
Priority: -- → P2
Summary: Implement nsBinaryOutputStream::WriteUtf8Z → [FIX]Implement nsBinaryOutputStream::WriteUtf8Z
Target Milestone: --- → mozilla1.8beta3

Comment 2

13 years ago
Comment on attachment 181113 [details] [diff] [review]
Patch

r+sr=darin

looks good.  we need to freeze this interface too :-)
Attachment #181113 - Flags: superreview?(darin)
Attachment #181113 - Flags: superreview+
Attachment #181113 - Flags: review?(darin)
Attachment #181113 - Flags: review+
OK. how does this implementation match the interface which says:

 79      * Write a NUL-terminated UTF8-encoded string to a binary stream, produced
 80      * from a NUL-terminated 16-bit PRUnichar* string argument.
 81      */
 82     void writeUtf8Z(in wstring aString);

It does not look like the written string is actually nul-terminated, since
str.Length() does not include the nul.

Additionally, this implementation differs from the other string-writing ones, in
that it does not write the length of the string to the stream. Of course, the
interface does not talk about that at all...

As it is, it seems to me like this implementation is inconsistent both with
respect to the interface and the other implementations.
OS: Linux → All
Hardware: PC → All

Comment 4

13 years ago
hmm, yeah.. comparing the API specs for WriteStringZ, WriteWStringZ, and
WriteUtf8Z, I'd have to agree with what you are saying.  It seems to me that
WriteUtf8Z should just call:

  WriteStringZ(NS_ConvertUTF16toUTF8(input).get());

That would seem to "Write a NUL-terminated [XXX] string to a binary stream" in a
consistent manner.

We should fixup the documentation to be more clear.  We should also document the
endianness of WriteWStringZ.  These details are important if people are going to
use these interfaces to manipulate binary data.
>  WriteStringZ(NS_ConvertUTF16toUTF8(input).get());

Yeah, you're right....

Note that once this has been done, there's no way to read the string back out of
the stream as Unicode, btw -- you have to readCString() and then convert from
UTF8 to UTF16 yourself....

> We should also document the endianness of WriteWStringZ. 

The whole interface is documented as writing in big-endian for all types.

Note that writing a "string" doesn't just write a string; this interface is not
designed for generally manipulating binary data but for being used with
nsIBinaryInputStream....
Created attachment 181188 [details] [diff] [review]
Updated patch
Attachment #181113 - Attachment is obsolete: true
Attachment #181188 - Flags: superreview?(darin)
Attachment #181188 - Flags: review?(cbiesinger)
Attachment #181188 - Flags: review?(cbiesinger) → review+

Updated

13 years ago
Attachment #181188 - Flags: superreview?(darin) → superreview+
Comment on attachment 181188 [details] [diff] [review]
Updated patch

Requesting 1.8b2 approval, since this is sorta an api change -- we implement
this method now.  ;)
Attachment #181188 - Flags: approval1.8b2?
Summary: [FIX]Implement nsBinaryOutputStream::WriteUtf8Z → [FIXr]Implement nsBinaryOutputStream::WriteUtf8Z

Comment 8

13 years ago
Comment on attachment 181188 [details] [diff] [review]
Updated patch

a=asa
Attachment #181188 - Flags: approval1.8b2? → approval1.8b2+
Fixed
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.8beta3 → mozilla1.8beta2
You need to log in before you can comment on or make changes to this bug.