Changeset 113573 in webkit


Ignore:
Timestamp:
Apr 9, 2012 4:42:18 AM (12 years ago)
Author:
commit-queue@webkit.org
Message:

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.
Location:
trunk
Files:
3 deleted
10 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r113567 r113573  
     12012-04-09  James Robinson  <jamesr@chromium.org>
     2
     3        Remove partially implemented per-Element visibility checks from requestAnimationFrame logic
     4        https://bugs.webkit.org/show_bug.cgi?id=74232
     5
     6        Reviewed by Dean Jackson.
     7
     8        Remove tests for removed functionality.
     9
     10        * fast/animation/request-animation-frame-display-expected.txt: Removed.
     11        * fast/animation/request-animation-frame-display.html: Removed.
     12        * fast/animation/script-tests/request-animation-frame-display.js: Removed.
     13
    1142012-04-09  Ami Fischman  <fischman@chromium.org>
    215

  • trunk/Source/WebCore/ChangeLog

    r113569 r113573  
     12012-04-09  James Robinson  <jamesr@chromium.org>
     2
     3        Remove partially implemented per-Element visibility checks from requestAnimationFrame logic
     4        https://bugs.webkit.org/show_bug.cgi?id=74232
     5
     6        Reviewed by Dean Jackson.
     7
     8        The initial requestAnimationFrame implementation had an Element parameter as the second argument to the
     9        function. This element was intended to convey the element associated with the animation so that when the element
     10        was not visible the animation callback would not be run. The checked in implementation does a very limited check
     11        - testing for display:none and being detached from the tree - but does it in a way that does not work correctly
     12        if an element's visibility is manipulated by a callback running from a different document. It also adds
     13        significant complexity to the code, making it less hackable and easy to introduce subtle security bugs or
     14        infinite loops.
     15
     16        This patch removes the parameter. Since it has always been marked optional, there is no web compat risk.
     17
     18        If this functionality is added back in the future it needs to be implemented in a way that considers all
     19        callbacks within a Page and not only those within a single Document.
     20
     21        * dom/Document.cpp:
     22        (WebCore::Document::webkitRequestAnimationFrame):
     23        * dom/Document.h:
     24        * dom/RequestAnimationFrameCallback.h:
     25        * dom/ScriptedAnimationController.cpp:
     26        (WebCore::ScriptedAnimationController::registerCallback):
     27        (WebCore::ScriptedAnimationController::serviceScriptedAnimations):
     28        * dom/ScriptedAnimationController.h:
     29        * page/DOMWindow.cpp:
     30        (WebCore::DOMWindow::webkitRequestAnimationFrame):
     31        * page/DOMWindow.h:
     32        * page/DOMWindow.idl:
     33
    1342012-04-09  Chris Guan  <chris.guan@torchmobile.com.cn>
    235

  • trunk/Source/WebCore/dom/Document.cpp

    r113503 r113573  
    56445644
    56455645#if ENABLE(REQUEST_ANIMATION_FRAME)
    5646 int Document::webkitRequestAnimationFrame(PassRefPtr<RequestAnimationFrameCallback> callback, Element* animationElement)
     5646int Document::webkitRequestAnimationFrame(PassRefPtr<RequestAnimationFrameCallback> callback)
    56475647{
    56485648    if (!m_scriptedAnimationController) {
    … …  
    56595659    }
    56605660
    5661     return m_scriptedAnimationController->registerCallback(callback, animationElement);
     5661    return m_scriptedAnimationController->registerCallback(callback);
    56625662}
    56635663

  • trunk/Source/WebCore/dom/Document.h

    r113503 r113573  
    11071107
    11081108#if ENABLE(REQUEST_ANIMATION_FRAME)
    1109     int webkitRequestAnimationFrame(PassRefPtr<RequestAnimationFrameCallback>, Element*);
     1109    int webkitRequestAnimationFrame(PassRefPtr<RequestAnimationFrameCallback>);
    11101110    void webkitCancelAnimationFrame(int id);
    11111111    void serviceScriptedAnimations(DOMTimeStamp);

  • trunk/Source/WebCore/dom/RequestAnimationFrameCallback.h

    r95901 r113573  
    3232#define RequestAnimationFrameCallback_h
    3333
    34 #include "Element.h"
    35 #include <wtf/PassRefPtr.h>
    3634#include <wtf/RefCounted.h>
    3735
    … …  
    4341    virtual bool handleEvent(DOMTimeStamp) = 0;
    4442
    45     RefPtr<Element> m_element;
    4643    int m_id;
    4744    bool m_firedOrCancelled;

  • trunk/Source/WebCore/dom/ScriptedAnimationController.cpp

    r113381 r113573  
    3030
    3131#include "Document.h"
    32 #include "Element.h"
    3332#include "FrameView.h"
    3433#include "InspectorInstrumentation.h"
    … …  
    8281}
    8382
    84 ScriptedAnimationController::CallbackId ScriptedAnimationController::registerCallback(PassRefPtr<RequestAnimationFrameCallback> callback, Element* animationElement)
     83ScriptedAnimationController::CallbackId ScriptedAnimationController::registerCallback(PassRefPtr<RequestAnimationFrameCallback> callback)
    8584{
    8685    ScriptedAnimationController::CallbackId id = m_nextCallbackId++;
    8786    callback->m_firedOrCancelled = false;
    8887    callback->m_id = id;
    89     callback->m_element = animationElement;
    9088    m_callbacks.append(callback);
    9189
    … …  
    113111    if (!m_callbacks.size() || m_suspendCount)
    114112        return;
    115     // We want to run the callback for all elements in the document that have registered
    116     // for a callback and that are visible.  Running the callbacks can cause new callbacks
    117     // to be registered, existing callbacks to be cancelled, and elements to gain or lose
    118     // visibility so this code has to iterate carefully.
    119 
    120     // FIXME: Currently, this code doesn't do any visibility tests beyond checking display:
    121113
    122114    // First, generate a list of callbacks to consider.  Callbacks registered from this point
    … …  
    124116    CallbackList callbacks(m_callbacks);
    125117
    126     // Firing the callback may cause the visibility of other elements to change.  To avoid
    127     // missing any callbacks, we keep iterating through the list of candiate callbacks and firing
    128     // them until nothing new becomes visible.
    129     bool firedCallback;
    130 
    131118    // Invoking callbacks may detach elements from our document, which clear's the document's
    132119    // reference to us, so take a defensive reference.
    133120    RefPtr<ScriptedAnimationController> protector(this);
    134     do {
    135         firedCallback = false;
    136         // A previous iteration may have detached this Document from the DOM tree.
    137         // If so, then we do not need to process any more callbacks.
    138         if (!m_document)
    139             continue;
    140121
    141         // A previous iteration may have invalidated style (or layout).  Update styles for each iteration
    142         // for now since all we check is the existence of a renderer.
    143         m_document->updateStyleIfNeeded();
    144         for (size_t i = 0; i < callbacks.size(); ++i) {
    145             RequestAnimationFrameCallback* callback = callbacks[i].get();
    146             if (!callback->m_firedOrCancelled && (!callback->m_element || callback->m_element->renderer())) {
    147                 callback->m_firedOrCancelled = true;
    148                 InspectorInstrumentationCookie cookie = InspectorInstrumentation::willFireAnimationFrame(m_document, callback->m_id);
    149                 callback->handleEvent(time);
    150                 InspectorInstrumentation::didFireAnimationFrame(cookie);
    151                 firedCallback = true;
    152                 callbacks.remove(i);
    153                 break;
    154             }
     122    for (size_t i = 0; i < callbacks.size(); ++i) {
     123        RequestAnimationFrameCallback* callback = callbacks[i].get();
     124        if (!callback->m_firedOrCancelled) {
     125            callback->m_firedOrCancelled = true;
     126            InspectorInstrumentationCookie cookie = InspectorInstrumentation::willFireAnimationFrame(m_document, callback->m_id);
     127            callback->handleEvent(time);
     128            InspectorInstrumentation::didFireAnimationFrame(cookie);
    155129        }
    156     } while (firedCallback);
     130    }
    157131
    158132    // Remove any callbacks we fired from the list of pending callbacks.

  • trunk/Source/WebCore/dom/ScriptedAnimationController.h

    r102405 r113573  
    4343
    4444class Document;
    45 class Element;
    4645class RequestAnimationFrameCallback;
    4746
    … …  
    6160    typedef int CallbackId;
    6261
    63     CallbackId registerCallback(PassRefPtr<RequestAnimationFrameCallback>, Element*);
     62    CallbackId registerCallback(PassRefPtr<RequestAnimationFrameCallback>);
    6463    void cancelCallback(CallbackId);
    6564    void serviceScriptedAnimations(DOMTimeStamp);

  • trunk/Source/WebCore/page/DOMWindow.cpp

    r112395 r113573  
    14861486
    14871487#if ENABLE(REQUEST_ANIMATION_FRAME)
    1488 int DOMWindow::webkitRequestAnimationFrame(PassRefPtr<RequestAnimationFrameCallback> callback, Element* e)
     1488int DOMWindow::webkitRequestAnimationFrame(PassRefPtr<RequestAnimationFrameCallback> callback)
    14891489{
    14901490    if (Document* d = document())
    1491         return d->webkitRequestAnimationFrame(callback, e);
     1491        return d->webkitRequestAnimationFrame(callback);
    14921492    return 0;
    14931493}

  • trunk/Source/WebCore/page/DOMWindow.h

    r111836 r113573  
    253253        // WebKit animation extensions
    254254#if ENABLE(REQUEST_ANIMATION_FRAME)
    255         int webkitRequestAnimationFrame(PassRefPtr<RequestAnimationFrameCallback>, Element*);
     255        int webkitRequestAnimationFrame(PassRefPtr<RequestAnimationFrameCallback>);
    256256        void webkitCancelAnimationFrame(int id);
    257257        void webkitCancelRequestAnimationFrame(int id) { webkitCancelAnimationFrame(id); }

  • trunk/Source/WebCore/page/DOMWindow.idl

    r113092 r113573  
    214214#if defined(ENABLE_REQUEST_ANIMATION_FRAME)
    215215        // WebKit animation extensions, being standardized in the WebPerf WG
    216         long webkitRequestAnimationFrame(in [Callback] RequestAnimationFrameCallback callback, in [Optional=DefaultIsUndefined] Element element);
     216        long webkitRequestAnimationFrame(in [Callback] RequestAnimationFrameCallback callback);
    217217        void webkitCancelAnimationFrame(in long id);
    218218        void webkitCancelRequestAnimationFrame(in long id); // This is a deprecated alias for webkitCancelAnimationFrame(). Remove this when removing vendor prefix.
Note: See TracChangeset for help on using the changeset viewer.