355358 - pnglets don't work anymore

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Martijn Wargers (dead)]

See url, you should see small images on that site of various forms (red rectangles, blue circles, etc), but they are not visible anymore with current trunk and current 1.8.1 branch builds.

This regressed on trunk between 2006-04-24 and 2006-04-26.
On the 1.8.1 branch this regressed between 2006-05-04 and 2006-05-06.
So it has to be a regression from bug 335298, somehow.

If you need a simplified testcase, then I'll try to make one.

Comment 1

[Ria Klaassen (not reading all bugmail)]

I see multiple times:

Error: pnglets is not defined
Source File: pnglets.line2
Line: 1

Comment 2

[Martijn Wargers (dead)]

(In reply to comment #1)

Hmm, yes, I think that's a different bug that manifests itself on trunk, I filed bug 355365 for it.

Comment 3

[Asaf Romano (gone)]

Error: uncaught exception: Load of javascript:pnglets.pnglet from http://www.elf.org/pnglets/#rectangle denied.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah.  So the problem is that the PNG data is NOT in fact UTF16, even though it's being passed around in a JSString.

I don't really see a way of making both this and non-ASCII HTML returned from javascript: work offhand; to make this work we need to treat each PRUnichar in the JSString as a single byte value (so need to decimate!), while to make non-ASCII HTML work we need to treat it as a Unicode char.

Perhaps we should decimate only if it's lossless or something like that?  That is, if all the PRUnichars only have the low 8 bits set?

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I was just going to suggest pretty much the same thing:  we can check if all the characters are either valid ISO-8859-1 or invalid Unicode < 255, and if so, use ISO-8859-1.  (Or did the Unicode spec recently define those characters as something?)

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The problem with doing that is that it looks to me like we set the channel on the stream before we have the data.  Is there a reasonably safe way to change that?

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

er, set the *charset* on the stream

Comment 8

[Brendan Eich [:brendan]]

Back in Netscape 2 and 3, JS used only ISO-Latin-1.  The extension to Unicode was straightforward in Netscape 4 (ah, the bliss of pretending Unicode chars were just 16 bits).  But the code that stored and fetched exactly one byte value per string character worked still, and that's the point.  We should still preserve backward compatibility.

/be

Comment 9

[Boris Zbarsky [:bzbarsky]]

You can call SetContentCharset on the channel any time you want to.  Doing so once we're giving it the data in this case should work fine.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

This won't block the release of Firefox 2.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch (against 1.8 branch)

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 241188 [details] [diff] [review]
patch (against 1.8 branch)

>Index: nsJSProtocolHandler.cpp
>+    else if (IsISO88591(result)) {

I wonder whether it makes sense to factor this a bit better.  something like:

  else {
    PRInt32 resultBytesLength;
    char* resultBytes;
    nsAutoString charset;
    if (IsISO88591(result)) {
      // use result.Length(), ToNewCString, ISO-8859-1
    } else {
      // UTF8
    }

    // NS_NewByteInputStream goes here, using resultBytes, etc.
  }

r=me for the patch as-is if we decide not to do that.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: trunk patch

This is a trunk patch, with the consolidation.  I can't test it against the pnglets testcase, though...

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 241393 [details] [diff] [review]
trunk patch

Looks good.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Checked in to trunk.

Comment 16

[Brendan Eich [:brendan]]

We shouldn't break the ability to handle data "packed" one byte per jschar in strings returned via javascript: URLs.  Renominating.

David, why did you nominate the trunk patch for 1.8.1?

/be

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It addresses the review comments.  I'll have to merge back to the branch, though.  I'll do that shortly.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: 1.8 branch patch

This is attachment 241393 [details] [diff] [review] merged back to the 1.8 branch, and tested with both pnglets and the testcase in bug 335298.

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Blocking for Fx2 RC3

Comment 20

[Mike Schroepfer]

Comment on attachment 241470 [details] [diff] [review]
1.8 branch patch

Approved for RC3.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Checked in to MOZILLA_1_8_BRANCH.

Comment 22

[Benjamin Smedberg]

Is there a way to unit-test this (yet)?

Comment 23

[Carsten Book [:Tomcat]]

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1) Gecko/20061010 Firefox/2.0 (RC3)

Images are now visible and no Errors in the Error Console