[webkit-reviews] review denied: [Bug 66406] New XML parser: scripting support : [Attachment 104229] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Jeffrey Pfau <jeffrey at endrift.com>'s
request for review:
Bug 66406: New XML parser: scripting support
https://bugs.webkit.org/show_bug.cgi?id=66406

Attachment 104229: Patch
https://bugs.webkit.org/attachment.cgi?id=104229&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=104229&action=review


> Source/WebCore/xml/parser/NewXMLDocumentParser.cpp:105
> +    if (scriptElement->prepareScript(m_scriptStartPosition,
ScriptElement::AllowLegacyTypeInTypeAttribute)) {
> +	   if (scriptElement->readyToBeParserExecuted())
> +	      
scriptElement->executeScript(ScriptSourceCode(scriptElement->scriptContent(),
document()->url(), m_scriptStartPosition));
> +	   else if (scriptElement->willBeParserExecuted()) {
> +	       m_pendingScript = scriptElement->cachedScript();
> +	       m_scriptElement = scriptElement->element();
> +	       m_pendingScript->addClient(this);
> +
> +	       // m_pendingScript will be 0 if script was already loaded and
addClient() executed it.
> +	       if (m_pendingScript)
> +		   pauseParsing();
> +	   } else
> +	       m_scriptElement = 0;
> +    }

Is this code copy/pasted from the HTML parser?	Can the code be shared instead?


> Source/WebCore/xml/parser/NewXMLDocumentParser.cpp:119
> -    return 0;
> +    return m_tokenizer->lineNumber() + 1;

The TextPosition types are supposed to save us from this kind of +1 madness. 
There should be some way for the C++ type system to do this work for you.

> Source/WebCore/xml/parser/NewXMLDocumentParser.cpp:131
> +    // Do some bookkeeping to make sure that re-appending doesn't mess up
the cursor

What is re-appending?  Why would we append the same string more than once?

> Source/WebCore/xml/parser/NewXMLDocumentParser.cpp:169
> +    if (m_parserPaused)
> +	   return;

Why can't you tell the parser to finish while it's paused?

> Source/WebCore/xml/parser/NewXMLDocumentParser.cpp:185
> -    return m_finishWasCalled;
> +    return m_parserPaused || m_finishWasCalled;

This is clearly wrong.	This function is just an accessor for
m_finishWasCalled.