There are too many poorly named functions to create a fragment from markup
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 24 May 2012 21:15:17 +0000 (21:15 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 24 May 2012 21:15:17 +0000 (21:15 +0000)
https://bugs.webkit.org/show_bug.cgi?id=87339

Reviewed by Eric Seidel.

Source/WebCore:

Moved all functions that create a fragment from markup to markup.h/cpp.
There should be no behavioral change.

* dom/Range.cpp:
(WebCore::Range::createContextualFragment):
* dom/Range.h: Removed createDocumentFragmentForElement.
* dom/ShadowRoot.cpp:
(WebCore::ShadowRoot::setInnerHTML):
* editing/markup.cpp:
(WebCore::createFragmentFromMarkup):
(WebCore::createFragmentForInnerOuterHTML): Renamed from createFragmentFromSource.
(WebCore::createFragmentForTransformToFragment): Moved from XSLTProcessor.
(WebCore::removeElementPreservingChildren): Moved from Range.
(WebCore::createContextualFragment): Ditto.
* editing/markup.h:
* html/HTMLElement.cpp:
(WebCore::HTMLElement::setInnerHTML):
(WebCore::HTMLElement::setOuterHTML):
(WebCore::HTMLElement::insertAdjacentHTML):
* inspector/DOMPatchSupport.cpp:
(WebCore::DOMPatchSupport::patchNode): Added a FIXME since this code should be using
one of the functions listed in markup.h
* xml/XSLTProcessor.cpp:
(WebCore::XSLTProcessor::transformToFragment):

Source/WebKit/qt:

Replace calls to Range::createDocumentFragmentForElement by calls to
createContextualDocumentFragment.

* Api/qwebelement.cpp:
(QWebElement::appendInside):
(QWebElement::prependInside):
(QWebElement::prependOutside):
(QWebElement::appendOutside):
(QWebElement::encloseContentsWith):
(QWebElement::encloseWith):

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

Source/WebCore/ChangeLog
Source/WebCore/dom/Range.cpp
Source/WebCore/dom/Range.h
Source/WebCore/dom/ShadowRoot.cpp
Source/WebCore/editing/markup.cpp
Source/WebCore/editing/markup.h
Source/WebCore/html/HTMLElement.cpp
Source/WebCore/inspector/DOMPatchSupport.cpp
Source/WebCore/xml/XSLTProcessor.cpp
Source/WebKit/qt/Api/qwebelement.cpp
Source/WebKit/qt/ChangeLog

index 884787a..9702de3 100644 (file)
@@ -1,3 +1,35 @@
+2012-05-24  Ryosuke Niwa  <rniwa@webkit.org>
+
+        There are too many poorly named functions to create a fragment from markup
+        https://bugs.webkit.org/show_bug.cgi?id=87339
+
+        Reviewed by Eric Seidel.
+
+        Moved all functions that create a fragment from markup to markup.h/cpp.
+        There should be no behavioral change.
+
+        * dom/Range.cpp:
+        (WebCore::Range::createContextualFragment):
+        * dom/Range.h: Removed createDocumentFragmentForElement.
+        * dom/ShadowRoot.cpp:
+        (WebCore::ShadowRoot::setInnerHTML):
+        * editing/markup.cpp:
+        (WebCore::createFragmentFromMarkup):
+        (WebCore::createFragmentForInnerOuterHTML): Renamed from createFragmentFromSource.
+        (WebCore::createFragmentForTransformToFragment): Moved from XSLTProcessor.
+        (WebCore::removeElementPreservingChildren): Moved from Range.
+        (WebCore::createContextualFragment): Ditto.
+        * editing/markup.h:
+        * html/HTMLElement.cpp:
+        (WebCore::HTMLElement::setInnerHTML):
+        (WebCore::HTMLElement::setOuterHTML):
+        (WebCore::HTMLElement::insertAdjacentHTML):
+        * inspector/DOMPatchSupport.cpp:
+        (WebCore::DOMPatchSupport::patchNode): Added a FIXME since this code should be using
+        one of the functions listed in markup.h
+        * xml/XSLTProcessor.cpp:
+        (WebCore::XSLTProcessor::transformToFragment):
+
 2012-05-24  Jer Noble  <jer.noble@apple.com>
 
         MediaControlTimelineElement is adjusting time 3 times per click
index b466dc8..2d86fcf 100644 (file)
@@ -1111,57 +1111,6 @@ String Range::text() const
     return plainText(this);
 }
 
-static inline void removeElementPreservingChildren(PassRefPtr<DocumentFragment> fragment, HTMLElement* element)
-{
-    ExceptionCode ignoredExceptionCode;
-
-    RefPtr<Node> nextChild;
-    for (RefPtr<Node> child = element->firstChild(); child; child = nextChild) {
-        nextChild = child->nextSibling();
-        element->removeChild(child.get(), ignoredExceptionCode);
-        ASSERT(!ignoredExceptionCode);
-        fragment->insertBefore(child, element, ignoredExceptionCode);
-        ASSERT(!ignoredExceptionCode);
-    }
-    fragment->removeChild(element, ignoredExceptionCode);
-    ASSERT(!ignoredExceptionCode);
-}
-
-PassRefPtr<DocumentFragment> Range::createDocumentFragmentForElement(const String& markup, Element* element,  FragmentScriptingPermission scriptingPermission)
-{
-    ASSERT(element);
-    HTMLElement* htmlElement = toHTMLElement(element);
-    if (htmlElement->ieForbidsInsertHTML())
-        return 0;
-
-    if (htmlElement->hasLocalName(colTag) || htmlElement->hasLocalName(colgroupTag) || htmlElement->hasLocalName(framesetTag)
-        || htmlElement->hasLocalName(headTag) || htmlElement->hasLocalName(styleTag) || htmlElement->hasLocalName(titleTag))
-        return 0;
-
-    RefPtr<DocumentFragment> fragment = element->document()->createDocumentFragment();
-
-    if (element->document()->isHTMLDocument())
-        fragment->parseHTML(markup, element, scriptingPermission);
-    else if (!fragment->parseXML(markup, element, scriptingPermission))
-        return 0; // FIXME: We should propagate a syntax error exception out here.
-
-    // We need to pop <html> and <body> elements and remove <head> to
-    // accommodate folks passing complete HTML documents to make the
-    // child of an element.
-
-    RefPtr<Node> nextNode;
-    for (RefPtr<Node> node = fragment->firstChild(); node; node = nextNode) {
-        nextNode = node->nextSibling();
-        if (node->hasTagName(htmlTag) || node->hasTagName(headTag) || node->hasTagName(bodyTag)) {
-            HTMLElement* element = toHTMLElement(node.get());
-            if (Node* firstChild = element->firstChild())
-                nextNode = firstChild;
-            removeElementPreservingChildren(fragment, element);
-        }
-    }
-    return fragment.release();
-}
-
 PassRefPtr<DocumentFragment> Range::createContextualFragment(const String& markup, ExceptionCode& ec)
 {
     if (!m_start.container()) {
@@ -1175,7 +1124,7 @@ PassRefPtr<DocumentFragment> Range::createContextualFragment(const String& marku
         return 0;
     }
 
-    RefPtr<DocumentFragment> fragment = createDocumentFragmentForElement(markup, toElement(element), AllowScriptingContentAndDoNotMarkAlreadyStarted);
+    RefPtr<DocumentFragment> fragment = WebCore::createContextualFragment(markup, toElement(element), AllowScriptingContentAndDoNotMarkAlreadyStarted);
 
     if (!fragment) {
         ec = NOT_SUPPORTED_ERR;
index 276c72e..8f0e98b 100644 (file)
@@ -90,7 +90,6 @@ public:
     String text() const;
 
     PassRefPtr<DocumentFragment> createContextualFragment(const String& html, ExceptionCode&);
-    static PassRefPtr<DocumentFragment> createDocumentFragmentForElement(const String& markup, Element*,  FragmentScriptingPermission = AllowScriptingContent);
 
     void detach(ExceptionCode&);
     PassRefPtr<Range> cloneRange(ExceptionCode&) const;
index ffc5952..86a10cd 100644 (file)
@@ -144,8 +144,7 @@ String ShadowRoot::innerHTML() const
 
 void ShadowRoot::setInnerHTML(const String& markup, ExceptionCode& ec)
 {
-    RefPtr<DocumentFragment> fragment = createFragmentFromSource(markup, host(), ec);
-    if (fragment)
+    if (RefPtr<DocumentFragment> fragment = createFragmentForInnerOuterHTML(markup, host(), ec))
         replaceChildrenWithFragment(this, fragment.release(), ec);
 }
 
index a1c5a33..4983dcd 100644 (file)
@@ -665,7 +665,7 @@ PassRefPtr<DocumentFragment> createFragmentFromMarkup(Document* document, const
 {
     // We use a fake body element here to trick the HTML parser to using the InBody insertion mode.
     RefPtr<HTMLBodyElement> fakeBody = HTMLBodyElement::create(document);
-    RefPtr<DocumentFragment> fragment =  Range::createDocumentFragmentForElement(markup, fakeBody.get(), scriptingPermission);
+    RefPtr<DocumentFragment> fragment = createContextualFragment(markup, fakeBody.get(), scriptingPermission);
 
     if (fragment && !baseURL.isEmpty() && baseURL != blankURL() && baseURL != document->baseURL())
         completeURLs(fragment.get(), baseURL);
@@ -992,7 +992,7 @@ String urlToMarkup(const KURL& url, const String& title)
     return markup.toString();
 }
 
-PassRefPtr<DocumentFragment> createFragmentFromSource(const String& markup, Element* contextElement, ExceptionCode& ec)
+PassRefPtr<DocumentFragment> createFragmentForInnerOuterHTML(const String& markup, Element* contextElement, ExceptionCode& ec)
 {
     Document* document = contextElement->document();
     RefPtr<DocumentFragment> fragment = DocumentFragment::create(document);
@@ -1010,6 +1010,82 @@ PassRefPtr<DocumentFragment> createFragmentFromSource(const String& markup, Elem
     return fragment.release();
 }
 
+PassRefPtr<DocumentFragment> createFragmentForTransformToFragment(const String& sourceString, const String& sourceMIMEType, Document* outputDoc)
+{
+    RefPtr<DocumentFragment> fragment = outputDoc->createDocumentFragment();
+    
+    if (sourceMIMEType == "text/html") {
+        // As far as I can tell, there isn't a spec for how transformToFragment is supposed to work.
+        // Based on the documentation I can find, it looks like we want to start parsing the fragment in the InBody insertion mode.
+        // Unfortunately, that's an implementation detail of the parser.
+        // We achieve that effect here by passing in a fake body element as context for the fragment.
+        RefPtr<HTMLBodyElement> fakeBody = HTMLBodyElement::create(outputDoc);
+        fragment->parseHTML(sourceString, fakeBody.get());
+    } else if (sourceMIMEType == "text/plain")
+        fragment->parserAddChild(Text::create(outputDoc, sourceString));
+    else {
+        bool successfulParse = fragment->parseXML(sourceString, 0);
+        if (!successfulParse)
+            return 0;
+    }
+    
+    // FIXME: Do we need to mess with URLs here?
+    
+    return fragment.release();
+}
+
+static inline void removeElementPreservingChildren(PassRefPtr<DocumentFragment> fragment, HTMLElement* element)
+{
+    ExceptionCode ignoredExceptionCode;
+
+    RefPtr<Node> nextChild;
+    for (RefPtr<Node> child = element->firstChild(); child; child = nextChild) {
+        nextChild = child->nextSibling();
+        element->removeChild(child.get(), ignoredExceptionCode);
+        ASSERT(!ignoredExceptionCode);
+        fragment->insertBefore(child, element, ignoredExceptionCode);
+        ASSERT(!ignoredExceptionCode);
+    }
+    fragment->removeChild(element, ignoredExceptionCode);
+    ASSERT(!ignoredExceptionCode);
+}
+
+PassRefPtr<DocumentFragment> createContextualFragment(const String& markup, Element* element,  FragmentScriptingPermission scriptingPermission)
+{
+    ASSERT(element);
+    HTMLElement* htmlElement = toHTMLElement(element);
+    if (htmlElement->ieForbidsInsertHTML())
+        return 0;
+
+    if (htmlElement->hasLocalName(colTag) || htmlElement->hasLocalName(colgroupTag) || htmlElement->hasLocalName(framesetTag)
+        || htmlElement->hasLocalName(headTag) || htmlElement->hasLocalName(styleTag) || htmlElement->hasLocalName(titleTag))
+        return 0;
+
+    // FIXME: This code is almost identical to createFragmentForInnerOuterHTML except this code doesn't handle exceptions.
+    RefPtr<DocumentFragment> fragment = element->document()->createDocumentFragment();
+
+    if (element->document()->isHTMLDocument())
+        fragment->parseHTML(markup, element, scriptingPermission);
+    else if (!fragment->parseXML(markup, element, scriptingPermission))
+        return 0; // FIXME: We should propagate a syntax error exception out here.
+
+    // We need to pop <html> and <body> elements and remove <head> to
+    // accommodate folks passing complete HTML documents to make the
+    // child of an element.
+
+    RefPtr<Node> nextNode;
+    for (RefPtr<Node> node = fragment->firstChild(); node; node = nextNode) {
+        nextNode = node->nextSibling();
+        if (node->hasTagName(htmlTag) || node->hasTagName(headTag) || node->hasTagName(bodyTag)) {
+            HTMLElement* element = toHTMLElement(node.get());
+            if (Node* firstChild = element->firstChild())
+                nextNode = firstChild;
+            removeElementPreservingChildren(fragment, element);
+        }
+    }
+    return fragment.release();
+}
+
 static inline bool hasOneChild(ContainerNode* node)
 {
     Node* firstChild = node->firstChild();
index f12f562..a36cb35 100644 (file)
@@ -51,7 +51,9 @@ namespace WebCore {
     PassRefPtr<DocumentFragment> createFragmentFromMarkup(Document*, const String& markup, const String& baseURL, FragmentScriptingPermission = AllowScriptingContent);
     PassRefPtr<DocumentFragment> createFragmentFromMarkupWithContext(Document*, const String& markup, unsigned fragmentStart, unsigned fragmentEnd, const String& baseURL, FragmentScriptingPermission);
     PassRefPtr<DocumentFragment> createFragmentFromNodes(Document*, const Vector<Node*>&);
-    PassRefPtr<DocumentFragment> createFragmentFromSource(const String&, Element*, ExceptionCode&);
+    PassRefPtr<DocumentFragment> createFragmentForInnerOuterHTML(const String&, Element*, ExceptionCode&);
+    PassRefPtr<DocumentFragment> createFragmentForTransformToFragment(const String&, const String& sourceMIMEType, Document* outputDoc);
+    PassRefPtr<DocumentFragment> createContextualFragment(const String&, Element*,  FragmentScriptingPermission);
 
     bool isPlainTextMarkup(Node *node);
 
index 1fa2496..b420c34 100644 (file)
@@ -342,8 +342,7 @@ String HTMLElement::outerHTML() const
 
 void HTMLElement::setInnerHTML(const String& html, ExceptionCode& ec)
 {
-    RefPtr<DocumentFragment> fragment = createFragmentFromSource(html, this, ec);
-    if (fragment)
+    if (RefPtr<DocumentFragment> fragment = createFragmentForInnerOuterHTML(html, this, ec))
         replaceChildrenWithFragment(this, fragment.release(), ec);
 }
 
@@ -374,7 +373,7 @@ void HTMLElement::setOuterHTML(const String& html, ExceptionCode& ec)
     RefPtr<Node> prev = previousSibling();
     RefPtr<Node> next = nextSibling();
 
-    RefPtr<DocumentFragment> fragment = createFragmentFromSource(html, parent.get(), ec);
+    RefPtr<DocumentFragment> fragment = createFragmentForInnerOuterHTML(html, parent.get(), ec);
     if (ec)
         return;
       
@@ -576,19 +575,13 @@ static Element* contextElementForInsertion(const String& where, Element* element
 
 void HTMLElement::insertAdjacentHTML(const String& where, const String& markup, ExceptionCode& ec)
 {
-    RefPtr<DocumentFragment> fragment = document()->createDocumentFragment();
     Element* contextElement = contextElementForInsertion(where, this, ec);
     if (!contextElement)
         return;
-
-    if (document()->isHTMLDocument())
-         fragment->parseHTML(markup, contextElement);
-    else {
-        if (!fragment->parseXML(markup, contextElement))
-            // FIXME: We should propagate a syntax error exception out here.
-            return;
-    }
-
+    ExceptionCode ignoredEc = 0; // FIXME: We should propagate a syntax error exception out here.
+    RefPtr<DocumentFragment> fragment = createFragmentForInnerOuterHTML(markup, this, ignoredEc);
+    if (ignoredEc)
+        return;
     insertAdjacent(where, fragment.get(), ec);
 }
 
index b7fdbb0..ba0d8c8 100644 (file)
@@ -114,6 +114,7 @@ Node* DOMPatchSupport::patchNode(Node* node, const String& markup, ExceptionCode
     }
 
     Node* previousSibling = node->previousSibling();
+    // FIXME: This code should use one of createFragment* in markup.h
     RefPtr<DocumentFragment> fragment = DocumentFragment::create(m_document);
     fragment->parseHTML(markup, node->parentElement() ? node->parentElement() : m_document->documentElement());
 
index 13733dc..c5c84e7 100644 (file)
@@ -107,32 +107,6 @@ PassRefPtr<Document> XSLTProcessor::createDocumentFromSource(const String& sourc
     return result.release();
 }
 
-static inline RefPtr<DocumentFragment> createFragmentFromSource(const String& sourceString, const String& sourceMIMEType, Document* outputDoc)
-{
-    RefPtr<DocumentFragment> fragment = outputDoc->createDocumentFragment();
-
-    if (sourceMIMEType == "text/html") {
-        // As far as I can tell, there isn't a spec for how transformToFragment
-        // is supposed to work.  Based on the documentation I can find, it looks
-        // like we want to start parsing the fragment in the InBody insertion
-        // mode.  Unfortunately, that's an implementation detail of the parser.
-        // We achieve that effect here by passing in a fake body element as
-        // context for the fragment.
-        RefPtr<HTMLBodyElement> fakeBody = HTMLBodyElement::create(outputDoc);
-        fragment->parseHTML(sourceString, fakeBody.get());
-    } else if (sourceMIMEType == "text/plain")
-        fragment->parserAddChild(Text::create(outputDoc, sourceString));
-    else {
-        bool successfulParse = fragment->parseXML(sourceString, 0);
-        if (!successfulParse)
-            return 0;
-    }
-
-    // FIXME: Do we need to mess with URLs here?
-
-    return fragment;
-}
-
 PassRefPtr<Document> XSLTProcessor::transformToDocument(Node* sourceNode)
 {
     String resultMIMEType;
@@ -155,7 +129,7 @@ PassRefPtr<DocumentFragment> XSLTProcessor::transformToFragment(Node* sourceNode
 
     if (!transformToString(sourceNode, resultMIMEType, resultString, resultEncoding))
         return 0;
-    return createFragmentFromSource(resultString, resultMIMEType, outputDoc);
+    return createFragmentForTransformToFragment(resultString, resultMIMEType, outputDoc);
 }
 
 void XSLTProcessor::setParameter(const String& /*namespaceURI*/, const String& localName, const String& value)
index fb9be78..1d020e1 100644 (file)
@@ -51,6 +51,7 @@
 #include "ScriptState.h"
 #include "StaticNodeList.h"
 #include "StyleResolver.h"
+#include "markup.h"
 #include "qwebframe.h"
 #include "qwebframe_p.h"
 #if USE(JSC)
@@ -1010,7 +1011,7 @@ void QWebElement::appendInside(const QString &markup)
     if (!m_element->isHTMLElement())
         return;
 
-    RefPtr<DocumentFragment> fragment =  Range::createDocumentFragmentForElement(markup, toHTMLElement(m_element));
+    RefPtr<DocumentFragment> fragment =  createContextualFragment(markup, toHTMLElement(m_element), AllowScriptingContent);
 
     ExceptionCode exception = 0;
     m_element->appendChild(fragment, exception);
@@ -1055,7 +1056,7 @@ void QWebElement::prependInside(const QString &markup)
     if (!m_element->isHTMLElement())
         return;
 
-    RefPtr<DocumentFragment> fragment =  Range::createDocumentFragmentForElement(markup, toHTMLElement(m_element));
+    RefPtr<DocumentFragment> fragment =  createContextualFragment(markup, toHTMLElement(m_element), AllowScriptingContent);
 
     ExceptionCode exception = 0;
 
@@ -1107,7 +1108,7 @@ void QWebElement::prependOutside(const QString &markup)
     if (!parent->isHTMLElement())
         return;
 
-    RefPtr<DocumentFragment> fragment = Range::createDocumentFragmentForElement(markup, toHTMLElement(parent));
+    RefPtr<DocumentFragment> fragment = createContextualFragment(markup, toHTMLElement(parent), AllowScriptingContent);
 
     ExceptionCode exception = 0;
     parent->insertBefore(fragment, m_element, exception);
@@ -1157,7 +1158,7 @@ void QWebElement::appendOutside(const QString &markup)
     if (!parent->isHTMLElement())
         return;
 
-    RefPtr<DocumentFragment> fragment = Range::createDocumentFragmentForElement(markup, toHTMLElement(parent));
+    RefPtr<DocumentFragment> fragment = createContextualFragment(markup, toHTMLElement(parent), AllowScriptingContent);
 
     ExceptionCode exception = 0;
     if (!m_element->nextSibling())
@@ -1303,7 +1304,7 @@ void QWebElement::encloseContentsWith(const QString &markup)
     if (!m_element->isHTMLElement())
         return;
 
-    RefPtr<DocumentFragment> fragment =  Range::createDocumentFragmentForElement(markup, toHTMLElement(m_element));
+    RefPtr<DocumentFragment> fragment =  createContextualFragment(markup, toHTMLElement(m_element), AllowScriptingContent);
 
     if (!fragment || !fragment->firstChild())
         return;
@@ -1378,7 +1379,7 @@ void QWebElement::encloseWith(const QString &markup)
     if (!parent->isHTMLElement())
         return;
 
-    RefPtr<DocumentFragment> fragment = Range::createDocumentFragmentForElement(markup, toHTMLElement(parent));
+    RefPtr<DocumentFragment> fragment = createContextualFragment(markup, toHTMLElement(parent), AllowScriptingContent);
 
     if (!fragment || !fragment->firstChild())
         return;
index 6d2680a..e1e5049 100644 (file)
@@ -1,3 +1,21 @@
+2012-05-24  Ryosuke Niwa  <rniwa@webkit.org>
+
+        There are too many poorly named functions to create a fragment from markup
+        https://bugs.webkit.org/show_bug.cgi?id=87339
+
+        Reviewed by Eric Seidel.
+
+        Replace calls to Range::createDocumentFragmentForElement by calls to
+        createContextualDocumentFragment.
+
+        * Api/qwebelement.cpp:
+        (QWebElement::appendInside):
+        (QWebElement::prependInside):
+        (QWebElement::prependOutside):
+        (QWebElement::appendOutside):
+        (QWebElement::encloseContentsWith):
+        (QWebElement::encloseWith):
+
 2012-05-24  Gabor Ballabas  <gaborb@inf.u-szeged.hu>
 
         [Qt]  Fix Webkit1 + V8 build.