361933 - [FIX]XML serializer should match "encoding" to actual encoding

Status: RESOLVED FIXED

(Core :: DOM: Serializers, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

Right now if I take an XML document that started out as ISO-8859-1 and serialize it as UTF8, we'll happily serialize encoding="ISO-8859-1" in the XML decl.  This seems pretty suboptimal, and in particular causes issues with XMLHttpRequest.  See the thread at http://lists.w3.org/Archives/Public/public-webapi/2006Nov/0040.html

I'll aim to fix the underlying bug here and the XMLHttpRequest issue in a separate bug.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Comment 2

[Boris Zbarsky [:bzbarsky]]

Bug 361934 covers the XMLHttpRequest end of things.

Comment 3

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

Comment on attachment 246649 [details] [diff] [review]
Fix

+   *                to a byte stream.  If this string is empty and root is
+   *                a document, the document's character set will be used.

what happens if I pass an empty string and a non-document root?

Comment 4

[Boris Zbarsky [:bzbarsky]]

We throw.  I can document that if desired.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 246649 [details] [diff] [review]
Fix

>@@ -997,17 +998,23 @@ nsXMLContentSerializer::AppendDocumentSt
> 
>   if (version.IsEmpty())
>     return NS_OK; // A declaration must have version, or there is no decl
> 
>   NS_NAMED_LITERAL_STRING(endQuote, "\"");
> 
>   aStr += NS_LITERAL_STRING("<?xml version=\"") + version + endQuote;
>   
>-  if (!encoding.IsEmpty()) {
>+  if (!mCharset.IsEmpty()) {
>+    aStr += NS_LITERAL_STRING(" encoding=\"") +
>+      NS_ConvertASCIItoUTF16(mCharset) + endQuote;
>+  }
>+  // Otherwise fall back on the document's encoding, I guess...
>+  // Though we don't really expect this to happen.
>+  else if (!encoding.IsEmpty()) {
>     aStr += NS_LITERAL_STRING(" encoding=\"") + encoding + endQuote;
>   }

Is this fallback ever really right (other than by just pure chance). Would it be better to not put an encoding attribute there at all rather one that's likely wrong?

r/sr=me either way

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Is this fallback ever really right (other than by just pure chance)

Not really.  In fact, we shouldn't even end up in this code if that member is empty...

I can just not put the attr there, sure.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to review comments

Comment 8

[Boris Zbarsky [:bzbarsky]]

Fixed.  I'll try to come up with a unit test for this.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Kinda silly unit test

This isn't checking that we actually used the specified encoding... I can't quite work out enough about getting non-ASCII data from JS into Gecko in a specified non-UTF8 encoding to do that right this second.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Slightly cleaner

Comment 11

[Boris Zbarsky [:bzbarsky]]

Checked in that test