[Webkit-unassigned] [Bug 42365] HTML5 Parser: document.write in a asynchronous script which is specified to load before page finish blows away document

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=42365


Eric Seidel <eric at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #62338|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #9 from Eric Seidel <eric at webkit.org>  2010-07-22 16:05:49 PST ---
(From update of attachment 62338)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.