REGRESSION (r102983): ClicktoFlash drawing of old style youtube embeds missing until...
authoraestes@apple.com <aestes@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 8 Feb 2012 20:20:41 +0000 (20:20 +0000)
committeraestes@apple.com <aestes@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 8 Feb 2012 20:20:41 +0000 (20:20 +0000)
https://bugs.webkit.org/show_bug.cgi?id=77167

Reviewed by Eric Seidel.

Source/WebCore:

Test: plugins/layout-in-beforeload-listener-affects-plugin-loading.html

r102983 made FrameView::updateWidgets() check if the DOM node actually
needs a widget update before calling updateWidget(). Due to historical
reasons, however, updateWidget() can be legitimately called twice: once
at attach time for non-Netscape plug-ins and once at layout time for
Netscape plug-ins.

If the widget represents a Netscape plug-in, but updateWidget() is
called for the CreateOnlyNonNetscapePlugins case after the DOM node was
marked as needing an update, updateWidget() will clear the update flag
and prevent a second call to updateWidget() at layout time for the
CreateAnyWidgetType case.

As much as I loathe adding to the code duplication between
HTMLEmbedElement::updateWidget() and HTMLObjectElement::updateWidget(),
the simplest solution seems to be marking the DOM node as needing
update in the case where we are calling updateWidget() for the
CreateOnlyNonNetscapePlugins case and we know we will be loading a
Netscape plug-in.

* html/HTMLEmbedElement.cpp:
(WebCore::HTMLEmbedElement::updateWidget): Call
setNeedsWidgetUpdate(true) if pluginCreationOption is
CreateOnlyNonNetscapePlugins but we will load a Netscape plug-in.
* html/HTMLObjectElement.cpp:
(WebCore::HTMLObjectElement::updateWidget): Ditto.
* html/HTMLPlugInElement.cpp:
(WebCore::HTMLPlugInElement::guardedDispatchBeforeLoadEvent): Remove an
invalid assertion that prevents the layout test from running in a Debug
configuration.

LayoutTests:

* plugins/layout-in-beforeload-listener-affects-plugin-loading-expected.txt: Added.
* plugins/layout-in-beforeload-listener-affects-plugin-loading.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/plugins/layout-in-beforeload-listener-affects-plugin-loading-expected.txt [new file with mode: 0644]
LayoutTests/plugins/layout-in-beforeload-listener-affects-plugin-loading.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/html/HTMLEmbedElement.cpp
Source/WebCore/html/HTMLObjectElement.cpp
Source/WebCore/html/HTMLPlugInElement.cpp

index 56446a5..09a7dab 100644 (file)
@@ -1,3 +1,13 @@
+2012-02-07  Andy Estes  <aestes@apple.com>
+
+        REGRESSION (r102983): ClicktoFlash drawing of old style youtube embeds missing until resize
+        https://bugs.webkit.org/show_bug.cgi?id=77167
+
+        Reviewed by Eric Seidel.
+
+        * plugins/layout-in-beforeload-listener-affects-plugin-loading-expected.txt: Added.
+        * plugins/layout-in-beforeload-listener-affects-plugin-loading.html: Added.
+
 2012-02-08  Cary Clark  <caryclark@google.com>
 
         Unreviewed rebaselines for vertical text tests.
diff --git a/LayoutTests/plugins/layout-in-beforeload-listener-affects-plugin-loading-expected.txt b/LayoutTests/plugins/layout-in-beforeload-listener-affects-plugin-loading-expected.txt
new file mode 100644 (file)
index 0000000..26a8cfc
--- /dev/null
@@ -0,0 +1,2 @@
+CONSOLE MESSAGE: line 27: PASS
diff --git a/LayoutTests/plugins/layout-in-beforeload-listener-affects-plugin-loading.html b/LayoutTests/plugins/layout-in-beforeload-listener-affects-plugin-loading.html
new file mode 100644 (file)
index 0000000..c5a4840
--- /dev/null
@@ -0,0 +1,42 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <script>
+        if (window.layoutTestController) {
+            layoutTestController.dumpAsText();
+            layoutTestController.waitUntilDone();
+        }
+        
+        window._beforeloadReceivedForEmbed = false;
+        
+        document.addEventListener("beforeload", function(event) {
+            if (document.getElementsByTagName("object")[0] == event.target) {
+                // trigger a layout
+                event.target.offsetWidth;
+                return;
+            }
+            
+            _beforeloadReceivedForEmbed = document.getElementsByTagName("embed")[0] == event.target;
+        }, true);
+        
+        function test() {
+            // The <embed> should load as a post-layout task that executes in
+            // the current run loop iteration. Queue a task to check if the
+            // <embed>'s beforeload event fired.
+            window.setTimeout(function() {
+                console.log(_beforeloadReceivedForEmbed ? "PASS" : "FAIL");
+                if (window.layoutTestController)
+                    layoutTestController.notifyDone();
+            }, 0);
+        }
+    </script>
+</head>
+<body onload="test();">
+    <!-- Test that performing layout in a beforeload listener does not affect
+         plug-in loading. See http://webkit.org/b/77167 for details. This test
+         requires TestNetscapePlugIn so it must be run in DumpRenderTree. -->
+    <object>
+        <embed type="application/x-webkit-test-netscape">
+    </object>
+</body>
+</html>
\ No newline at end of file
index a3d74ab..024ecbf 100644 (file)
@@ -1,3 +1,42 @@
+2012-02-07  Andy Estes  <aestes@apple.com>
+
+        REGRESSION (r102983): ClicktoFlash drawing of old style youtube embeds missing until resize
+        https://bugs.webkit.org/show_bug.cgi?id=77167
+
+        Reviewed by Eric Seidel.
+
+        Test: plugins/layout-in-beforeload-listener-affects-plugin-loading.html
+
+        r102983 made FrameView::updateWidgets() check if the DOM node actually
+        needs a widget update before calling updateWidget(). Due to historical
+        reasons, however, updateWidget() can be legitimately called twice: once
+        at attach time for non-Netscape plug-ins and once at layout time for
+        Netscape plug-ins.
+
+        If the widget represents a Netscape plug-in, but updateWidget() is
+        called for the CreateOnlyNonNetscapePlugins case after the DOM node was
+        marked as needing an update, updateWidget() will clear the update flag
+        and prevent a second call to updateWidget() at layout time for the
+        CreateAnyWidgetType case.
+
+        As much as I loathe adding to the code duplication between
+        HTMLEmbedElement::updateWidget() and HTMLObjectElement::updateWidget(),
+        the simplest solution seems to be marking the DOM node as needing
+        update in the case where we are calling updateWidget() for the
+        CreateOnlyNonNetscapePlugins case and we know we will be loading a
+        Netscape plug-in.
+
+        * html/HTMLEmbedElement.cpp:
+        (WebCore::HTMLEmbedElement::updateWidget): Call
+        setNeedsWidgetUpdate(true) if pluginCreationOption is
+        CreateOnlyNonNetscapePlugins but we will load a Netscape plug-in.
+        * html/HTMLObjectElement.cpp:
+        (WebCore::HTMLObjectElement::updateWidget): Ditto.
+        * html/HTMLPlugInElement.cpp:
+        (WebCore::HTMLPlugInElement::guardedDispatchBeforeLoadEvent): Remove an
+        invalid assertion that prevents the layout test from running in a Debug
+        configuration.
+
 2012-02-07  Ojan Vafai  <ojan@chromium.org>
 
         Floated flexboxes render as regular RenderBlocks
index 165efb9..1d64eca 100644 (file)
@@ -133,11 +133,15 @@ void HTMLEmbedElement::updateWidget(PluginCreationOption pluginCreationOption)
     // <object> which modifies url and serviceType before calling these.
     if (!allowedToLoadFrameURL(m_url))
         return;
+
     // FIXME: It's sadness that we have this special case here.
     //        See http://trac.webkit.org/changeset/25128 and
     //        plugins/netscape-plugin-setwindow-size.html
-    if (pluginCreationOption == CreateOnlyNonNetscapePlugins && wouldLoadAsNetscapePlugin(m_url, m_serviceType))
+    if (pluginCreationOption == CreateOnlyNonNetscapePlugins && wouldLoadAsNetscapePlugin(m_url, m_serviceType)) {
+        // Ensure updateWidget() is called again during layout to create the Netscape plug-in.
+        setNeedsWidgetUpdate(true);
         return;
+    }
 
     // FIXME: These should be joined into a PluginParameters class.
     Vector<String> paramNames;
index 544890b..e2bef01 100644 (file)
@@ -285,8 +285,14 @@ void HTMLObjectElement::updateWidget(PluginCreationOption pluginCreationOption)
     bool fallbackContent = hasFallbackContent();
     renderEmbeddedObject()->setHasFallbackContent(fallbackContent);
 
-    if (pluginCreationOption == CreateOnlyNonNetscapePlugins && wouldLoadAsNetscapePlugin(url, serviceType))
+    // FIXME: It's sadness that we have this special case here.
+    //        See http://trac.webkit.org/changeset/25128 and
+    //        plugins/netscape-plugin-setwindow-size.html
+    if (pluginCreationOption == CreateOnlyNonNetscapePlugins && wouldLoadAsNetscapePlugin(url, serviceType)) {
+        // Ensure updateWidget() is called again during layout to create the Netscape plug-in.
+        setNeedsWidgetUpdate(true);
         return;
+    }
 
     RefPtr<HTMLObjectElement> protect(this); // beforeload and plugin loading can make arbitrary DOM mutations.
     bool beforeLoadAllowedLoad = guardedDispatchBeforeLoadEvent(url);
index 691cede..66d6092 100644 (file)
@@ -112,7 +112,11 @@ PassScriptInstance HTMLPlugInElement::getInstance()
 
 bool HTMLPlugInElement::guardedDispatchBeforeLoadEvent(const String& sourceURL)
 {
-    ASSERT(!m_inBeforeLoadEventHandler);
+    // FIXME: Our current plug-in loading design can't guarantee the following
+    // assertion is true, since plug-in loading can be initiated during layout,
+    // and synchronous layout can be initiated in a beforeload event handler!
+    // See <http://webkit.org/b/71264>.
+    // ASSERT(!m_inBeforeLoadEventHandler);
     m_inBeforeLoadEventHandler = true;
     // static_cast is used to avoid a compile error since dispatchBeforeLoadEvent
     // is intentionally undefined on this class.