Changeset 251742 in webkit


Ignore:
Timestamp:
Oct 29, 2019 4:07:16 PM (4 years ago)
Author:
commit-queue@webkit.org
Message:

WebAnimation should never prevent entering the back/forward cache
​https://bugs.webkit.org/show_bug.cgi?id=203088
<rdar://problem/56374249>

Patch by Antoine Quint <Antoine Quint> on 2019-10-29
Reviewed by Antti Koivisto.

Source/WebCore:

Test: webanimations/animation-page-cache.html

We remove the Web Animation override of the deprecated method ActiveDOMObject::shouldPreventEnteringBackForwardCache_DEPRECATED()
and instead ensure event dispatch is suspended along with the WebAnimation object through the adoption of a SuspendableTaskQueue.

We also ensure an animation correctly suspends itself when ActiveDOMObject::suspend() and ActiveDOMObject::resume() are called.
Implementing these methods showed that we have some placeholders in DeclarativeAnimation that were not necessary, so we remove those.

Finally, we no longer need to track the stopped state since the SuspendableTaskQueue will close itself when ActiveDOMObject::stop()
is called.

  • animation/DeclarativeAnimation.cpp:

(WebCore::DeclarativeAnimation::stop): Deleted.
(WebCore::DeclarativeAnimation::suspend): Deleted.
(WebCore::DeclarativeAnimation::resume): Deleted.

  • animation/DeclarativeAnimation.h:
  • animation/WebAnimation.cpp:

(WebCore::WebAnimation::WebAnimation):
(WebCore::WebAnimation::enqueueAnimationPlaybackEvent):
(WebCore::WebAnimation::suspend):
(WebCore::WebAnimation::resume):
(WebCore::WebAnimation::stop):
(WebCore::WebAnimation::hasPendingActivity):
(WebCore::WebAnimation::shouldPreventEnteringBackForwardCache_DEPRECATED const): Deleted.

  • animation/WebAnimation.h:

LayoutTests:

Add a new test that checks that an Animation that would run past a page's navigation is correctly suspended
and resumed as it enters and leaves the back/forward cache.

  • webanimations/animation-page-cache-expected.txt: Added.
  • webanimations/animation-page-cache.html: Added.
Location:
trunk
Files:
2 added
9 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r251741 r251742  
     12019-10-29  Antoine Quint  <graouts@apple.com>
     2
     3        WebAnimation should never prevent entering the back/forward cache
     4        https://bugs.webkit.org/show_bug.cgi?id=203088
     5        <rdar://problem/56374249>
     6
     7        Reviewed by Antti Koivisto.
     8
     9        Add a new test that checks that an Animation that would run past a page's navigation is correctly suspended
     10        and resumed as it enters and leaves the back/forward cache.
     11
     12        * webanimations/animation-page-cache-expected.txt: Added.
     13        * webanimations/animation-page-cache.html: Added.
     14
    1152019-10-29  Megan Gardner  <megan_gardner@apple.com>
    216

  • trunk/LayoutTests/webanimations/leak-document-with-web-animation-expected.txt

    r233583 r251742  
    55
    66The iframe has finished loading.
    7 The iframe has been destroyed.
    8 PASS internals.numberOfLiveDocuments() is numberOfLiveDocumentsAfterIframeLoaded - 1
    9 
     7PASS The document was destroyed
    108PASS successfullyParsed is true
    119

  • trunk/LayoutTests/webanimations/leak-document-with-web-animation.html

    r233583 r251742  
    22<html>
    33<body onload="runTest()">
    4 <script src="../resources/js-test-pre.js"></script>
     4<script src="../resources/js-test.js"></script>
    55<script>
    66description("This test asserts that Document doesn't leak when a Web Animation is created.");
    … …  
    88if (window.internals)
    99    jsTestIsAsync = true;
    10 
    11 gc();
    12 
    13 var numberOfLiveDocumentsAfterIframeLoaded = 0;
    1410
    1511function runTest() {
    … …  
    2319            return true;
    2420
    25         numberOfLiveDocumentsAfterIframeLoaded = internals.numberOfLiveDocuments();
     21        documentIdentifier = internals.documentIdentifier(frame.contentDocument);
    2622        debug("The iframe has finished loading.");
    2723
    … …  
    2925        frame = null;
    3026
    31         setTimeout(() => {
     27        gc();
     28        timeout = 0;
     29        handle = setInterval(() => {
     30            if (!internals.isDocumentAlive(documentIdentifier)) {
     31                clearInterval(handle);
     32                testPassed("The document was destroyed");
     33                finishJSTest();
     34                return;
     35            }
     36            timeout++;
     37            if (timeout == 500) {
     38                clearInterval(handle);
     39                testFailed("The document was leaked");
     40                finishJSTest();
     41                return;
     42            }
    3243            gc();
    33             setTimeout(function () {
    34                 debug("The iframe has been destroyed.");
    35                 shouldBe("internals.numberOfLiveDocuments()", "numberOfLiveDocumentsAfterIframeLoaded - 1");
    36                 debug("");
    37                 finishJSTest();
    38             });
    39         });
     44        }, 10);
    4045    }
    4146
    … …  
    4449
    4550</script>
    46 <script src="../resources/js-test-post.js"></script>
    4751</body>
    4852</html>

  • trunk/Source/WebCore/ChangeLog

    r251737 r251742  
     12019-10-29  Antoine Quint  <graouts@apple.com>
     2
     3        WebAnimation should never prevent entering the back/forward cache
     4        https://bugs.webkit.org/show_bug.cgi?id=203088
     5        <rdar://problem/56374249>
     6
     7        Reviewed by Antti Koivisto.
     8
     9        Test: webanimations/animation-page-cache.html
     10
     11        We remove the Web Animation override of the deprecated method ActiveDOMObject::shouldPreventEnteringBackForwardCache_DEPRECATED()
     12        and instead ensure event dispatch is suspended along with the WebAnimation object through the adoption of a SuspendableTaskQueue.
     13
     14        We also ensure an animation correctly suspends itself when ActiveDOMObject::suspend() and ActiveDOMObject::resume() are called.
     15        Implementing these methods showed that we have some placeholders in DeclarativeAnimation that were not necessary, so we remove those.
     16
     17        Finally, we no longer need to track the stopped state since the SuspendableTaskQueue will close itself when ActiveDOMObject::stop()
     18        is called.
     19
     20        * animation/DeclarativeAnimation.cpp:
     21        (WebCore::DeclarativeAnimation::stop): Deleted.
     22        (WebCore::DeclarativeAnimation::suspend): Deleted.
     23        (WebCore::DeclarativeAnimation::resume): Deleted.
     24        * animation/DeclarativeAnimation.h:
     25        * animation/WebAnimation.cpp:
     26        (WebCore::WebAnimation::WebAnimation):
     27        (WebCore::WebAnimation::enqueueAnimationPlaybackEvent):
     28        (WebCore::WebAnimation::suspend):
     29        (WebCore::WebAnimation::resume):
     30        (WebCore::WebAnimation::stop):
     31        (WebCore::WebAnimation::hasPendingActivity):
     32        (WebCore::WebAnimation::shouldPreventEnteringBackForwardCache_DEPRECATED const): Deleted.
     33        * animation/WebAnimation.h:
     34
    1352019-10-07  Jer Noble  <jer.noble@apple.com>
    236

  • trunk/Source/WebCore/animation/DeclarativeAnimation.cpp

    r250617 r251742  
    347347}
    348348
    349 void DeclarativeAnimation::stop()
    350 {
    351     WebAnimation::stop();
    352 }
    353 
    354 void DeclarativeAnimation::suspend(ReasonForSuspension reason)
    355 {
    356     WebAnimation::suspend(reason);
    357 }
    358 
    359 void DeclarativeAnimation::resume()
    360 {
    361     WebAnimation::resume();
    362 }
    363 
    364349} // namespace WebCore

  • trunk/Source/WebCore/animation/DeclarativeAnimation.h

    r250617 r251742  
    8282    void remove() final;
    8383
    84     // ActiveDOMObject.
    85     void suspend(ReasonForSuspension) final;
    86     void resume() final;
    87     void stop() final;
    88 
    8984    bool m_wasPending { false };
    9085    AnimationEffectPhase m_previousPhase { AnimationEffectPhase::Idle };

  • trunk/Source/WebCore/animation/WebAnimation.cpp

    r251244 r251742  
    6868    , m_readyPromise(makeUniqueRef<ReadyPromise>(*this, &WebAnimation::readyPromiseResolve))
    6969    , m_finishedPromise(makeUniqueRef<FinishedPromise>(*this, &WebAnimation::finishedPromiseResolve))
     70    , m_taskQueue(SuspendableTaskQueue::create(document))
    7071{
    7172    m_readyPromise->resolve(*this);
    … …  
    614615    } else {
    615616        // Otherwise, queue a task to dispatch event at animation. The task source for this task is the DOM manipulation task source.
    616         callOnMainThread([this, pendingActivity = makePendingActivity(*this), event = WTFMove(event)]() {
    617             if (!m_isStopped)
    618                 this->dispatchEvent(event);
     617        m_taskQueue->enqueueTask([this, event = WTFMove(event)] {
     618            dispatchEvent(event);
    619619        });
    620620    }
    … …  
    11581158}
    11591159
    1160 // FIXME: This should never prevent entering the back/forward cache.
    1161 bool WebAnimation::shouldPreventEnteringBackForwardCache_DEPRECATED() const
    1162 {
    1163     // Use the base class's implementation of hasPendingActivity() since we wouldn't want the custom implementation
    1164     // in this class designed to keep JS wrappers alive to interfere with the ability for a page using animations
    1165     // to enter the back/forward cache.
    1166     return ActiveDOMObject::hasPendingActivity();
     1160void WebAnimation::suspend(ReasonForSuspension)
     1161{
     1162    setSuspended(true);
     1163}
     1164
     1165void WebAnimation::resume()
     1166{
     1167    setSuspended(false);
    11671168}
    11681169
    11691170void WebAnimation::stop()
    11701171{
     1172    m_taskQueue->cancelAllTasks();
    11711173    ActiveDOMObject::stop();
    1172     m_isStopped = true;
    11731174    removeAllEventListeners();
    11741175}
    … …  
    11771178{
    11781179    // Keep the JS wrapper alive if the animation is considered relevant or could become relevant again by virtue of having a timeline.
    1179     return m_timeline || m_isRelevant || ActiveDOMObject::hasPendingActivity();
     1180    return m_timeline || m_isRelevant || m_taskQueue->hasPendingTasks() || ActiveDOMObject::hasPendingActivity();
    11801181}
    11811182

  • trunk/Source/WebCore/animation/WebAnimation.h

    r251244 r251742  
    3030#include "ExceptionOr.h"
    3131#include "IDLTypes.h"
     32#include "SuspendableTaskQueue.h"
    3233#include "WebAnimationUtilities.h"
    3334#include <wtf/Markable.h>
    … …  
    140141protected:
    141142    explicit WebAnimation(Document&);
    142 
    143     void stop() override;
    144143
    145144private:
    … …  
    177176    UniqueRef<ReadyPromise> m_readyPromise;
    178177    UniqueRef<FinishedPromise> m_finishedPromise;
     178    UniqueRef<SuspendableTaskQueue> m_taskQueue;
    179179    Markable<Seconds, Seconds::MarkableTraits> m_previousCurrentTime;
    180180    Markable<Seconds, Seconds::MarkableTraits> m_startTime;
    … …  
    186186    int m_suspendCount { 0 };
    187187
    188     bool m_isStopped { false };
    189188    bool m_isSuspended { false };
    190189    bool m_finishNotificationStepsMicrotaskPending;
    … …  
    198197    // ActiveDOMObject.
    199198    const char* activeDOMObjectName() const final;
    200     bool shouldPreventEnteringBackForwardCache_DEPRECATED() const final;
     199    void suspend(ReasonForSuspension) final;
     200    void resume() final;
     201    void stop() final;
    201202
    202203    // EventTarget

  • trunk/Source/WebCore/platform/SuspendableTaskQueue.h

    r251244 r251742  
    5959    bool isClosed() const { return m_isClosed; }
    6060
    61     bool hasPendingTasks() const { return m_pendingTasks.isEmpty(); }
     61    bool hasPendingTasks() const { return !m_pendingTasks.isEmpty(); }
    6262
    6363private:
Note: See TracChangeset for help on using the changeset viewer.