[mac-wk1] LayoutTest media/modern-media-controls/tracks-support/tracks-support-click...
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 1 Feb 2017 19:54:25 +0000 (19:54 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 1 Feb 2017 19:54:25 +0000 (19:54 +0000)
https://bugs.webkit.org/show_bug.cgi?id=165319
<rdar://problem/30284104>

Patch by Antoine Quint <graouts@apple.com> on 2017-02-01
Reviewed by Dean Jackson.

Source/WebCore:

Running media/controls/track-menu.html before media/modern-media-controls/tracks-support/tracks-
support-click-track-in-panel.html makes that test time out in all test runs. The root of the issue
is that animations are suspended by media/controls/track-menu.html with a call to
internals.suspendAnimations(), and that state isn't reset with a call to internals.resumeAnimations().
Then, media/modern-media-controls/tracks-support/tracks-support-click-track-in-panel.html fails because
the selection animation for the tracks panel menu item that is clicked never completes and the delegate
to notify that an item in the tracks panel was selected is never fired, which leads to the test failure.

We change Internals::suspendAnimations() and Internals::resumeAnimations() to only affect the current
document, rather than calling into AnimationController::suspendAnimations() which would do just that,
but also set a Frame-wide flag that would prevent further animations from running, even in a subsequent
document load.

* dom/Document.cpp:
(WebCore::Document::prepareForDestruction): Ensure the document that is about to be destroyed is no longer
associated with an AnimationController.
* page/animation/AnimationController.cpp:
(WebCore::AnimationControllerPrivate::ensureCompositeAnimation): Update the animation's suspend state in case
the document its renderer is associated with is suspended. This is required since previously CompositeAnimations
would set their suspend state in their constructor, based on the Frame-wide suspended state, but there is no
document to use as a basis to query its suspended state in that constructor.
(WebCore::AnimationControllerPrivate::animationsAreSuspendedForDocument):
(WebCore::AnimationControllerPrivate::detachFromDocument):
(WebCore::AnimationControllerPrivate::suspendAnimationsForDocument):
(WebCore::AnimationControllerPrivate::resumeAnimationsForDocument):
(WebCore::AnimationControllerPrivate::startAnimationsIfNotSuspended):
(WebCore::AnimationController::animationsAreSuspendedForDocument):
(WebCore::AnimationController::detachFromDocument):
* page/animation/AnimationController.h:
* page/animation/AnimationControllerPrivate.h:
* testing/Internals.cpp:
(WebCore::Internals::animationsAreSuspended):
(WebCore::Internals::suspendAnimations):
(WebCore::Internals::resumeAnimations):

LayoutTests:

Since we've fixed the root cause of this test's flakiness, we no longer need to mark it as flaky.

* platform/mac/TestExpectations:

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

LayoutTests/ChangeLog
LayoutTests/platform/mac/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/dom/Document.cpp
Source/WebCore/page/animation/AnimationController.cpp
Source/WebCore/page/animation/AnimationController.h
Source/WebCore/page/animation/AnimationControllerPrivate.h
Source/WebCore/testing/Internals.cpp

index 14b471e..b7463e0 100644 (file)
@@ -1,3 +1,15 @@
+2017-02-01  Antoine Quint  <graouts@apple.com>
+
+        [mac-wk1] LayoutTest media/modern-media-controls/tracks-support/tracks-support-click-track-in-panel.html is a flaky timeout
+        https://bugs.webkit.org/show_bug.cgi?id=165319
+        <rdar://problem/30284104>
+
+        Reviewed by Dean Jackson.
+
+        Since we've fixed the root cause of this test's flakiness, we no longer need to mark it as flaky.
+
+        * platform/mac/TestExpectations:
+
 2017-02-01  Jer Noble  <jer.noble@apple.com>
 
         NULL-deref crash in TextTrack::removeCue()
index fcd5fc1..804a4c7 100644 (file)
@@ -1484,8 +1484,6 @@ webkit.org/b/167396 media/modern-media-controls/media-controller/media-controlle
 
 webkit.org/b/167442 [ ElCapitan ] media/modern-media-controls/airplay-support/airplay-support.html [ Pass Timeout ]
 
-webkit.org/b/165319 media/modern-media-controls/tracks-support/tracks-support-click-track-in-panel.html [ Pass Timeout ]
-
 webkit.org/b/167461 media/modern-media-controls/layout-node/addChild.html [ Pass Timeout ]
 
 webkit.org/b/165874 [ Debug ] streams/pipe-to.html [ Pass Failure ]
index 22b214e..9fcd807 100644 (file)
@@ -1,3 +1,46 @@
+2017-02-01  Antoine Quint  <graouts@apple.com>
+
+        [mac-wk1] LayoutTest media/modern-media-controls/tracks-support/tracks-support-click-track-in-panel.html is a flaky timeout
+        https://bugs.webkit.org/show_bug.cgi?id=165319
+        <rdar://problem/30284104>
+
+        Reviewed by Dean Jackson.
+
+        Running media/controls/track-menu.html before media/modern-media-controls/tracks-support/tracks-
+        support-click-track-in-panel.html makes that test time out in all test runs. The root of the issue
+        is that animations are suspended by media/controls/track-menu.html with a call to
+        internals.suspendAnimations(), and that state isn't reset with a call to internals.resumeAnimations().
+        Then, media/modern-media-controls/tracks-support/tracks-support-click-track-in-panel.html fails because
+        the selection animation for the tracks panel menu item that is clicked never completes and the delegate
+        to notify that an item in the tracks panel was selected is never fired, which leads to the test failure.
+
+        We change Internals::suspendAnimations() and Internals::resumeAnimations() to only affect the current
+        document, rather than calling into AnimationController::suspendAnimations() which would do just that,
+        but also set a Frame-wide flag that would prevent further animations from running, even in a subsequent
+        document load.
+
+        * dom/Document.cpp:
+        (WebCore::Document::prepareForDestruction): Ensure the document that is about to be destroyed is no longer
+        associated with an AnimationController.
+        * page/animation/AnimationController.cpp:
+        (WebCore::AnimationControllerPrivate::ensureCompositeAnimation): Update the animation's suspend state in case
+        the document its renderer is associated with is suspended. This is required since previously CompositeAnimations
+        would set their suspend state in their constructor, based on the Frame-wide suspended state, but there is no
+        document to use as a basis to query its suspended state in that constructor.
+        (WebCore::AnimationControllerPrivate::animationsAreSuspendedForDocument):
+        (WebCore::AnimationControllerPrivate::detachFromDocument):
+        (WebCore::AnimationControllerPrivate::suspendAnimationsForDocument):
+        (WebCore::AnimationControllerPrivate::resumeAnimationsForDocument):
+        (WebCore::AnimationControllerPrivate::startAnimationsIfNotSuspended):
+        (WebCore::AnimationController::animationsAreSuspendedForDocument):
+        (WebCore::AnimationController::detachFromDocument):
+        * page/animation/AnimationController.h:
+        * page/animation/AnimationControllerPrivate.h:
+        * testing/Internals.cpp:
+        (WebCore::Internals::animationsAreSuspended):
+        (WebCore::Internals::suspendAnimations):
+        (WebCore::Internals::resumeAnimations):
+
 2017-02-01  Ryan Haddad  <ryanhaddad@apple.com>
 
         Unreviewed build fix after r211488.
index a69958c..0ae70fe 100644 (file)
@@ -2263,6 +2263,8 @@ void Document::prepareForDestruction()
     if (m_hasPreparedForDestruction)
         return;
 
+    m_frame->animation().detachFromDocument(this);
+
 #if ENABLE(IOS_TOUCH_EVENTS)
     clearTouchEventHandlersAndListeners();
 #endif
index 4c40c92..ca0358b 100644 (file)
@@ -91,6 +91,9 @@ CompositeAnimation& AnimationControllerPrivate::ensureCompositeAnimation(RenderE
         renderer.setIsCSSAnimating(true);
     }
 
+    if (animationsAreSuspendedForDocument(&renderer.document()))
+        result.iterator->value->suspendAnimations();
+
     return *result.iterator->value;
 }
 
@@ -308,8 +311,23 @@ void AnimationControllerPrivate::resumeAnimations()
     m_isSuspended = false;
 }
 
+bool AnimationControllerPrivate::animationsAreSuspendedForDocument(Document* document)
+{
+    return isSuspended() || m_suspendedDocuments.contains(document);
+}
+
+void AnimationControllerPrivate::detachFromDocument(Document* document)
+{
+    m_suspendedDocuments.remove(document);
+}
+
 void AnimationControllerPrivate::suspendAnimationsForDocument(Document* document)
 {
+    if (animationsAreSuspendedForDocument(document))
+        return;
+
+    m_suspendedDocuments.add(document);
+
     AnimationPrivateUpdateBlock updateBlock(*this);
 
     for (auto& animation : m_compositeAnimations) {
@@ -322,6 +340,11 @@ void AnimationControllerPrivate::suspendAnimationsForDocument(Document* document
 
 void AnimationControllerPrivate::resumeAnimationsForDocument(Document* document)
 {
+    if (!animationsAreSuspendedForDocument(document))
+        return;
+
+    detachFromDocument(document);
+
     AnimationPrivateUpdateBlock updateBlock(*this);
 
     for (auto& animation : m_compositeAnimations) {
@@ -334,7 +357,7 @@ void AnimationControllerPrivate::resumeAnimationsForDocument(Document* document)
 
 void AnimationControllerPrivate::startAnimationsIfNotSuspended(Document* document)
 {
-    if (!isSuspended() || allowsNewAnimationsWhileSuspended())
+    if (!animationsAreSuspendedForDocument(document) || allowsNewAnimationsWhileSuspended())
         resumeAnimationsForDocument(document);
 }
 
@@ -701,6 +724,16 @@ void AnimationController::serviceAnimations()
     m_data->animationFrameCallbackFired();
 }
 
+bool AnimationController::animationsAreSuspendedForDocument(Document* document)
+{
+    return m_data->animationsAreSuspendedForDocument(document);
+}
+
+void AnimationController::detachFromDocument(Document* document)
+{
+    return m_data->detachFromDocument(document);
+}
+
 void AnimationController::suspendAnimationsForDocument(Document* document)
 {
     LOG(Animations, "suspending animations for document %p", document);
index 18641bc..42324b9 100644 (file)
@@ -72,8 +72,10 @@ public:
     WEBCORE_EXPORT void resumeAnimations();
     void serviceAnimations();
 
-    void suspendAnimationsForDocument(Document*);
-    void resumeAnimationsForDocument(Document*);
+    WEBCORE_EXPORT void suspendAnimationsForDocument(Document*);
+    WEBCORE_EXPORT void resumeAnimationsForDocument(Document*);
+    WEBCORE_EXPORT bool animationsAreSuspendedForDocument(Document*);
+    void detachFromDocument(Document*);
     void startAnimationsIfNotSuspended(Document*);
 
     void beginAnimationUpdate();
index 49ad3f9..c9b8ffc 100644 (file)
@@ -69,7 +69,9 @@ public:
 
     void suspendAnimationsForDocument(Document*);
     void resumeAnimationsForDocument(Document*);
+    bool animationsAreSuspendedForDocument(Document*);
     void startAnimationsIfNotSuspended(Document*);
+    void detachFromDocument(Document*);
 
     bool isRunningAnimationOnRenderer(RenderElement&, CSSPropertyID, AnimationBase::RunningState) const;
     bool isRunningAcceleratedAnimationOnRenderer(RenderElement&, CSSPropertyID, AnimationBase::RunningState) const;
@@ -131,7 +133,8 @@ private:
     };
     Vector<EventToDispatch> m_eventsToDispatch;
     Vector<Ref<Element>> m_elementChangesToDispatch;
-    
+    HashSet<Document*> m_suspendedDocuments;
+
     double m_beginAnimationUpdateTime;
 
     using AnimationsSet = HashSet<RefPtr<AnimationBase>>;
index 2129dce..7e2138b 100644 (file)
@@ -739,7 +739,7 @@ ExceptionOr<bool> Internals::animationsAreSuspended() const
     if (!document || !document->frame())
         return Exception { INVALID_ACCESS_ERR };
 
-    return document->frame()->animation().isSuspended();
+    return document->frame()->animation().animationsAreSuspendedForDocument(document);
 }
 
 ExceptionOr<void> Internals::suspendAnimations() const
@@ -748,7 +748,13 @@ ExceptionOr<void> Internals::suspendAnimations() const
     if (!document || !document->frame())
         return Exception { INVALID_ACCESS_ERR };
 
-    document->frame()->animation().suspendAnimations();
+    document->frame()->animation().suspendAnimationsForDocument(document);
+
+    for (Frame* frame = document->frame(); frame; frame = frame->tree().traverseNext()) {
+        if (Document* document = frame->document())
+            frame->animation().suspendAnimationsForDocument(document);
+    }
+
     return { };
 }
 
@@ -758,7 +764,13 @@ ExceptionOr<void> Internals::resumeAnimations() const
     if (!document || !document->frame())
         return Exception { INVALID_ACCESS_ERR };
 
-    document->frame()->animation().resumeAnimations();
+    document->frame()->animation().resumeAnimationsForDocument(document);
+
+    for (Frame* frame = document->frame(); frame; frame = frame->tree().traverseNext()) {
+        if (Document* document = frame->document())
+            frame->animation().resumeAnimationsForDocument(document);
+    }
+
     return { };
 }