Refactor post-attach and HTMLObjectElement-related code

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

Reviewed by Antti Koivisto.

Source/WebCore:

* dom/ContainerNode.cpp: Moved the post-attach callback code from here to
StyleResolveTree.h/cpp.
* dom/ContainerNode.h: Ditto.

* dom/Document.cpp:
(WebCore::Document::recalcStyle): Use Style::PostResolutionCallbackDisabler instead of
PostAttachCallbackDisabler.

* dom/Element.h: Moved the post-attach callback code from here to StyleResolveTree.h/cpp.

* html/HTMLEmbedElement.cpp:
(WebCore::HTMLEmbedElement::parseAttribute): Simplified the code for typeAttr, turning
it into a 1-liner. Added a FIXME in codeAttr about the fact that it does not have the
code to trigger image loads.

* html/HTMLFormControlElement.cpp:
(WebCore::HTMLFormControlElement::didAttachRenderers): Updated to use
Style::queuePostResolutionCallback and use a lambda instead of a function.
(WebCore::HTMLFormControlElement::didRecalcStyle): Ditto. Also added RefPtr instead
of just using wishful thinking to keep the object alive.
* html/HTMLFrameOwnerElement.cpp:
(WebCore::HTMLFrameOwnerElement::scheduleSetNeedsStyleRecalc): Ditto.

* html/HTMLObjectElement.cpp:
(WebCore::HTMLObjectElement::parseAttribute):: Simplified the code for typeAttr, turning
it into a 1-liner. Made dataAttr call setNeedsWidgetUpdate(true) unconditionally after
checking carefully to see that's harmless if there is no renderer. Changed classidAttr
to call setNeedsWidgetUpdate(true) unconditionally and not set m_classId.
(WebCore::HTMLObjectElement::shouldAllowQuickTimeClassIdQuirk): Use fastGetAttribute
instead of classId and descendantsOfType instead of getElementsByTagName.
(WebCore::HTMLObjectElement::hasValidClassId): Use fastGetAttribute instead of classId.
(WebCore::HTMLObjectElement::renderFallbackContent): Use imageLoader instead of m_imageLoader.

* html/HTMLObjectElement.h: Removed classId, since there is no reason to cache that
attribute in a data member. Rearranged header, making more private, and fixing some typos,
and doing a "using" instead of a function to disambiguate the inherited form functions.

* html/HTMLPlugInImageElement.cpp:
(WebCore::HTMLPlugInImageElement::createElementRenderer): Fixed some code that assumed the
first child of the shadow root is guaranteed to be an element.
(WebCore::HTMLPlugInImageElement::didMoveToNewDocument): Removed null check on oldDocument,
since m_needsDocumentActivationCallbacks can't be true if the old document was null.
(WebCore::is100Percent): Added helper to make function below more readable.
(WebCore::HTMLPlugInImageElement::subframeLoaderWillCreatePlugIn): Restructured the code a
bit. The part that attracted my attention was the local variable of type RenderBox, which
was named renderEmbeddedObject. Turns out the caller guarantees to only call this if there
is a renderer of type RenderEmbeddedObject, so depend on that.

* html/HTMLPlugInImageElement.h: Trimmed includes a bit. Made more members private.
Marked more function members final. Made a protected imageLoader function so that
m_imageLoader can be private eventually. Made m_imageLoader be std::unique_ptr.

* style/StyleResolveTree.cpp:
(WebCore::Style::needsPseudoElement): Fixed spelling error in the name of this function.
(WebCore::Style::attachBeforeOrAfterPseudoElementIfNeeded): Updated for name change.
(WebCore::Style::attachRenderTree): Update for new name of PostResolutionCallbackDisabler.
(WebCore::Style::updateBeforeOrAfterPseudoElement): Updated for name change.
(WebCore::Style::postResolutionCallbackQueue): Added.
(WebCore::Style::queuePostResolutionCallback): Added.
(WebCore::Style::suspendMemoryCacheClientCalls): Added. This is a side effect of the original
PostAttachCallbackDisabler that is now done in a cleaner way, using the callback queue, instead
of as a special case. It should not work for multiple documents across multiple pages instead of
only the outermost one.
(WebCore::Style::PostResolutionCallbackDisabler::PostResolutionCallbackDisabler): Added.
Calls suspendMemoryCacheClientCalls, but a FIXME tries to point out why that isn't so great.
(WebCore::Style::PostResolutionCallbackDisabler::~PostResolutionCallbackDisabler): Added.
(WebCore::Style::postResolutionCallbacksAreSuspended): Added.

* style/StyleResolveTree.h: Added queuePostResolutionCallback and
postResolutionCallbacksAreSuspended. Also added PostResolutionCallbackDisabler, which should
eventually become a private implementation detail.

Source/WebKit/mac:

* WebCoreSupport/WebFrameLoaderClient.mm: Call toHTMLPlugInImageElement instead of
doing a static_cast.

LayoutTests:

* svg/custom/object-no-size-attributes-expected.txt: Removed expectation of an empty text
renderer from the render tree.
* svg/custom/object-no-size-attributes.xhtml: Restructured the source so there is no text
to render. Without this, we were seeing two text renderers due to the loading timing change.

Canonical link: https://commits.webkit.org/149338@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@166853 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog

@@ -1,3 +1,15 @@
+2014-03-23  Darin Adler  <darin@apple.com>
+
+        Refactor post-attach and HTMLObjectElement-related code
+        https://bugs.webkit.org/show_bug.cgi?id=131282
+
+        Reviewed by Antti Koivisto.
+
+        * svg/custom/object-no-size-attributes-expected.txt: Removed expectation of an empty text
+        renderer from the render tree.
+        * svg/custom/object-no-size-attributes.xhtml: Restructured the source so there is no text
+        to render. Without this, we were seeing two text renderers due to the loading timing change.
+
 2014-04-05  Dirk Schulze  <krit@webkit.org>
 
         Canvas strokeText and fillText with SourceIn, DestinationIn, SourceOut, DestinationAtop and Copy have errors

LayoutTests/svg/custom/object-no-size-attributes-expected.txt

@@ -9,4 +9,3 @@ layer at (0,0) size 800x120
         layer at (0,0) size 100x100
           RenderSVGRoot {svg} at (0,0) size 100x100
             RenderSVGRect {rect} at (0,0) size 100x100 [fill={[type=SOLID] [color=#008000]}] [x=0.00] [y=0.00] [width=100.00] [height=100.00]
-      RenderText {#text} at (0,0) size 0x0

LayoutTests/svg/custom/object-no-size-attributes.xhtml

@@ -1,9 +1,7 @@
 <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
 <html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en">
 <body>
-<object type="image/svg+xml" data="data:image/svg+xml;charset=utf-8,%3Csvg%20width%3D%22100px%22%20height%3D%22100px%22%20xmlns%3D%22http%3A%2F%2Fwww.w3.org%2F2000%2Fsvg%22%3E%3Crect%20fill%3D%22green%22%20width%3D%22100px%22%20height%3D%22100px%22%20%2F%3E%3C%2Fsvg%3E" style='background: red'></object>
-
-<!--
+<object type="image/svg+xml" data="data:image/svg+xml;charset=utf-8,%3Csvg%20width%3D%22100px%22%20height%3D%22100px%22%20xmlns%3D%22http%3A%2F%2Fwww.w3.org%2F2000%2Fsvg%22%3E%3Crect%20fill%3D%22green%22%20width%3D%22100px%22%20height%3D%22100px%22%20%2F%3E%3C%2Fsvg%3E" style='background: red'></object><!--
 100px x 100px SVG
 
 Code:
@@ -13,7 +11,5 @@ Code:
 </svg>
 
 You should see a 100 x 100 green square above.  No red should be visible.  &lt;object&gt; tags should treat the width/height attributes on an embeded SVG as the intrinsic size and not default to the CSS default of 300 x 150.
--->
-
-</body>
+--></body>
 </html>

Source/WebCore/ChangeLog

@@ -1,3 +1,82 @@
+2014-03-23  Darin Adler  <darin@apple.com>
+
+        Refactor post-attach and HTMLObjectElement-related code
+        https://bugs.webkit.org/show_bug.cgi?id=131282
+
+        Reviewed by Antti Koivisto.
+
+        * dom/ContainerNode.cpp: Moved the post-attach callback code from here to
+        StyleResolveTree.h/cpp.
+        * dom/ContainerNode.h: Ditto.
+
+        * dom/Document.cpp:
+        (WebCore::Document::recalcStyle): Use Style::PostResolutionCallbackDisabler instead of
+        PostAttachCallbackDisabler.
+
+        * dom/Element.h: Moved the post-attach callback code from here to StyleResolveTree.h/cpp.
+
+        * html/HTMLEmbedElement.cpp:
+        (WebCore::HTMLEmbedElement::parseAttribute): Simplified the code for typeAttr, turning
+        it into a 1-liner. Added a FIXME in codeAttr about the fact that it does not have the
+        code to trigger image loads.
+
+        * html/HTMLFormControlElement.cpp:
+        (WebCore::HTMLFormControlElement::didAttachRenderers): Updated to use
+        Style::queuePostResolutionCallback and use a lambda instead of a function.
+        (WebCore::HTMLFormControlElement::didRecalcStyle): Ditto. Also added RefPtr instead
+        of just using wishful thinking to keep the object alive.
+        * html/HTMLFrameOwnerElement.cpp:
+        (WebCore::HTMLFrameOwnerElement::scheduleSetNeedsStyleRecalc): Ditto.
+
+        * html/HTMLObjectElement.cpp:
+        (WebCore::HTMLObjectElement::parseAttribute):: Simplified the code for typeAttr, turning
+        it into a 1-liner. Made dataAttr call setNeedsWidgetUpdate(true) unconditionally after
+        checking carefully to see that's harmless if there is no renderer. Changed classidAttr
+        to call setNeedsWidgetUpdate(true) unconditionally and not set m_classId.
+        (WebCore::HTMLObjectElement::shouldAllowQuickTimeClassIdQuirk): Use fastGetAttribute
+        instead of classId and descendantsOfType instead of getElementsByTagName.
+        (WebCore::HTMLObjectElement::hasValidClassId): Use fastGetAttribute instead of classId.
+        (WebCore::HTMLObjectElement::renderFallbackContent): Use imageLoader instead of m_imageLoader.
+
+        * html/HTMLObjectElement.h: Removed classId, since there is no reason to cache that
+        attribute in a data member. Rearranged header, making more private, and fixing some typos,
+        and doing a "using" instead of a function to disambiguate the inherited form functions.
+
+        * html/HTMLPlugInImageElement.cpp:
+        (WebCore::HTMLPlugInImageElement::createElementRenderer): Fixed some code that assumed the
+        first child of the shadow root is guaranteed to be an element.
+        (WebCore::HTMLPlugInImageElement::didMoveToNewDocument): Removed null check on oldDocument,
+        since m_needsDocumentActivationCallbacks can't be true if the old document was null.
+        (WebCore::is100Percent): Added helper to make function below more readable.
+        (WebCore::HTMLPlugInImageElement::subframeLoaderWillCreatePlugIn): Restructured the code a
+        bit. The part that attracted my attention was the local variable of type RenderBox, which
+        was named renderEmbeddedObject. Turns out the caller guarantees to only call this if there
+        is a renderer of type RenderEmbeddedObject, so depend on that.
+
+        * html/HTMLPlugInImageElement.h: Trimmed includes a bit. Made more members private.
+        Marked more function members final. Made a protected imageLoader function so that
+        m_imageLoader can be private eventually. Made m_imageLoader be std::unique_ptr.
+
+        * style/StyleResolveTree.cpp:
+        (WebCore::Style::needsPseudoElement): Fixed spelling error in the name of this function.
+        (WebCore::Style::attachBeforeOrAfterPseudoElementIfNeeded): Updated for name change.
+        (WebCore::Style::attachRenderTree): Update for new name of PostResolutionCallbackDisabler.
+        (WebCore::Style::updateBeforeOrAfterPseudoElement): Updated for name change.
+        (WebCore::Style::postResolutionCallbackQueue): Added.
+        (WebCore::Style::queuePostResolutionCallback): Added.
+        (WebCore::Style::suspendMemoryCacheClientCalls): Added. This is a side effect of the original
+        PostAttachCallbackDisabler that is now done in a cleaner way, using the callback queue, instead
+        of as a special case. It should not work for multiple documents across multiple pages instead of
+        only the outermost one.
+        (WebCore::Style::PostResolutionCallbackDisabler::PostResolutionCallbackDisabler): Added.
+        Calls suspendMemoryCacheClientCalls, but a FIXME tries to point out why that isn't so great.
+        (WebCore::Style::PostResolutionCallbackDisabler::~PostResolutionCallbackDisabler): Added.
+        (WebCore::Style::postResolutionCallbacksAreSuspended): Added.
+
+        * style/StyleResolveTree.h: Added queuePostResolutionCallback and
+        postResolutionCallbacksAreSuspended. Also added PostResolutionCallbackDisabler, which should
+        eventually become a private implementation detail.
+
 2014-04-04  Brian J. Burg  <burg@cs.washington.edu>
 
         Enable WEB_REPLAY for PLATFORM(MAC)

Source/WebCore/dom/ContainerNode.cpp

@@ -37,13 +37,10 @@
 #include "JSLazyEventListener.h"
 #include "JSNode.h"
 #include "LabelsNodeList.h"
-#include "LoaderStrategy.h"
-#include "MemoryCache.h"
 #include "MutationEvent.h"
 #include "NameNodeList.h"
 #include "NodeRareData.h"
 #include "NodeRenderStyle.h"
-#include "PlatformStrategies.h"
 #include "RadioNodeList.h"
 #include "RenderBox.h"
 #include "RenderTheme.h"
@@ -63,15 +60,6 @@ namespace WebCore {
 static void dispatchChildInsertionEvents(Node&);
 static void dispatchChildRemovalEvents(Node&);
 
-typedef std::pair<RefPtr<Node>, unsigned> CallbackParameters;
-typedef std::pair<NodeCallback, CallbackParameters> CallbackInfo;
-typedef Vector<CallbackInfo> NodeCallbackQueue;
-
-static NodeCallbackQueue* s_postAttachCallbackQueue;
-
-static size_t s_attachDepth;
-static bool s_shouldReEnableMemoryCacheCallsAfterAttach;
-
 ChildNodesLazySnapshot* ChildNodesLazySnapshot::latestSnapshot = 0;
 
 #ifndef NDEBUG
@@ -758,67 +746,6 @@ void ContainerNode::parserAppendChild(PassRefPtr<Node> newChild)
     newChild->setNeedsStyleRecalc(ReconstructRenderTree);
 }
 
-void ContainerNode::suspendPostAttachCallbacks(Document& document)
-{
-    if (!s_attachDepth) {
-        ASSERT(!s_shouldReEnableMemoryCacheCallsAfterAttach);
-        if (Page* page = document.page()) {
-            // FIXME: How can this call be specific to one Page, while the
-            // s_attachDepth is a global? Doesn't make sense.
-            if (page->areMemoryCacheClientCallsEnabled()) {
-                page->setMemoryCacheClientCallsEnabled(false);
-                s_shouldReEnableMemoryCacheCallsAfterAttach = true;
-            }
-        }
-        platformStrategies()->loaderStrategy()->resourceLoadScheduler()->suspendPendingRequests();
-    }
-    ++s_attachDepth;
-}
-
-void ContainerNode::resumePostAttachCallbacks(Document& document)
-{
-    if (s_attachDepth == 1) {
-        Ref<Document> protect(document);
-
-        if (s_postAttachCallbackQueue)
-            dispatchPostAttachCallbacks();
-        if (s_shouldReEnableMemoryCacheCallsAfterAttach) {
-            s_shouldReEnableMemoryCacheCallsAfterAttach = false;
-            if (Page* page = document.page())
-                page->setMemoryCacheClientCallsEnabled(true);
-        }
-        platformStrategies()->loaderStrategy()->resourceLoadScheduler()->resumePendingRequests();
-    }
-    --s_attachDepth;
-}
-
-void ContainerNode::queuePostAttachCallback(NodeCallback callback, Node& node, unsigned callbackData)
-{
-    if (!s_postAttachCallbackQueue)
-        s_postAttachCallbackQueue = new NodeCallbackQueue;
-
-    s_postAttachCallbackQueue->append(CallbackInfo(callback, CallbackParameters(&node, callbackData)));
-}
-
-bool ContainerNode::postAttachCallbacksAreSuspended()
-{
-    return s_attachDepth;
-}
-
-void ContainerNode::dispatchPostAttachCallbacks()
-{
-    // We recalculate size() each time through the loop because a callback
-    // can add more callbacks to the end of the queue.
-    for (size_t i = 0; i < s_postAttachCallbackQueue->size(); ++i) {
-        const CallbackInfo& info = (*s_postAttachCallbackQueue)[i];
-        NodeCallback callback = info.first;
-        CallbackParameters params = info.second;
-
-        callback(*params.first, params.second);
-    }
-    s_postAttachCallbackQueue->clear();
-}
-
 void ContainerNode::childrenChanged(const ChildChange& change)
 {
     document().incDOMTreeVersion();

Source/WebCore/dom/ContainerNode.h

@@ -79,7 +79,6 @@ class NoEventDispatchAssertion {
 };
 
 class ContainerNode : public Node {
-    friend class PostAttachCallbackDisabler;
 public:
     virtual ~ContainerNode();
 
@@ -140,9 +139,6 @@ class ContainerNode : public Node {
 protected:
     explicit ContainerNode(Document&, ConstructionType = CreateContainer);
 
-    static void queuePostAttachCallback(NodeCallback, Node&, unsigned = 0);
-    static bool postAttachCallbacksAreSuspended();
-
     template<class GenericNode, class GenericNodeContainer>
     friend void appendChildToContainer(GenericNode* child, GenericNodeContainer&);
 
@@ -157,10 +153,6 @@ class ContainerNode : public Node {
     void removeBetween(Node* previousChild, Node* nextChild, Node& oldChild);
     void insertBeforeCommon(Node& nextChild, Node& oldChild);
 
-    static void dispatchPostAttachCallbacks();
-    static void suspendPostAttachCallbacks(Document&);
-    static void resumePostAttachCallbacks(Document&);
-
     bool getUpperLeftCorner(FloatPoint&) const;
     bool getLowerRightCorner(FloatPoint&) const;
 

Source/WebCore/dom/Document.cpp

@@ -1754,7 +1754,7 @@ void Document::recalcStyle(Style::Change change)
 
     m_inStyleRecalc = true;
     {
-        PostAttachCallbackDisabler disabler(*this);
+        Style::PostResolutionCallbackDisabler disabler(*this);
         WidgetHierarchyUpdatesSuspensionScope suspendWidgetHierarchyUpdates;
 
         if (m_pendingStyleRecalcShouldForce)

Source/WebCore/dom/Element.h

@@ -796,23 +796,6 @@ inline UniqueElementData& Element::ensureUniqueElementData()
     return static_cast<UniqueElementData&>(*m_elementData);
 }
 
-class PostAttachCallbackDisabler {
-public:
-    explicit PostAttachCallbackDisabler(Document& document)
-        : m_document(document)
-    {
-        Element::suspendPostAttachCallbacks(m_document);
-    }
-
-    ~PostAttachCallbackDisabler()
-    {
-        Element::resumePostAttachCallbacks(m_document);
-    }
-
-private:
-    Document& m_document;
-};
-
 } // namespace WebCore
 
 #endif

Source/WebCore/html/HTMLEmbedElement.cpp

@@ -97,20 +97,23 @@ void HTMLEmbedElement::collectStyleForPresentationAttribute(const QualifiedName&
 void HTMLEmbedElement::parseAttribute(const QualifiedName& name, const AtomicString& value)
 {
     if (name == typeAttr) {
-        m_serviceType = value.string().lower();
-        size_t pos = m_serviceType.find(";");
-        if (pos != notFound)
-            m_serviceType = m_serviceType.left(pos);
-    } else if (name == codeAttr)
+        m_serviceType = value.string().left(value.find(";")).lower();
+        // FIXME: The only difference between this and HTMLObjectElement's corresponding
+        // code is that HTMLObjectElement does setNeedsWidgetUpdate(true). Consider moving
+        // this up to the HTMLPlugInImageElement to be shared.
+    } else if (name == codeAttr) {
         m_url = stripLeadingAndTrailingHTMLSpaces(value);
-    else if (name == srcAttr) {
+        // FIXME: Why no call to the image loader?
+        // FIXME: If both code and src attributes are specified, last one parsed/changed wins. That can't be right!
+    } else if (name == srcAttr) {
         m_url = stripLeadingAndTrailingHTMLSpaces(value);
         document().updateStyleIfNeeded();
         if (renderer() && isImageType()) {
             if (!m_imageLoader)
-                m_imageLoader = adoptPtr(new HTMLImageLoader(*this));
+                m_imageLoader = std::make_unique<HTMLImageLoader>(*this);
             m_imageLoader->updateFromElementIgnoringPreviousError();
         }
+        // FIXME: If both code and src attributes are specified, last one parsed/changed wins. That can't be right!
     } else
         HTMLPlugInImageElement::parseAttribute(name, value);
 }

Source/WebCore/html/HTMLFormControlElement.cpp

@@ -208,12 +208,6 @@ static bool shouldAutofocus(HTMLFormControlElement* element)
     return false;
 }
 
-static void focusPostAttach(Node& element, unsigned)
-{ 
-    toElement(element).focus();
-    element.deref();
-}
-
 void HTMLFormControlElement::didAttachRenderers()
 {
     // The call to updateFromElement() needs to go after the call through
@@ -224,8 +218,11 @@ void HTMLFormControlElement::didAttachRenderers()
 
     if (shouldAutofocus(this)) {
         setAutofocused();
-        ref();
-        queuePostAttachCallback(focusPostAttach, *this);
+
+        RefPtr<HTMLFormControlElement> element = this;
+        Style::queuePostResolutionCallback([element] {
+            element->focus();
+        });
     }
 }
 
@@ -288,18 +285,17 @@ bool HTMLFormControlElement::isRequired() const
     return m_isRequired;
 }
 
-static void updateFromElementCallback(Node& node, unsigned)
-{
-    if (auto renderer = toHTMLFormControlElement(node).renderer())
-        renderer->updateFromElement();
-}
-
 void HTMLFormControlElement::didRecalcStyle(Style::Change)
 {
     // updateFromElement() can cause the selection to change, and in turn
     // trigger synchronous layout, so it must not be called during style recalc.
-    if (renderer())
-        queuePostAttachCallback(updateFromElementCallback, *this);
+    if (renderer()) {
+        RefPtr<HTMLFormControlElement> element = this;
+        Style::queuePostResolutionCallback([element]{
+            if (auto* renderer = element->renderer())
+                renderer->updateFromElement();
+        });
+    }
 }
 
 bool HTMLFormControlElement::supportsFocus() const

Source/WebCore/html/HTMLFrameOwnerElement.cpp

@@ -121,16 +121,14 @@ SVGDocument* HTMLFrameOwnerElement::getSVGDocument(ExceptionCode& ec) const
     return 0;
 }
 
-static void needsStyleRecalcCallback(Node& node, unsigned data)
-{
-    node.setNeedsStyleRecalc(static_cast<StyleChangeType>(data));
-}
-
 void HTMLFrameOwnerElement::scheduleSetNeedsStyleRecalc(StyleChangeType changeType)
 {
-    if (postAttachCallbacksAreSuspended())
-        queuePostAttachCallback(needsStyleRecalcCallback, *this, static_cast<unsigned>(changeType));
-    else
+    if (Style::postResolutionCallbacksAreSuspended()) {
+        RefPtr<HTMLFrameOwnerElement> element = this;
+        Style::queuePostResolutionCallback([element, changeType]{
+            element->setNeedsStyleRecalc(changeType);
+        });
+    } else
         setNeedsStyleRecalc(changeType);
 }