191327 - Make internal subset not include delimiting brackets

Status: RESOLVED FIXED

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

Description

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

Currently our internalSubset includes the delimiting brackets. I just noticed
that DOM 3 says they should not be there. Should we fix things? What do other
browsers do?

DOM Level 2 Core on internalSubset does not define what happens with brackets:

http://www.w3.org/TR/DOM-Level-2-Core/core.html#ID-412266927

DOM Level 3 Core says brackets are not included:

http://www.w3.org/TR/DOM-Level-2-Core/core.html#ID-412266927

Comment 1

[Ruslan Ismailov]

Both link was same and I try to find difference :-). Second link should be

http://www.w3.org/TR/DOM-Level-3-Core/core.html#ID-412266927

Comment 2

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

Doh! Thanks. Actually here are directs link to internalSubset:

http://www.w3.org/TR/DOM-Level-2-Core/core.html#ID-Core-DocType-internalSubset
http://www.w3.org/TR/DOM-Level-3-Core/core.html#ID-Core-DocType-internalSubset

Comment 3

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

Attachment: Possible fix

Contains also fix for bug 191482

Comment 4

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

Attachment: one that actually compiles...

Comment 5

[harishd]

Comment on attachment 116221 [details] [diff] [review]
one that actually compiles...

>Index: htmlparser/src/nsExpatDriver.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/htmlparser/src/nsExpatDriver.cpp,v
>+    mDoctypeText.SetLength(mDoctypeText.Length() - 1);
>+    mDoctypeText.Cut(0, 1);

Replace SetLength with Truncate. With that r=harishd

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 116221 [details] [diff] [review]
one that actually compiles...

- In nsExpatDriver.cpp:

     // The rest is the internal subset (minus whitespace)
     mDoctypeText.Trim(kWhitespace);
+    // And take out the brackets too
+    mDoctypeText.SetLength(mDoctypeText.Length() - 1);
+    mDoctypeText.Cut(0, 1);

     mInternalState = mSink->HandleDoctypeDecl(mDoctypeText, 
					       name, ...

mDoctypeText.Cut(0, 1) is rather expensive if there's more than a little text
in the doctype (which is not uncommon), how about this in stead:

+    // And take out the brackets too
+
+    const nsAString& substr = Substring(mDoctypeText, 1,
+					 mDoctypeText.Length() - 2);
+
     mInternalState = mSink->HandleDoctypeDecl(substr,
					       name, ...

Do we need to verify that mDoctypeText is at least 2 characters long here
before running the above code?

Please make this change before landing this.

Comment 7

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

Note to self: always remember Substring. Yeah, I'll make that change, thanks.

We probably need to check that length >= 2, so I'll make that change just in case.

Comment 8

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

Checking in content/base/src/nsXMLContentSerializer.cpp;
/cvsroot/mozilla/content/base/src/nsXMLContentSerializer.cpp,v  <--  nsXMLConten
tSerializer.cpp
new revision: 1.34; previous revision: 1.33
done
Checking in htmlparser/src/nsExpatDriver.cpp;
/cvsroot/mozilla/htmlparser/src/nsExpatDriver.cpp,v  <--  nsExpatDriver.cpp
new revision: 3.34; previous revision: 3.33
done
Checking in expat/xmlparse/xmlparse.c;
/cvsroot/mozilla/expat/xmlparse/xmlparse.c,v  <--  xmlparse.c
new revision: 1.25; previous revision: 1.24

I noticed that the serializer part of the patch was not exactly correct, fixed
that too before checkin in.

Comment 9

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

Should have been marked fixed.

Comment 10

[parish]

Heikki: Is it possible that the fix for this, or bug 191482, is the cause of bug
197477 ?

Note that the description for bug 197477 is misleading; the XML parser barfs on
© anywhere, not just Help->About Mozilla

Comment 11

[parish]

Re comment 10: No, it's not. Right file, wrong bug, bug 194240 was the one.