241092 - make nsIStreamLoaderObserver usable from javascript

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

from bug 240920 comment 2
 ------- Additional Comment #2 From Christian Biesinger  2004-04-18 17:10 PDT 
[reply] -------

(From update of attachment 146449 [details] [diff] [review])
I'd also like to change the "result" parameter of the streamloaderobserver to
"[const,array,size_is(resultLength)]in octet result"

this bug is for that change :)

the current usage of |string| makes it impossible to use this method from
javascript, when 0-bytes or bytes with a set 8th bit are present.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

Comment 2

[Darin Fisher]

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

>Index: widget/src/gtk2/nsSound.cpp

>+#define GET_DWORD(s, i) (s[i+3] << 24) | \
>+    (s[i+2] << 16) | (s[i+1] << 8) | \
>+    s[i]

all on one line seems better:

#define GET_DWORD(s, i) (s[i+3] << 24) | (s[i+2] << 16) | (s[i+1] << 8) | s[i]

78 characters by my count =)


> NS_IMETHODIMP nsSound::OnStreamComplete(nsIStreamLoader *aLoader,
>                                         nsISupports *context,
>                                         nsresult aStatus,
>                                         PRUint32 stringLen,
>+                                        const PRUint8 *string)

hmm, maybe the variable should be named differently... like

  data _and_ dataLen

what'd you think?  (that way someone doesn't misuse it as a "string")


>     /* open up conneciton to esd */  
>     EsdPlayStreamFallbackType EsdPlayStreamFallback = 
>         (EsdPlayStreamFallbackType) PR_FindSymbol(elib, 
>                                                   "esd_play_stream_fallback");
>+    // XXX what if that fails?

maybe file a bug on this?


>Index: widget/src/gtk/nsSound.cpp

>+                                        const PRUint8 *string)

same nit about the name of this parameter.


>Index: widget/src/mac/nsSound.cpp

>+                                        const PRUint8 *stringData)

again, it's not really "string" data, right?


>Index: widget/src/os2/nsSound.cpp

>+                                        const PRUint8 *string)

same nit


>Index: widget/src/windows/nsSound.cpp

>+                                        const PRUint8 *string)

same nit


>+    // XXX we have sharable strings now! but the passed-in
>+    // data is not a string, it's an array of bytes

maybe this doesn't need to be a XXX comment anymore?


>+    theMM.PlaySound(NS_REINTERPRET_CAST(const PRUint8*, string), 0, flags);

isn't this cast redundant?


>Index: content/base/src/nsScriptLoader.cpp

>+                                 const PRUint8* string)

in this case "string" seems right.


>Index: content/xul/document/src/nsXULDocument.cpp

>+        // XXX this seems broken - what if the script is non-ascii?
>+        nsString stringStr; stringStr.AssignWithConversion(NS_REINTERPRET_CAST(const char*, string), stringLen);

maybe file a bug on this too since it seems like it could happen.


>Index: netwerk/base/src/nsStreamLoader.cpp

>     mObserver->OnStreamComplete(this, mContext, aStatus, 
>+                                mData.Length(), NS_REINTERPRET_CAST(const PRUint8*, mData.get()));

maybe bring the NS_REINTERPRET_CAST down to the next line to make it more
readable for those with narrow text editors :-)


r=darin (Thanks for doing this!)

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

filed Bug 241738 on nsSound crashing when esd symbols are unavaialble

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

filed Bug 241739 about nsXULDocument and non-ascii scripts

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #2)
> all on one line seems better:

Hm, indeed. it's also what gtk1 does.

> hmm, maybe the variable should be named differently... like
> 
>   data _and_ dataLen
> 
> what'd you think?  (that way someone doesn't misuse it as a "string")

Good idea, I'll change that.

> maybe this doesn't need to be a XXX comment anymore?

I decided to remove it (as sharable strings could only be relevant if this is
passed in as an ACString, and I don't see that happening, as that is no string)

> >+    theMM.PlaySound(NS_REINTERPRET_CAST(const PRUint8*, string), 0, flags);
> 
> isn't this cast redundant?

Er, yeah. I meant to cast to const char*. Will fix.

> >Index: netwerk/base/src/nsStreamLoader.cpp
> 
> >     mObserver->OnStreamComplete(this, mContext, aStatus, 
> >+                                mData.Length(), NS_REINTERPRET_CAST(const
PRUint8*, mData.get()));
> 
> maybe bring the NS_REINTERPRET_CAST down to the next line to make it more
> readable for those with narrow text editors :-)

changed to:
    mObserver->OnStreamComplete(this, mContext, aStatus, 
                                mData.Length(),
                                NS_REINTERPRET_CAST(const PRUint8*,
                                                    mData.get()));
to fit into 80 columns

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v2

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: camino patch

looks like camino also has an impl of nsIStreamLoaderObserver, this patch fixes
it

Comment 8

[Brian Ryner (not reading)]

Comment on attachment 147048 [details] [diff] [review]
patch v2

>--- widget/src/gtk2/nsSound.cpp	18 Apr 2004 22:00:17 -0000	1.9
>+++ widget/src/gtk2/nsSound.cpp	26 Apr 2004 14:04:38 -0000
>@@ -158,51 +158,51 @@ NS_IMETHODIMP nsSound::OnStreamComplete(
>     }
> #endif
> 
>     int fd, mask = 0;
>     unsigned long samples_per_sec=0, avg_bytes_per_sec=0;
>     unsigned long rate=0;
>     unsigned short format, channels = 1, block_align, bits_per_sample=0;
> 
>-    if (PL_strncmp(string, "RIFF", 4)) {
>+    if (memcmp(data, "RIFF", 4)) {

You've made this check case-sensitive on gtk 1/2 but not on other toolkits. 
Why?

Looks ok other than that.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #8)
> >-    if (PL_strncmp(string, "RIFF", 4)) {
> >+    if (memcmp(data, "RIFF", 4)) {
> 
> You've made this check case-sensitive on gtk 1/2 but not on other toolkits. 
> Why?

hm, indeed, I missed OS/2. I'll change that. but PL_strncmp is not
case-sensitive either.

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

checked in

Comment 11

[Mostafa Hosseini]

This has caused bug 242939.

The reason is the streamobserver used at:

http://lxr.mozilla.org/mozilla/source/calendar/resources/content/calendarManager.js#702

considers "result" to be a string.

Now that it is an octet array, how could it be converted to a string in JS?

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #11)
> considers "result" to be a string.

*sigh*
what if the data is UTF-16? or any non-ascii data?

> Now that it is an octet array, how could it be converted to a string in JS?

try: var my_string = String.fromCharCode.apply(this, result);