120345 - Avoid mDoctypeText string in nsExpatDriver

Status: VERIFIED FIXED

(Core :: DOM: HTML Parser, defect, P1)

Description

[harishd]

Comment 1

[harishd]

Attachment: patch v1.0.

Comment 2

[harishd]

Attachment: patch v1.1 [ not too different from the above ]

Comment 3

[harishd]

--> 0.9.8.

This could have an impact on the xul window time.

Comment 4

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

Comment on attachment 65311 [details] [diff] [review]
patch v1.1 [ not too different from the above ]

>Index: htmlparser/src/nsExpatDriver.cpp
>===================================================================
>+static const PRUint32 kNotInDoctype = -1;

Doesn't this give a compiler warning? Either change that to PRInt32
or cast -1 to unsigned explicitly or something...

> nsExpatDriver::nsExpatDriver()
>   :mExpatParser(0), 

... I assume all the members are in declaration order...

>+  PRUint32 startOffset = ( mDoctypePos - mBytesParsed ) / 2;
>+  PRUint32 endOffset   = ( XML_GetCurrentByteIndex(mExpatParser) - mBytesParsed ) / 2;
>   
>+  const PRUnichar* doctypeStart = mBuffer + startOffset;
>+  const PRUnichar* doctypeEnd   = mBuffer + endOffset;
>+ 
>   if(mSink) {
>-    mInternalState = mSink->HandleDoctypeDecl(mDoctypeText.get());
>+    mInternalState = mSink->HandleDoctypeDecl(doctypeStart, doctypeEnd - doctypeStart);

I believe you can write the above more efficiently by avoiding
the variables.

>Index: htmlparser/src/nsHTMLTokenizer.cpp
>===================================================================
>-          aToken->SetEmpty(PR_TRUE);
>           isUsableAttr = (mFlags & NS_IPARSER_FLAG_VIEW_SOURCE)? PR_TRUE:PR_FALSE; // Fix bug 103095
>+          aToken->SetEmpty(isUsableAttr); // We need this for viewsource

It looks like this is fixing some other bug?

>Index: content/xml/tests/load/test.xml

Better test with some XMLHTTPRequest cases as well because this
document.load() moves the doctype around weirdly - dunno what else
it is doing...

>Index: content/xul/document/src/nsXULContentSink.cpp
>===================================================================
>-          aAttributes += 2;

What is this?

Comment 5

[harishd]

>+  PRUint32 startOffset = ( mDoctypePos - mBytesParsed ) / 2;
>+  PRUint32 endOffset   = ( XML_GetCurrentByteIndex(mExpatParser) -
mBytesParsed ) / 2;
> >+  const PRUnichar* doctypeStart = mBuffer + startOffset;
>+  const PRUnichar* doctypeEnd   = mBuffer + endOffset;
>+ 
> if(mSink) {
>-    mInternalState = mSink->HandleDoctypeDecl(mDoctypeText.get());
>+    mInternalState = mSink->HandleDoctypeDecl(doctypeStart, doctypeEnd -
doctypeStart);

>>I believe you can write the above more efficiently by avoiding
>>the variables.

Yes, that's what I had initially but it was not too easy to read.

>Index: content/xul/document/src/nsXULContentSink.cpp
>===================================================================
>-          aAttributes += 2;

>>What is this?
Found this bug. This was needed when there was a |while| loop but then when I
landed my changes I replaced the |while| with a |for| loop but forgot to remove
this line.

Comment 6

[harishd]

Attachment: patch v1.2 [ includes heikki's suggestions ]

Comment 7

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

Comment on attachment 65373 [details] [diff] [review]
patch v1.2 [ includes heikki's suggestions ]

r=heikki

There is still the nsHTMLTokenizer.cpp, which I seem to recall you do not
intend to check in right now (part of another bug).

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 65373 [details] [diff] [review]
patch v1.2 [ includes heikki's suggestions ]

sr=jst, modulo the changes that were not supposed to be in this patch
(nsHTMLTokenizer).

nsXMLContentSink::HandleDoctypeDecl could be futher optimized by removing the
nsAutoString's in favor of nsDependentString + iterators. Maybe file a new bug
on that?

Comment 9

[harishd]

--> 0.9.9

Comment 10

[harishd]

Txul improved bug ~1%.

FIXED.

Comment 11

[Moied]

Verified Fixed in cvs (Rev 1.3)