63558 - XML declaration not serialized

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[Heikki Toivonen (remove -bugzilla when emailing directly)]

<?xml ...> is missing from the output.

Comment 1

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Targeting for mozilla0.9.1

Comment 2

[Blake Ross]

Doesn't look like this is getting fixed before the freeze tomorrow night.
Pushing out a milestone.  Please correct if I'm mistaken.

Comment 3

[Alex Vincent [:WeirdAl]]

I assume this is why it doesn't show up in the DOM either...

Comment 4

[Heikki Toivonen (remove -bugzilla when emailing directly)]

We do not put XML declaration in the content model because strictly speaking DOM
does not know about an XML declaration (IE puts a processing instruction in its
place, and we could imitate that which is how I plan to fix this). We serialize
from the DOM so the serializer never even sees the XML declaration.

Comment 5

[Heikki Toivonen (remove -bugzilla when emailing directly)]

*** Bug 127300 has been marked as a duplicate of this bug. ***

Comment 6

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I think we can put the declaration infomation into the document (not making PI)
and write the declaration from there. This would be DOM compliant.

Missing declaration can make documemt non-wellformed, for example when the
encoding is not one of the defaults.

Comment 7

[Rakesh Mishra]

Changing Q/A contact

Comment 8

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: WIP, does not work yet

Comment 9

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: first working version

This patch seems to work. Ihave not done any performance tests yet, but we need
to check startup times - if they regressed (I doubt it but still) - we probably
need to get rid of the calls to GetQuotedAttributeValue() and just parse the
declaration ourselves (there might be some code sharing possible between
encoding detection in the parser and this code).

We don't seem to output a newline after the declaration. I think I need to fix
bug 158614 and/or bug 158617 to fix that.

Comment 10

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: testcase

Comment 11

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: fixes newlines moving from front of doctype to after doctype

Reviews? 

The newline moving problem is bug 158617, and the problem was missing
FlushText() in the beginning of HandleDoctypeDecl().

I decided against implementing DOM Level 3 methods in this bug, but it will be
easy to add later in a new bug. The implementation will most likely make us use
a tearoff object (same as with DOM 3 Node), and I don't think we should use
that internally because it needlessly slows us down (maybe we should make
internal APIs for Level 3 Node as well?). If you disagree, I can change the
internal APIs to DOM 3.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Hmm... Why would we use a tearoff for nsIDOM3Document?  We do this for nodes to
save on the extra vtable pointer, and if we have enough documents floating
around that the extra vtable pointer is a problem we have much bigger issues....

Comment 13

[Boris Zbarsky [:bzbarsky]]

Other comments:

> +  NS_ENSURE_TRUE(aLength >= 19, NS_ERROR_INVALID_ARG);

What's 19?  Is it some string's length?  If so:

NS_ENSURE_TRUE(aLength > NS_LITERAL_CSTRING("<?xml version=\"").Length(),
               NS_ERROR_INVALID_ARG);

This way the code is sorta self-documenting...

If it's something else, at least comment what it is.

> +  // XXX how to do dependent string?
> +  nsAutoString data(aData+6, aLength-8); // strip out "<?xml " and "?>"

nsAString& data = Substring(aData + 6, aData + aLength - 2);

should do it....

> +  if (aVersion.IsEmpty()) {
> +    mXMLDeclarationBits = 0;
> +    return NS_OK;
> +  }

How would nsXMLDocument::SetXMLDeclaration get called with an empty aVersion,
exactly?  Should this return an error (and maybe assert) instead?

The indentation of the args to this function is odd.

Should we consider always serializing out an encoding="whatever", even if the
original doc did not have an encoding set in the declaration?  We're already
setting "whatever" to the actual encoding as opposed to what the encoding in the
declaration was...

>   PRBool mCrossSiteAccessEnabled;
>+
>+  PRUint32 mXMLDeclarationBits;

Will we ever have anything close to 32 bits here?  Would a PRUint8 make sense? 
then the PRBool could become a PRPackedBool too....

The rest looks pretty good to me....

Comment 14

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Doh, sorry about my confusion on the tearoff.

I'll comment why 19 (I don't want to cause more code to execute which is why I
won't do the example you gave).

I think there needs to be some way to set the XML Declaration to nothing, so
calling with empty version seemed like a reasonable thing to be the reset.

If the endocing did not exist, and we were able to parse the doc, I don't think
we should generate it for output. It looks like DOM3 has properties for original
encoding and actual encoding, maybe these would store what was in the file and
what we think it is (I have not read the descriptions of those).

The PRBool is a minor thing; it could be made into PRPackedBool and moved up to
a group of other packed booleans, but this would mean we need at least one
temporary variable in one function call. Not a big thing, so I can do that.
Which means I can also pack the declaration, but I can't remember if this will
buy us anything at the moment.

Comment 15

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: fixed bzbarsky's issues

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 95001 [details] [diff] [review]
fixed bzbarsky's issues

Found a few nits.

- In nsXMLDocument::GetXMLDeclaration():

+  if (mXMLDeclarationBits & XML_DECLARATION_BITS_ENCODING_EXISTS) {
+    GetDocumentCharacterSet(aEncoding); // This is what we have stored, no
necessarily what was written in the original
+  }

s/no/not/ in the comment.

- In nsExpatDriver.cpp:

+    if (!mHandledXMLDeclaration && !mBytesParsed) {
+      static const PRUnichar xmlDecl[] = {'<', '?', 'x', 'm', 'l', ' ', '\0'};
+      if ( aLength >= 19 && // strlen("<?xml version='a'?>") == 19, shortest
decl
+	    nsCRT::strncmp(aValue, xmlDecl, 6) == 0 ) {

Please move the comment about 19 above the if statement. And loose the space
after the opening paren and before the closing paren.

sr=jst

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 95001 [details] [diff] [review]
fixed bzbarsky's issues

r=bzbarsky

Comment 18

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Ok, made jst's changes in my tree. I did startup perf tests on my Linux system.
Lots of variation, so the average slowdown of 0.2% is far from accurate. I'll
watch the Tinderbox when I check in, but I don't expect measurable differences.

Comment 19

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Checked in.

Comment 20

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

This patch looks like it broke the OS/2 build on Seamonkey-Ports because of an
extraneous |const|, here:

NS_IMETHODIMP
XULContentSinkImpl::HandleXMLDeclaration(const PRUnichar *aData, 
                                       const PRUint32 aLength)

(The second const shouldn't be there.  It shouldn't do any harm, either, but
OS/2 is confused about that bit.)

Comment 21

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Checked in a fix for the OS/2 bustage.