2010-09-02 Eric Seidel <eric@webkit.org>
authoreric@webkit.org <eric@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 3 Sep 2010 04:49:14 +0000 (04:49 +0000)
committereric@webkit.org <eric@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 3 Sep 2010 04:49:14 +0000 (04:49 +0000)
        Reviewed by Dimitri Glazkov.

        Move updateWidget into FrameView from RenderEmbeddedObject
        https://bugs.webkit.org/show_bug.cgi?id=45065

        I also made updateWidget() virtual on HTMLPlugInImageElement.
        I'm not yet sure that updateWidget belongs on HTMLElement since
        I'm not sure that HTMLMediaElement's use of the updateWidget
        infrastructure is correct.

        I also got rid of the strange !m_replacementText.isEmpty() checks
        by making a pluginCrashedOrWasMissing() call which seems to embody
        the idea behind that check and hides the screwy details.

        I noticed a couple methods on HTMLPlugInImageElement were public
        which did not need to be.  Fixed.

        No functional change, thus no tests.

        * html/HTMLEmbedElement.h:
        * html/HTMLObjectElement.h:
        * html/HTMLPlugInImageElement.cpp:
        (WebCore::HTMLPlugInImageElement::updateWidgetIfNecessary):
        * html/HTMLPlugInImageElement.h:
        (WebCore::HTMLPlugInImageElement::serviceType):
        (WebCore::HTMLPlugInImageElement::url):
        * page/FrameView.cpp:
        (WebCore::FrameView::updateWidget):
        (WebCore::FrameView::updateWidgets):
        * page/FrameView.h:
        * rendering/RenderEmbeddedObject.cpp:
        (WebCore::RenderEmbeddedObject::pluginCrashedOrWasMissing):
        (WebCore::RenderEmbeddedObject::paint):
        (WebCore::RenderEmbeddedObject::paintReplaced):
        * rendering/RenderEmbeddedObject.h:

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

WebCore/ChangeLog
WebCore/html/HTMLEmbedElement.cpp
WebCore/html/HTMLEmbedElement.h
WebCore/html/HTMLObjectElement.cpp
WebCore/html/HTMLObjectElement.h
WebCore/html/HTMLPlugInImageElement.cpp
WebCore/html/HTMLPlugInImageElement.h
WebCore/page/FrameView.cpp
WebCore/page/FrameView.h
WebCore/rendering/RenderEmbeddedObject.cpp
WebCore/rendering/RenderEmbeddedObject.h

index 2c3dc74..f4c33ca 100644 (file)
@@ -2,6 +2,44 @@
 
         Reviewed by Dimitri Glazkov.
 
+        Move updateWidget into FrameView from RenderEmbeddedObject
+        https://bugs.webkit.org/show_bug.cgi?id=45065
+
+        I also made updateWidget() virtual on HTMLPlugInImageElement.
+        I'm not yet sure that updateWidget belongs on HTMLElement since
+        I'm not sure that HTMLMediaElement's use of the updateWidget
+        infrastructure is correct.
+
+        I also got rid of the strange !m_replacementText.isEmpty() checks
+        by making a pluginCrashedOrWasMissing() call which seems to embody
+        the idea behind that check and hides the screwy details.
+
+        I noticed a couple methods on HTMLPlugInImageElement were public
+        which did not need to be.  Fixed.
+
+        No functional change, thus no tests.
+
+        * html/HTMLEmbedElement.h:
+        * html/HTMLObjectElement.h:
+        * html/HTMLPlugInImageElement.cpp:
+        (WebCore::HTMLPlugInImageElement::updateWidgetIfNecessary):
+        * html/HTMLPlugInImageElement.h:
+        (WebCore::HTMLPlugInImageElement::serviceType):
+        (WebCore::HTMLPlugInImageElement::url):
+        * page/FrameView.cpp:
+        (WebCore::FrameView::updateWidget):
+        (WebCore::FrameView::updateWidgets):
+        * page/FrameView.h:
+        * rendering/RenderEmbeddedObject.cpp:
+        (WebCore::RenderEmbeddedObject::pluginCrashedOrWasMissing):
+        (WebCore::RenderEmbeddedObject::paint):
+        (WebCore::RenderEmbeddedObject::paintReplaced):
+        * rendering/RenderEmbeddedObject.h:
+
+2010-09-02  Eric Seidel  <eric@webkit.org>
+
+        Reviewed by Dimitri Glazkov.
+
         Move updateWidget implementations into the DOM
         https://bugs.webkit.org/show_bug.cgi?id=45058
 
index 0ab00ec..a0ee7bd 100644 (file)
@@ -137,6 +137,7 @@ void HTMLEmbedElement::parametersForPlugin(Vector<String>& paramNames, Vector<St
 // moved down into HTMLPluginImageElement.cpp
 void HTMLEmbedElement::updateWidget(bool onlyCreateNonNetscapePlugins)
 {
+    ASSERT(!renderEmbeddedObject()->pluginCrashedOrWasMissing());
     // FIXME: We should ASSERT(needsWidgetUpdate()), but currently
     // FrameView::updateWidget() calls updateWidget(false) without checking if
     // the widget actually needs updating!
index 0fb7f50..70eb0dc 100644 (file)
@@ -31,8 +31,6 @@ class HTMLEmbedElement : public HTMLPlugInImageElement {
 public:
     static PassRefPtr<HTMLEmbedElement> create(const QualifiedName&, Document*, bool createdByParser);
 
-    void updateWidget(bool onlyCreateNonNetscapePlugins);
-
 private:
     HTMLEmbedElement(const QualifiedName&, Document*, bool createdByParser);
 
@@ -43,12 +41,14 @@ private:
     virtual void insertedIntoDocument();
     virtual void removedFromDocument();
     virtual void attributeChanged(Attribute*, bool preserveDecls = false);
-    
+
     virtual bool isURLAttribute(Attribute*) const;
     virtual const QualifiedName& imageSourceAttributeName() const;
 
     virtual RenderWidget* renderWidgetForJSBindings() const;
 
+    virtual void updateWidget(bool onlyCreateNonNetscapePlugins);
+
     virtual void addSubresourceAttributeURLs(ListHashSet<KURL>&) const;
 
     void parametersForPlugin(Vector<String>& paramNames, Vector<String>& paramValues);
index c970293..56a6095 100644 (file)
@@ -240,6 +240,7 @@ bool HTMLObjectElement::hasFallbackContent() const
 // moved down into HTMLPluginImageElement.cpp
 void HTMLObjectElement::updateWidget(bool onlyCreateNonNetscapePlugins)
 {
+    ASSERT(!renderEmbeddedObject()->pluginCrashedOrWasMissing());
     // FIXME: We should ASSERT(needsWidgetUpdate()), but currently
     // FrameView::updateWidget() calls updateWidget(false) without checking if
     // the widget actually needs updating!
index 36c3c4b..2d416c3 100644 (file)
@@ -37,8 +37,6 @@ public:
 
     bool containsJavaApplet() const;
 
-    void updateWidget(bool onlyCreateNonNetscapePlugins);
-
     virtual bool useFallbackContent() const { return m_useFallbackContent; }
     void renderFallbackContent();
 
@@ -60,6 +58,7 @@ private:
 
     virtual void addSubresourceAttributeURLs(ListHashSet<KURL>&) const;
 
+    virtual void updateWidget(bool onlyCreateNonNetscapePlugins);
     void updateDocNamedItem();
 
     bool hasFallbackContent() const;
index 57a70a9..9ac5ad8 100644 (file)
@@ -159,8 +159,15 @@ void HTMLPlugInImageElement::detach()
 void HTMLPlugInImageElement::updateWidgetIfNecessary()
 {
     document()->updateStyleIfNeeded();
-    if (needsWidgetUpdate() && renderEmbeddedObject() && !useFallbackContent() && !isImageType())
-        renderEmbeddedObject()->updateWidget(true);
+
+    if (!needsWidgetUpdate() || useFallbackContent() || isImageType())
+        return;
+
+    if (!renderEmbeddedObject() || renderEmbeddedObject()->pluginCrashedOrWasMissing())
+        return;
+
+    // True indicates that this code path should only create non-netscape plugins (no clue why).
+    updateWidget(true);
 }
 
 void HTMLPlugInImageElement::finishParsingChildren()
index 78bdeb3..60ad0e6 100644 (file)
@@ -32,16 +32,18 @@ class FrameLoader;
 // Base class for HTMLObjectElement and HTMLEmbedElement
 class HTMLPlugInImageElement : public HTMLPlugInElement {
 public:
-    const String& serviceType() const { return m_serviceType; }
-    const String& url() const { return m_url; }
-
     RenderEmbeddedObject* renderEmbeddedObject() const;
 
+    virtual void updateWidget(bool onlyCreateNonNetscapePlugins) = 0;
+
 protected:
     HTMLPlugInImageElement(const QualifiedName& tagName, Document*, bool createdByParser);
 
     bool isImageType();
 
+    const String& serviceType() const { return m_serviceType; }
+    const String& url() const { return m_url; }
+
     OwnPtr<HTMLImageLoader> m_imageLoader;
     String m_serviceType;
     String m_url;
index d6b55c1..65fa49f 100644 (file)
@@ -44,6 +44,7 @@
 #include "HTMLFrameElement.h"
 #include "HTMLFrameSetElement.h"
 #include "HTMLNames.h"
+#include "HTMLPlugInImageElement.h"
 #include "InspectorTimelineAgent.h"
 #include "OverflowEvent.h"
 #include "RenderEmbeddedObject.h"
@@ -1573,6 +1574,37 @@ void FrameView::scrollToAnchor()
     m_maintainScrollPositionAnchor = anchorNode;
 }
 
+void FrameView::updateWidget(RenderEmbeddedObject* object)
+{
+    ASSERT(!object->node() || object->node()->isElementNode());
+    Element* ownerElement = static_cast<Element*>(object->node());
+    // The object may have already been destroyed (thus node cleared),
+    // but FrameView holds a manual ref, so it won't have been deleted.
+    ASSERT(m_widgetUpdateSet->contains(object));
+    if (!ownerElement)
+        return;
+
+    // No need to update if it's already crashed or known to be missing.
+    if (object->pluginCrashedOrWasMissing())
+        return;
+
+    // FIXME: This could turn into a real virtual dispatch if we defined
+    // updateWidget(bool) on HTMLElement.
+    if (ownerElement->hasTagName(objectTag) || ownerElement->hasTagName(embedTag))
+        static_cast<HTMLPlugInImageElement*>(ownerElement)->updateWidget(false);
+    // FIXME: It is not clear that Media elements need or want this updateWidget() call.
+#if ENABLE(PLUGIN_PROXY_FOR_VIDEO)
+    else if (ownerElement->hasTagName(videoTag) || ownerElement->hasTagName(audioTag))
+        static_cast<HTMLMediaElement*>(ownerElement)->updateWidget(false);
+#endif
+    else
+        ASSERT_NOT_REACHED();
+
+    // Caution: it's possible the object was destroyed again, since loading a
+    // plugin may run any arbitrary javascript.
+    object->updateWidgetPosition();
+}
+
 bool FrameView::updateWidgets()
 {
     if (m_nestedLayoutCount > 1 || !m_widgetUpdateSet || m_widgetUpdateSet->isEmpty())
@@ -1591,11 +1623,7 @@ bool FrameView::updateWidgets()
 
     for (size_t i = 0; i < size; ++i) {
         RenderEmbeddedObject* object = objects[i];
-
-        // The object may have been destroyed, but our manual ref() keeps the object from being deleted.
-        object->updateWidget(false);
-        object->updateWidgetPosition();
-
+        updateWidget(object);
         m_widgetUpdateSet->remove(object);
     }
 
index 463020a..f9a4bf3 100644 (file)
@@ -280,6 +280,7 @@ private:
     double adjustedDeferredRepaintDelay() const;
 
     bool updateWidgets();
+    void updateWidget(RenderEmbeddedObject*);
     void scrollToAnchor();
     void scrollPositionChanged();
 
index 43c8d22..d08174d 100644 (file)
@@ -102,30 +102,6 @@ bool RenderEmbeddedObject::allowsAcceleratedCompositing() const
 }
 #endif
 
-// FIXME: This should be moved into FrameView (the only caller)
-void RenderEmbeddedObject::updateWidget(bool onlyCreateNonNetscapePlugins)
-{
-    if (!m_replacementText.isNull() || !node()) // Check the node in case destroy() has been called.
-        return;
-
-    // The calls to SubframeLoader::requestObject within this function can result in a plug-in being initialized.
-    // This can run cause arbitrary JavaScript to run and may result in this RenderObject being detached from
-    // the render tree and destroyed, causing a crash like <rdar://problem/6954546>.  By extending our lifetime
-    // artifically to ensure that we remain alive for the duration of plug-in initialization.
-    RenderWidgetProtector protector(this);
-
-    if (node()->hasTagName(objectTag))
-        static_cast<HTMLObjectElement*>(node())->updateWidget(onlyCreateNonNetscapePlugins);
-    else if (node()->hasTagName(embedTag))
-        static_cast<HTMLEmbedElement*>(node())->updateWidget(onlyCreateNonNetscapePlugins);
-#if ENABLE(PLUGIN_PROXY_FOR_VIDEO)        
-    else if (node()->hasTagName(videoTag) || node()->hasTagName(audioTag))
-        static_cast<HTMLMediaElement*>(node())->updateWidget(onlyCreateNonNetscapePlugins);
-#endif
-    else
-        ASSERT_NOT_REACHED();
-}
-
 void RenderEmbeddedObject::setShowsMissingPluginIndicator()
 {
     ASSERT(m_replacementText.isEmpty());
@@ -139,6 +115,11 @@ void RenderEmbeddedObject::setShowsCrashedPluginIndicator()
     m_replacementText = crashedPluginText();
 }
 
+bool RenderEmbeddedObject::pluginCrashedOrWasMissing() const
+{
+    return !m_replacementText.isNull();
+}
+    
 void RenderEmbeddedObject::setMissingPluginIndicatorIsPressed(bool pressed)
 {
     if (m_missingPluginIndicatorIsPressed == pressed)
@@ -150,7 +131,7 @@ void RenderEmbeddedObject::setMissingPluginIndicatorIsPressed(bool pressed)
 
 void RenderEmbeddedObject::paint(PaintInfo& paintInfo, int tx, int ty)
 {
-    if (!m_replacementText.isNull()) {
+    if (pluginCrashedOrWasMissing()) {
         RenderReplaced::paint(paintInfo, tx, ty);
         return;
     }
@@ -160,7 +141,7 @@ void RenderEmbeddedObject::paint(PaintInfo& paintInfo, int tx, int ty)
 
 void RenderEmbeddedObject::paintReplaced(PaintInfo& paintInfo, int tx, int ty)
 {
-    if (!m_replacementText)
+    if (pluginCrashedOrWasMissing())
         return;
 
     if (paintInfo.phase == PaintPhaseSelection)
index 6376f93..8d09ede 100644 (file)
@@ -36,7 +36,8 @@ public:
     RenderEmbeddedObject(Element*);
     virtual ~RenderEmbeddedObject();
 
-    void updateWidget(bool onlyCreateNonNetscapePlugins);
+    bool pluginCrashedOrWasMissing() const;
+    
     void setShowsMissingPluginIndicator();
     void setShowsCrashedPluginIndicator();
     bool showsMissingPluginIndicator() const { return m_showsMissingPluginIndicator; }