summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>2012-04-09 11:42:18 (GMT)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>2012-04-09 11:42:18 (GMT)
commit9fb90af3cebd0e595990cded0941d230cf77dcc1 (patch)
tree281ef76123b8f5b190c53b79915f1bb9b8505d4f
parentcb2a03d8aebe8f95d4885c5f43b3e391cd0397aa (diff)
Remove partially implemented per-Element visibility checks from requestAnimationFrame logic
https://bugs.webkit.org/show_bug.cgi?id=74232 Patch by James Robinson <jamesr@chromium.org> on 2012-04-09 Reviewed by Dean Jackson. Source/WebCore: The initial requestAnimationFrame implementation had an Element parameter as the second argument to the function. This element was intended to convey the element associated with the animation so that when the element was not visible the animation callback would not be run. The checked in implementation does a very limited check - testing for display:none and being detached from the tree - but does it in a way that does not work correctly if an element's visibility is manipulated by a callback running from a different document. It also adds significant complexity to the code, making it less hackable and easy to introduce subtle security bugs or infinite loops. This patch removes the parameter. Since it has always been marked optional, there is no web compat risk. If this functionality is added back in the future it needs to be implemented in a way that considers all callbacks within a Page and not only those within a single Document. * dom/Document.cpp: (WebCore::Document::webkitRequestAnimationFrame): * dom/Document.h: * dom/RequestAnimationFrameCallback.h: * dom/ScriptedAnimationController.cpp: (WebCore::ScriptedAnimationController::registerCallback): (WebCore::ScriptedAnimationController::serviceScriptedAnimations): * dom/ScriptedAnimationController.h: * page/DOMWindow.cpp: (WebCore::DOMWindow::webkitRequestAnimationFrame): * page/DOMWindow.h: * page/DOMWindow.idl: LayoutTests: Remove tests for removed functionality. * fast/animation/request-animation-frame-display-expected.txt: Removed. * fast/animation/request-animation-frame-display.html: Removed. * fast/animation/script-tests/request-animation-frame-display.js: Removed. git-svn-id: http://svn.webkit.org/repository/webkit/trunk@113573 268f45cc-cd09-0410-ab3c-d52691b4dbfc
-rw-r--r--LayoutTests/ChangeLog13
-rw-r--r--LayoutTests/fast/animation/request-animation-frame-display-expected.txt10
-rw-r--r--LayoutTests/fast/animation/request-animation-frame-display.html11
-rw-r--r--LayoutTests/fast/animation/script-tests/request-animation-frame-display.js29
-rw-r--r--Source/WebCore/ChangeLog33
-rw-r--r--Source/WebCore/dom/Document.cpp4
-rw-r--r--Source/WebCore/dom/Document.h2
-rw-r--r--Source/WebCore/dom/RequestAnimationFrameCallback.h3
-rw-r--r--Source/WebCore/dom/ScriptedAnimationController.cpp46
-rw-r--r--Source/WebCore/dom/ScriptedAnimationController.h3
-rw-r--r--Source/WebCore/page/DOMWindow.cpp4
-rw-r--r--Source/WebCore/page/DOMWindow.h2
-rw-r--r--Source/WebCore/page/DOMWindow.idl2
13 files changed, 64 insertions, 98 deletions
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index 223fd52..568a9be 100644
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,3 +1,16 @@
+2012-04-09 James Robinson <jamesr@chromium.org>
+
+ Remove partially implemented per-Element visibility checks from requestAnimationFrame logic
+ https://bugs.webkit.org/show_bug.cgi?id=74232
+
+ Reviewed by Dean Jackson.
+
+ Remove tests for removed functionality.
+
+ * fast/animation/request-animation-frame-display-expected.txt: Removed.
+ * fast/animation/request-animation-frame-display.html: Removed.
+ * fast/animation/script-tests/request-animation-frame-display.js: Removed.
+
2012-04-09 Ami Fischman <fischman@chromium.org>
Layout Test http/tests/media/media-document.html is flaky
diff --git a/LayoutTests/fast/animation/request-animation-frame-display-expected.txt b/LayoutTests/fast/animation/request-animation-frame-display-expected.txt
deleted file mode 100644
index e982638..0000000
--- a/LayoutTests/fast/animation/request-animation-frame-display-expected.txt
+++ /dev/null
@@ -1,10 +0,0 @@
-Tests requestAnimationFrame callback handling of display: property changed within another callback
-
-On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
-
-
-PASS callbackInvokedOnA is true
-PASS successfullyParsed is true
-
-TEST COMPLETE
-
diff --git a/LayoutTests/fast/animation/request-animation-frame-display.html b/LayoutTests/fast/animation/request-animation-frame-display.html
deleted file mode 100644
index 782d3e0..0000000
--- a/LayoutTests/fast/animation/request-animation-frame-display.html
+++ /dev/null
@@ -1,11 +0,0 @@
-<!DOCTYPE html>
-<html>
-<head>
-<script src="../js/resources/js-test-pre.js"></script>
-</head>
-<body>
-<span id="e"></span>
-<span id="f"></span>
-<script src="script-tests/request-animation-frame-display.js"></script>
-</body>
-</html>
diff --git a/LayoutTests/fast/animation/script-tests/request-animation-frame-display.js b/LayoutTests/fast/animation/script-tests/request-animation-frame-display.js
deleted file mode 100644
index bcd6e99..0000000
--- a/LayoutTests/fast/animation/script-tests/request-animation-frame-display.js
+++ /dev/null
@@ -1,29 +0,0 @@
-description("Tests requestAnimationFrame callback handling of display: property changed within another callback");
-
-var e = document.getElementById("e");
-e.style.display="none";
-var callbackInvokedOnA=false;
-window.webkitRequestAnimationFrame(function() {
- callbackInvokedOnA=true;
-}, e);
-
-var f = document.getElementById("f");
-window.webkitRequestAnimationFrame(function() {
- e.style.display="";
-}, f);
-
-if (window.layoutTestController)
- layoutTestController.display();
-
-setTimeout(function() {
- shouldBeTrue("callbackInvokedOnA");
-}, 100);
-
-if (window.layoutTestController)
- layoutTestController.waitUntilDone();
-
-setTimeout(function() {
- isSuccessfullyParsed();
- if (window.layoutTestController)
- layoutTestController.notifyDone();
-}, 200);
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 11a5aeb..430ea6c 100644
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,36 @@
+2012-04-09 James Robinson <jamesr@chromium.org>
+
+ Remove partially implemented per-Element visibility checks from requestAnimationFrame logic
+ https://bugs.webkit.org/show_bug.cgi?id=74232
+
+ Reviewed by Dean Jackson.
+
+ The initial requestAnimationFrame implementation had an Element parameter as the second argument to the
+ function. This element was intended to convey the element associated with the animation so that when the element
+ was not visible the animation callback would not be run. The checked in implementation does a very limited check
+ - testing for display:none and being detached from the tree - but does it in a way that does not work correctly
+ if an element's visibility is manipulated by a callback running from a different document. It also adds
+ significant complexity to the code, making it less hackable and easy to introduce subtle security bugs or
+ infinite loops.
+
+ This patch removes the parameter. Since it has always been marked optional, there is no web compat risk.
+
+ If this functionality is added back in the future it needs to be implemented in a way that considers all
+ callbacks within a Page and not only those within a single Document.
+
+ * dom/Document.cpp:
+ (WebCore::Document::webkitRequestAnimationFrame):
+ * dom/Document.h:
+ * dom/RequestAnimationFrameCallback.h:
+ * dom/ScriptedAnimationController.cpp:
+ (WebCore::ScriptedAnimationController::registerCallback):
+ (WebCore::ScriptedAnimationController::serviceScriptedAnimations):
+ * dom/ScriptedAnimationController.h:
+ * page/DOMWindow.cpp:
+ (WebCore::DOMWindow::webkitRequestAnimationFrame):
+ * page/DOMWindow.h:
+ * page/DOMWindow.idl:
+
2012-04-09 Chris Guan <chris.guan@torchmobile.com.cn>
[Blackberry] m_isRequestedByPlugin should be copied in ResourceRequest
diff --git a/Source/WebCore/dom/Document.cpp b/Source/WebCore/dom/Document.cpp
index 8aad1a3..ec5effc 100644
--- a/Source/WebCore/dom/Document.cpp
+++ b/Source/WebCore/dom/Document.cpp
@@ -5643,7 +5643,7 @@ void Document::loadEventDelayTimerFired(Timer<Document>*)
}
#if ENABLE(REQUEST_ANIMATION_FRAME)
-int Document::webkitRequestAnimationFrame(PassRefPtr<RequestAnimationFrameCallback> callback, Element* animationElement)
+int Document::webkitRequestAnimationFrame(PassRefPtr<RequestAnimationFrameCallback> callback)
{
if (!m_scriptedAnimationController) {
#if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR)
@@ -5658,7 +5658,7 @@ int Document::webkitRequestAnimationFrame(PassRefPtr<RequestAnimationFrameCallba
m_scriptedAnimationController->suspend();
}
- return m_scriptedAnimationController->registerCallback(callback, animationElement);
+ return m_scriptedAnimationController->registerCallback(callback);
}
void Document::webkitCancelAnimationFrame(int id)
diff --git a/Source/WebCore/dom/Document.h b/Source/WebCore/dom/Document.h
index 9f74c0f..4e76478 100644
--- a/Source/WebCore/dom/Document.h
+++ b/Source/WebCore/dom/Document.h
@@ -1106,7 +1106,7 @@ public:
const DocumentTiming* timing() const { return &m_documentTiming; }
#if ENABLE(REQUEST_ANIMATION_FRAME)
- int webkitRequestAnimationFrame(PassRefPtr<RequestAnimationFrameCallback>, Element*);
+ int webkitRequestAnimationFrame(PassRefPtr<RequestAnimationFrameCallback>);
void webkitCancelAnimationFrame(int id);
void serviceScriptedAnimations(DOMTimeStamp);
#endif
diff --git a/Source/WebCore/dom/RequestAnimationFrameCallback.h b/Source/WebCore/dom/RequestAnimationFrameCallback.h
index 3edeb9e..420f023 100644
--- a/Source/WebCore/dom/RequestAnimationFrameCallback.h
+++ b/Source/WebCore/dom/RequestAnimationFrameCallback.h
@@ -31,8 +31,6 @@
#ifndef RequestAnimationFrameCallback_h
#define RequestAnimationFrameCallback_h
-#include "Element.h"
-#include <wtf/PassRefPtr.h>
#include <wtf/RefCounted.h>
namespace WebCore {
@@ -42,7 +40,6 @@ public:
virtual ~RequestAnimationFrameCallback() { }
virtual bool handleEvent(DOMTimeStamp) = 0;
- RefPtr<Element> m_element;
int m_id;
bool m_firedOrCancelled;
};
diff --git a/Source/WebCore/dom/ScriptedAnimationController.cpp b/Source/WebCore/dom/ScriptedAnimationController.cpp
index 807e913..380e0b2 100644
--- a/Source/WebCore/dom/ScriptedAnimationController.cpp
+++ b/Source/WebCore/dom/ScriptedAnimationController.cpp
@@ -29,7 +29,6 @@
#if ENABLE(REQUEST_ANIMATION_FRAME)
#include "Document.h"
-#include "Element.h"
#include "FrameView.h"
#include "InspectorInstrumentation.h"
#include "RequestAnimationFrameCallback.h"
@@ -81,12 +80,11 @@ void ScriptedAnimationController::resume()
scheduleAnimation();
}
-ScriptedAnimationController::CallbackId ScriptedAnimationController::registerCallback(PassRefPtr<RequestAnimationFrameCallback> callback, Element* animationElement)
+ScriptedAnimationController::CallbackId ScriptedAnimationController::registerCallback(PassRefPtr<RequestAnimationFrameCallback> callback)
{
ScriptedAnimationController::CallbackId id = m_nextCallbackId++;
callback->m_firedOrCancelled = false;
callback->m_id = id;
- callback->m_element = animationElement;
m_callbacks.append(callback);
InspectorInstrumentation::didRequestAnimationFrame(m_document, id);
@@ -112,48 +110,24 @@ void ScriptedAnimationController::serviceScriptedAnimations(DOMTimeStamp time)
{
if (!m_callbacks.size() || m_suspendCount)
return;
- // We want to run the callback for all elements in the document that have registered
- // for a callback and that are visible. Running the callbacks can cause new callbacks
- // to be registered, existing callbacks to be cancelled, and elements to gain or lose
- // visibility so this code has to iterate carefully.
-
- // FIXME: Currently, this code doesn't do any visibility tests beyond checking display:
// First, generate a list of callbacks to consider. Callbacks registered from this point
// on are considered only for the "next" frame, not this one.
CallbackList callbacks(m_callbacks);
- // Firing the callback may cause the visibility of other elements to change. To avoid
- // missing any callbacks, we keep iterating through the list of candiate callbacks and firing
- // them until nothing new becomes visible.
- bool firedCallback;
-
// Invoking callbacks may detach elements from our document, which clear's the document's
// reference to us, so take a defensive reference.
RefPtr<ScriptedAnimationController> protector(this);
- do {
- firedCallback = false;
- // A previous iteration may have detached this Document from the DOM tree.
- // If so, then we do not need to process any more callbacks.
- if (!m_document)
- continue;
-
- // A previous iteration may have invalidated style (or layout). Update styles for each iteration
- // for now since all we check is the existence of a renderer.
- m_document->updateStyleIfNeeded();
- for (size_t i = 0; i < callbacks.size(); ++i) {
- RequestAnimationFrameCallback* callback = callbacks[i].get();
- if (!callback->m_firedOrCancelled && (!callback->m_element || callback->m_element->renderer())) {
- callback->m_firedOrCancelled = true;
- InspectorInstrumentationCookie cookie = InspectorInstrumentation::willFireAnimationFrame(m_document, callback->m_id);
- callback->handleEvent(time);
- InspectorInstrumentation::didFireAnimationFrame(cookie);
- firedCallback = true;
- callbacks.remove(i);
- break;
- }
+
+ for (size_t i = 0; i < callbacks.size(); ++i) {
+ RequestAnimationFrameCallback* callback = callbacks[i].get();
+ if (!callback->m_firedOrCancelled) {
+ callback->m_firedOrCancelled = true;
+ InspectorInstrumentationCookie cookie = InspectorInstrumentation::willFireAnimationFrame(m_document, callback->m_id);
+ callback->handleEvent(time);
+ InspectorInstrumentation::didFireAnimationFrame(cookie);
}
- } while (firedCallback);
+ }
// Remove any callbacks we fired from the list of pending callbacks.
for (size_t i = 0; i < m_callbacks.size();) {
diff --git a/Source/WebCore/dom/ScriptedAnimationController.h b/Source/WebCore/dom/ScriptedAnimationController.h
index d2c9c0c..51c36b9 100644
--- a/Source/WebCore/dom/ScriptedAnimationController.h
+++ b/Source/WebCore/dom/ScriptedAnimationController.h
@@ -42,7 +42,6 @@
namespace WebCore {
class Document;
-class Element;
class RequestAnimationFrameCallback;
class ScriptedAnimationController : public RefCounted<ScriptedAnimationController>
@@ -60,7 +59,7 @@ public:
typedef int CallbackId;
- CallbackId registerCallback(PassRefPtr<RequestAnimationFrameCallback>, Element*);
+ CallbackId registerCallback(PassRefPtr<RequestAnimationFrameCallback>);
void cancelCallback(CallbackId);
void serviceScriptedAnimations(DOMTimeStamp);
diff --git a/Source/WebCore/page/DOMWindow.cpp b/Source/WebCore/page/DOMWindow.cpp
index 9d4c3b4..1a8ad71 100644
--- a/Source/WebCore/page/DOMWindow.cpp
+++ b/Source/WebCore/page/DOMWindow.cpp
@@ -1485,10 +1485,10 @@ void DOMWindow::clearInterval(int timeoutId)
}
#if ENABLE(REQUEST_ANIMATION_FRAME)
-int DOMWindow::webkitRequestAnimationFrame(PassRefPtr<RequestAnimationFrameCallback> callback, Element* e)
+int DOMWindow::webkitRequestAnimationFrame(PassRefPtr<RequestAnimationFrameCallback> callback)
{
if (Document* d = document())
- return d->webkitRequestAnimationFrame(callback, e);
+ return d->webkitRequestAnimationFrame(callback);
return 0;
}
diff --git a/Source/WebCore/page/DOMWindow.h b/Source/WebCore/page/DOMWindow.h
index 7014d96..1d3ff60 100644
--- a/Source/WebCore/page/DOMWindow.h
+++ b/Source/WebCore/page/DOMWindow.h
@@ -252,7 +252,7 @@ namespace WebCore {
// WebKit animation extensions
#if ENABLE(REQUEST_ANIMATION_FRAME)
- int webkitRequestAnimationFrame(PassRefPtr<RequestAnimationFrameCallback>, Element*);
+ int webkitRequestAnimationFrame(PassRefPtr<RequestAnimationFrameCallback>);
void webkitCancelAnimationFrame(int id);
void webkitCancelRequestAnimationFrame(int id) { webkitCancelAnimationFrame(id); }
#endif
diff --git a/Source/WebCore/page/DOMWindow.idl b/Source/WebCore/page/DOMWindow.idl
index b239d15..4cb9067 100644
--- a/Source/WebCore/page/DOMWindow.idl
+++ b/Source/WebCore/page/DOMWindow.idl
@@ -213,7 +213,7 @@ module window {
#if defined(ENABLE_REQUEST_ANIMATION_FRAME)
// WebKit animation extensions, being standardized in the WebPerf WG
- long webkitRequestAnimationFrame(in [Callback] RequestAnimationFrameCallback callback, in [Optional=DefaultIsUndefined] Element element);
+ long webkitRequestAnimationFrame(in [Callback] RequestAnimationFrameCallback callback);
void webkitCancelAnimationFrame(in long id);
void webkitCancelRequestAnimationFrame(in long id); // This is a deprecated alias for webkitCancelAnimationFrame(). Remove this when removing vendor prefix.
#endif