2011-02-10 Andy Estes <aestes@apple.com>
authoraestes@apple.com <aestes@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 11 Feb 2011 00:45:11 +0000 (00:45 +0000)
committeraestes@apple.com <aestes@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 11 Feb 2011 00:45:11 +0000 (00:45 +0000)
        Reviewed by Darin Adler.

        HTML5 TreeBuilder regressed a Peacekeeper DOM test by 40%
        https://bugs.webkit.org/show_bug.cgi?id=48719

        The HTML5 fragment parsing algorithm specifies that a new Document
        should be created to serve as the temporary parent of fragment nodes
        during parsing. Document creation is expensive and accounts for ~38% of
        the Peacekeeper DOM performance regression. Avoid the cost of creating
        a dummy document by using the already-created DocumentFragment as the
        root node during fragment parsing.

        With this patch, the regression in Peacekeeper from Safari 5.0.3 to ToT
        is ~24%.

        Test: fast/parser/fragment-parser-doctype.html

        * dom/ContainerNode.h:
        (WebCore::ContainerNode::firstElementChild): Add a method that returns
        the first element-typed child from a ContainerNode.
        * dom/Document.cpp:
        (WebCore::Document::cacheDocumentElement): Call
        ContainerNode::firstElementChild() to retrieve and cache the document
        element.
        * html/parser/HTMLConstructionSite.cpp:
        (WebCore::HTMLConstructionSite::HTMLConstructionSite): Initialize the
        root ContainerNode.
        (WebCore::HTMLConstructionSite::detach): Clear the reference to the
        root ContainerNode.
        (WebCore::HTMLConstructionSite::insertHTMLHtmlStartTagBeforeHTML):
        Attach the new element to the root ContainerNode.
        (WebCore::HTMLConstructionSite::insertDoctype): Ditto.
        (WebCore::HTMLConstructionSite::insertCommentOnDocument): Ditto.
        * html/parser/HTMLConstructionSite.h: Store a pointer to a
        ContainerNode that will be used as the root node for document parsing.
        This node might or might not be the same as m_document.
        * html/parser/HTMLTreeBuilder.cpp:
        (WebCore::HTMLTreeBuilder::HTMLTreeBuilder): Initialize the
        HTMLConstructionSite with the correct root ContainerNode based on
        whether or not we're parsing a fragment.
        (WebCore::HTMLTreeBuilder::FragmentParsingContext::FragmentParsingContext):
        Remove m_dummyDocumentForFragmentParsing.
        (WebCore::HTMLTreeBuilder::FragmentParsingContext::finished): If the
        fragment has a context element, store only the children of the root
        element (HTML5 Section 10.4, Step 7).
        * html/parser/HTMLTreeBuilder.h:
2011-02-09  Andy Estes  <aestes@apple.com>

        Reviewed by Darin Adler.

        HTML5 TreeBuilder regressed a Peacekeeper DOM test by 40%
        https://bugs.webkit.org/show_bug.cgi?id=48719

        * fast/parser/fragment-parser-doctype-expected.txt: Added.
        * fast/parser/fragment-parser-doctype.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@78286 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/parser/fragment-parser-doctype-expected.txt [new file with mode: 0644]
LayoutTests/fast/parser/fragment-parser-doctype.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/ContainerNode.h
Source/WebCore/dom/Document.cpp
Source/WebCore/html/parser/HTMLConstructionSite.cpp
Source/WebCore/html/parser/HTMLConstructionSite.h
Source/WebCore/html/parser/HTMLTreeBuilder.cpp
Source/WebCore/html/parser/HTMLTreeBuilder.h

index 68df418..1528304 100644 (file)
@@ -1,3 +1,13 @@
+2011-02-09  Andy Estes  <aestes@apple.com>
+
+        Reviewed by Darin Adler.
+
+        HTML5 TreeBuilder regressed a Peacekeeper DOM test by 40%
+        https://bugs.webkit.org/show_bug.cgi?id=48719
+
+        * fast/parser/fragment-parser-doctype-expected.txt: Added.
+        * fast/parser/fragment-parser-doctype.html: Added.
+
 2011-02-10  Zhenyao Mo  <zmo@google.com>
 
         Unreviewed, test expectations update.
diff --git a/LayoutTests/fast/parser/fragment-parser-doctype-expected.txt b/LayoutTests/fast/parser/fragment-parser-doctype-expected.txt
new file mode 100644 (file)
index 0000000..a64d0f1
--- /dev/null
@@ -0,0 +1,9 @@
+Verify that a fragment's DOCTYPE does not affect parsing. We expect DOCTYPEs to be ignored for fragments with context elements.
+PASS container.firstChild.nextSibling is null
+Verify that a fragment's DOCTYPE does not change the compatibility mode of the owner document.
+PASS container.firstChild.nextSibling is null
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
+
diff --git a/LayoutTests/fast/parser/fragment-parser-doctype.html b/LayoutTests/fast/parser/fragment-parser-doctype.html
new file mode 100644 (file)
index 0000000..b96fa4f
--- /dev/null
@@ -0,0 +1,25 @@
+<html>
+<head>
+<link rel="stylesheet" href="../js/resources/js-test-style.css">
+<script src="../js/resources/js-test-pre.js"></script>
+</head>
+<body>
+<p id="description"></p>
+<div id="console"></div>
+<script>
+    debug("Verify that a fragment's DOCTYPE does not affect parsing. We expect DOCTYPEs to be ignored for fragments with context elements.");
+    var container = document.createElement("div");
+    document.body.appendChild(container);
+    container.innerHTML = "<!DOCTYPE html><p><table>"
+    shouldBeNull("container.firstChild.nextSibling");    
+</script>
+<p id="test"><table></table>
+<script>
+    debug ("Verify that a fragment's DOCTYPE does not change the compatibility mode of the owner document.");
+    var test = document.getElementById("test");
+    shouldBeNull("container.firstChild.nextSibling");
+    var successfullyParsed = true;
+</script>
+<script src="../js/resources/js-test-post.js"></script>
+</body>
+</html>
index 2ef77d6..8a77363 100644 (file)
@@ -1,3 +1,52 @@
+2011-02-10  Andy Estes  <aestes@apple.com>
+
+        Reviewed by Darin Adler.
+
+        HTML5 TreeBuilder regressed a Peacekeeper DOM test by 40%
+        https://bugs.webkit.org/show_bug.cgi?id=48719
+        
+        The HTML5 fragment parsing algorithm specifies that a new Document
+        should be created to serve as the temporary parent of fragment nodes
+        during parsing. Document creation is expensive and accounts for ~38% of
+        the Peacekeeper DOM performance regression. Avoid the cost of creating
+        a dummy document by using the already-created DocumentFragment as the
+        root node during fragment parsing.
+        
+        With this patch, the regression in Peacekeeper from Safari 5.0.3 to ToT
+        is ~24%.
+
+        Test: fast/parser/fragment-parser-doctype.html
+
+        * dom/ContainerNode.h:
+        (WebCore::ContainerNode::firstElementChild): Add a method that returns
+        the first element-typed child from a ContainerNode.
+        * dom/Document.cpp:
+        (WebCore::Document::cacheDocumentElement): Call
+        ContainerNode::firstElementChild() to retrieve and cache the document
+        element.
+        * html/parser/HTMLConstructionSite.cpp:
+        (WebCore::HTMLConstructionSite::HTMLConstructionSite): Initialize the
+        root ContainerNode.
+        (WebCore::HTMLConstructionSite::detach): Clear the reference to the
+        root ContainerNode.
+        (WebCore::HTMLConstructionSite::insertHTMLHtmlStartTagBeforeHTML):
+        Attach the new element to the root ContainerNode.
+        (WebCore::HTMLConstructionSite::insertDoctype): Ditto.
+        (WebCore::HTMLConstructionSite::insertCommentOnDocument): Ditto.
+        * html/parser/HTMLConstructionSite.h: Store a pointer to a
+        ContainerNode that will be used as the root node for document parsing.
+        This node might or might not be the same as m_document.
+        * html/parser/HTMLTreeBuilder.cpp:
+        (WebCore::HTMLTreeBuilder::HTMLTreeBuilder): Initialize the
+        HTMLConstructionSite with the correct root ContainerNode based on
+        whether or not we're parsing a fragment.
+        (WebCore::HTMLTreeBuilder::FragmentParsingContext::FragmentParsingContext):
+        Remove m_dummyDocumentForFragmentParsing.
+        (WebCore::HTMLTreeBuilder::FragmentParsingContext::finished): If the
+        fragment has a context element, store only the children of the root
+        element (HTML5 Section 10.4, Step 7).
+        * html/parser/HTMLTreeBuilder.h:
+
 2011-02-10  Beth Dakin  <bdakin@apple.com>
 
         Reviewed by Oliver Hunt.
index 76eb1bd..a5b23f4 100644 (file)
@@ -43,6 +43,7 @@ public:
 
     Node* firstChild() const { return m_firstChild; }
     Node* lastChild() const { return m_lastChild; }
+    ContainerNode* firstElementChild() const;
 
     bool insertBefore(PassRefPtr<Node> newChild, Node* refChild, ExceptionCode&, bool shouldLazyAttach = false);
     bool replaceChild(PassRefPtr<Node> newChild, Node* oldChild, ExceptionCode&, bool shouldLazyAttach = false);
@@ -169,6 +170,14 @@ inline Node* Node::lastChild() const
         return 0;
     return toContainerNode(this)->lastChild();
 }
+    
+inline ContainerNode* ContainerNode::firstElementChild() const
+{
+    Node* child = firstChild();
+    while (child && !child->isElementNode())
+        child = child->nextSibling();
+    return static_cast<ContainerNode*>(child);
+}
 
 } // namespace WebCore
 
index 31b7678..023428b 100644 (file)
@@ -705,10 +705,7 @@ void Document::childrenChanged(bool changedByParser, Node* beforeChange, Node* a
 void Document::cacheDocumentElement() const
 {
     ASSERT(!m_documentElement);
-    Node* n = firstChild();
-    while (n && !n->isElementNode())
-        n = n->nextSibling();
-    m_documentElement = static_cast<Element*>(n);
+    m_documentElement = static_cast<Element*>(firstElementChild());
 }
 
 PassRefPtr<Element> Document::createElement(const AtomicString& name, ExceptionCode& ec)
index c46b9b9..a026ef9 100644 (file)
@@ -130,10 +130,20 @@ void HTMLConstructionSite::attachAtSite(const AttachmentSite& site, PassRefPtr<N
         child->attach();
 }
 
-HTMLConstructionSite::HTMLConstructionSite(Document* document, FragmentScriptingPermission scriptingPermission, bool isParsingFragment)
+HTMLConstructionSite::HTMLConstructionSite(Document* document)
     : m_document(document)
+    , m_attachmentRoot(document)
+    , m_fragmentScriptingPermission(FragmentScriptingAllowed)
+    , m_isParsingFragment(false)
+    , m_redirectAttachToFosterParent(false)
+{
+}
+
+HTMLConstructionSite::HTMLConstructionSite(DocumentFragment* fragment, FragmentScriptingPermission scriptingPermission)
+    : m_document(fragment->document())
+    , m_attachmentRoot(fragment)
     , m_fragmentScriptingPermission(scriptingPermission)
-    , m_isParsingFragment(isParsingFragment)
+    , m_isParsingFragment(true)
     , m_redirectAttachToFosterParent(false)
 {
 }
@@ -145,6 +155,7 @@ HTMLConstructionSite::~HTMLConstructionSite()
 void HTMLConstructionSite::detach()
 {
     m_document = 0;
+    m_attachmentRoot = 0;
 }
 
 void HTMLConstructionSite::setForm(HTMLFormElement* form)
@@ -170,7 +181,7 @@ void HTMLConstructionSite::insertHTMLHtmlStartTagBeforeHTML(AtomicHTMLToken& tok
 {
     RefPtr<HTMLHtmlElement> element = HTMLHtmlElement::create(m_document);
     element->setAttributeMap(token.takeAtributes(), m_fragmentScriptingPermission);
-    m_openElements.pushHTMLHtmlElement(attach<Element>(m_document, element.get()));
+    m_openElements.pushHTMLHtmlElement(attach<Element>(m_attachmentRoot, element.get()));
 #if ENABLE(OFFLINE_WEB_APPLICATIONS)
     element->insertedByParser();
 #endif
@@ -205,7 +216,16 @@ void HTMLConstructionSite::insertHTMLBodyStartTagInBody(AtomicHTMLToken& token)
 void HTMLConstructionSite::insertDoctype(AtomicHTMLToken& token)
 {
     ASSERT(token.type() == HTMLToken::DOCTYPE);
-    attach(m_document, DocumentType::create(m_document, token.name(), String::adopt(token.publicIdentifier()), String::adopt(token.systemIdentifier())));
+    attach(m_attachmentRoot, DocumentType::create(m_document, token.name(), String::adopt(token.publicIdentifier()), String::adopt(token.systemIdentifier())));
+    
+    // DOCTYPE nodes are only processed when parsing fragments w/o contextElements, which
+    // never occurs.  However, if we ever chose to support such, this code is subtly wrong,
+    // because context-less fragments can determine their own quirks mode, and thus change
+    // parsing rules (like <p> inside <table>).  For now we ASSERT that we never hit this code
+    // in a fragment, as changing the owning document's compatibility mode would be wrong.
+    ASSERT(!m_isParsingFragment);
+    if (m_isParsingFragment)
+        return;
     
     if (token.forceQuirks())
         m_document->setCompatibilityMode(Document::QuirksMode);
@@ -222,7 +242,7 @@ void HTMLConstructionSite::insertComment(AtomicHTMLToken& token)
 void HTMLConstructionSite::insertCommentOnDocument(AtomicHTMLToken& token)
 {
     ASSERT(token.type() == HTMLToken::Comment);
-    attach(m_document, Comment::create(m_document, token.comment()));
+    attach(m_attachmentRoot, Comment::create(m_document, token.comment()));
 }
 
 void HTMLConstructionSite::insertCommentOnHTMLHtmlElement(AtomicHTMLToken& token)
index 5a4a65d..0298503 100644 (file)
@@ -43,7 +43,8 @@ class Element;
 class HTMLConstructionSite {
     WTF_MAKE_NONCOPYABLE(HTMLConstructionSite);
 public:
-    HTMLConstructionSite(Document*, FragmentScriptingPermission, bool isParsingFragment);
+    HTMLConstructionSite(Document*);
+    HTMLConstructionSite(DocumentFragment*, FragmentScriptingPermission);
     ~HTMLConstructionSite();
 
     void detach();
@@ -130,6 +131,12 @@ private:
     void dispatchDocumentElementAvailableIfNeeded();
 
     Document* m_document;
+    
+    // This is the root ContainerNode to which the parser attaches all newly
+    // constructed nodes. It points to a DocumentFragment when parsing fragments
+    // and a Document in all other cases.
+    ContainerNode* m_attachmentRoot;
+    
     RefPtr<Element> m_head;
     RefPtr<HTMLFormElement> m_form;
     mutable HTMLElementStack m_openElements;
index f2461ca..e1498fd 100644 (file)
@@ -342,7 +342,7 @@ private:
 HTMLTreeBuilder::HTMLTreeBuilder(HTMLDocumentParser* parser, HTMLDocument* document, bool reportErrors, bool usePreHTML5ParserQuirks)
     : m_framesetOk(true)
     , m_document(document)
-    , m_tree(document, FragmentScriptingAllowed, false)
+    , m_tree(document)
     , m_reportErrors(reportErrors)
     , m_isPaused(false)
     , m_insertionMode(InitialMode)
@@ -360,8 +360,8 @@ HTMLTreeBuilder::HTMLTreeBuilder(HTMLDocumentParser* parser, HTMLDocument* docum
 HTMLTreeBuilder::HTMLTreeBuilder(HTMLDocumentParser* parser, DocumentFragment* fragment, Element* contextElement, FragmentScriptingPermission scriptingPermission, bool usePreHTML5ParserQuirks)
     : m_framesetOk(true)
     , m_fragmentContext(fragment, contextElement, scriptingPermission)
-    , m_document(m_fragmentContext.document())
-    , m_tree(m_document, scriptingPermission, true)
+    , m_document(fragment->document())
+    , m_tree(fragment, scriptingPermission)
     , m_reportErrors(false) // FIXME: Why not report errors in fragments?
     , m_isPaused(false)
     , m_insertionMode(InitialMode)
@@ -375,7 +375,6 @@ HTMLTreeBuilder::HTMLTreeBuilder(HTMLDocumentParser* parser, DocumentFragment* f
     if (contextElement) {
         // Steps 4.2-4.6 of the HTML5 Fragment Case parsing algorithm:
         // http://www.whatwg.org/specs/web-apps/current-work/multipage/the-end.html#fragment-case
-        m_document->setCompatibilityMode(contextElement->document()->compatibilityMode());
         processFakeStartTag(htmlTag);
         resetInsertionModeAppropriately();
         m_tree.setForm(closestFormAncestor(contextElement));
@@ -404,27 +403,24 @@ HTMLTreeBuilder::FragmentParsingContext::FragmentParsingContext()
 }
 
 HTMLTreeBuilder::FragmentParsingContext::FragmentParsingContext(DocumentFragment* fragment, Element* contextElement, FragmentScriptingPermission scriptingPermission)
-    : m_dummyDocumentForFragmentParsing(HTMLDocument::create(0, KURL(), fragment->document()->baseURI()))
-    , m_fragment(fragment)
+    : m_fragment(fragment)
     , m_contextElement(contextElement)
     , m_scriptingPermission(scriptingPermission)
 {
-    m_dummyDocumentForFragmentParsing->setCompatibilityMode(fragment->document()->compatibilityMode());
-}
-
-Document* HTMLTreeBuilder::FragmentParsingContext::document() const
-{
-    ASSERT(m_fragment);
-    return m_dummyDocumentForFragmentParsing.get();
+    ASSERT(!fragment->hasChildNodes());
 }
 
 void HTMLTreeBuilder::FragmentParsingContext::finished()
 {
-    // Populate the DocumentFragment with the parsed content now that we're done.
-    ContainerNode* root = m_dummyDocumentForFragmentParsing.get();
-    if (m_contextElement)
-        root = m_dummyDocumentForFragmentParsing->documentElement();
-    m_fragment->takeAllChildrenFrom(root);
+    if (!m_contextElement)
+        return;
+    
+    // The HTML5 spec says to return the children of the fragment's document
+    // element when there is a context element (10.4.7).
+    RefPtr<ContainerNode> documentElement = m_fragment->firstElementChild();
+    m_fragment->removeChildren();
+    ASSERT(documentElement);
+    m_fragment->takeAllChildrenFrom(documentElement.get());
 }
 
 HTMLTreeBuilder::FragmentParsingContext::~FragmentParsingContext()
index 2afac92..0cec667 100644 (file)
@@ -211,7 +211,6 @@ private:
         FragmentParsingContext(DocumentFragment*, Element* contextElement, FragmentScriptingPermission);
         ~FragmentParsingContext();
 
-        Document* document() const;
         DocumentFragment* fragment() const { return m_fragment; }
         Element* contextElement() const { ASSERT(m_fragment); return m_contextElement; }
         FragmentScriptingPermission scriptingPermission() const { ASSERT(m_fragment); return m_scriptingPermission; }
@@ -219,7 +218,6 @@ private:
         void finished();
 
     private:
-        RefPtr<Document> m_dummyDocumentForFragmentParsing;
         DocumentFragment* m_fragment;
         Element* m_contextElement;