[webkit-reviews] review denied: [Bug 42365] HTML5 Parser: document.write in a asynchronous script which is specified to load before page finish blows away document : [Attachment 62338] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Adam Barth <abarth at webkit.org>'s
request for review:
Bug 42365: HTML5 Parser: document.write in a asynchronous script which is
specified to load before page finish blows away document
https://bugs.webkit.org/show_bug.cgi?id=42365

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
I'm having trouble understanding this patch.

LayoutTests/http/tests/misc/write-from-dom-script.html:9
 +  script.src =
"data:text/javascript,document.write('FAIL');document.close();";
It's sad that this script would "PASS" if your script had an error.  Would be
better if the script set some global that was also checked and could cause
FAIL.

LayoutTests/http/tests/misc/write-while-waiting.html:8
 +  setTimeout("document.write('PASS');document.close();", 100);
Would be nice if you could explain in the ChangeLog why this is right?	Maybe
you did and I didn't understand.

WebCore/dom/Document.cpp:366
 +	, m_isWriteNeutralised(false)
It's sad that this is on Document and not DocumentWriter or something else. 
Document is such a dumping ground.

WebCore/dom/Document.cpp:2020
 +	bool insertionPointIsUndefined = !m_parser ||
!m_parser->hasInsertionPoint();
This would have been more clear (to me) as:
bool hasInsertionPoint = m_parser && m_parser->hasInsertionPoint();

WebCore/dom/Document.cpp:2023
 +	if (insertionPointIsUndefined && isWriteNeutralised())
Then this becomes !hasInsertionPoint

WebCore/dom/Document.h:1005
 +	bool isWriteNeutralised() const { return m_isWriteNeutralised; }
I wish the spec called this "writeDisabled", Neutralized is a poor name IMO.
Hard to spell, hard to type, hard for our numerous non-english speakers to
understand.

WebCore/dom/DocumentParser.h:44
 +	virtual bool hasInsertionPoint() { return true; }
So what does this mean when true?  Who should override it?  If it's true,
you'll always get an insert call?

WebCore/dom/ScriptElement.cpp:59
 +	// If the element's Document has an active parser, and the parser's
script
This comment makes it seem like this should not go inside ScriptElement.

WebCore/dom/ScriptElement.cpp:61
 +	// "parser-inserted" flag set, the user agent must set the element's
Or maybe that the Document should be notified that the script was inserted and
it should make a decision to nueter itself or not.

WebCore/dom/ScriptElement.cpp: 
 +  static inline bool useHTML5Parser(Document* document)
This was previous dead code?

WebCore/dom/ScriptElement.cpp:203
 +	    // flag was set as being itself "write-neutralised". Let
neutralised doc
God this is an awful name.  I need to complain to Hixie.
Done: http://www.w3.org/Bugs/Public/show_bug.cgi?id=10228

Btw, my spelling corrector seems to think it's "neutralized" not "neutralised".
 Yet another reason why that name needs to go.


WebCore/html/HTMLDocumentParser.cpp:218
 +	if (m_scriptRunner && m_scriptRunner->inScriptExecution())
This needs a comment to explain why.  It's not clear why "inScirptExecution"
means there is an insertion point.  Why would that even be needed?  Can't we
check m_input?

WebCore/html/HTMLDocumentParser.cpp:220
 +	// FIXME: Checking whether we've seen end of file isn't quite right
when
I don't undersand this FIXME.  Can you explain better?