Changeset 211501 in webkit

Timestamp:

Feb 1, 2017 11:54:25 AM (7 years ago)

Author:

commit-queue@webkit.org

Message:

[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>

Patch by Antoine Quint <Antoine Quint> 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:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/platform/mac/TestExpectations (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/dom/Document.cpp (modified) (1 diff)
• Source/WebCore/page/animation/AnimationController.cpp (modified) (5 diffs)
• Source/WebCore/page/animation/AnimationController.h (modified) (1 diff)
• Source/WebCore/page/animation/AnimationControllerPrivate.h (modified) (2 diffs)
• Source/WebCore/testing/Internals.cpp (modified) (3 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>
 

trunk/LayoutTests/platform/mac/TestExpectations

@@ -1485 +1485 @@
 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 ]
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/dom/Document.cpp

@@ -2264 +2264 @@
         return;
 
+    m_frame->animation().detachFromDocument(this);
+
 #if ENABLE(IOS_TOUCH_EVENTS)
     clearTouchEventHandlersAndListeners();

trunk/Source/WebCore/page/animation/AnimationController.cpp

@@ -92 +92 @@
     }
 
+    if (animationsAreSuspendedForDocument(&renderer.document()))
+        result.iterator->value->suspendAnimations();
+
     return *result.iterator->value;
 }
@@ -309 +312 @@
 }
 
+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);
 
@@ -323 +341 @@
 void AnimationControllerPrivate::resumeAnimationsForDocument(Document* document)
 {
+    if (!animationsAreSuspendedForDocument(document))
+        return;
+
+    detachFromDocument(document);
+
     AnimationPrivateUpdateBlock updateBlock(*this);
 
@@ -335 +358 @@
 void AnimationControllerPrivate::startAnimationsIfNotSuspended(Document* document)
 {
-    if (!isSuspended() || allowsNewAnimationsWhileSuspended())
+    if (!animationsAreSuspendedForDocument(document) || allowsNewAnimationsWhileSuspended())
         resumeAnimationsForDocument(document);
 }
@@ -702 +725 @@
 }
 
+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)
 {

trunk/Source/WebCore/page/animation/AnimationController.h

@@ -73 +73 @@
     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*);
 

trunk/Source/WebCore/page/animation/AnimationControllerPrivate.h

@@ -70 +70 @@
     void suspendAnimationsForDocument(Document*);
     void resumeAnimationsForDocument(Document*);
+    bool animationsAreSuspendedForDocument(Document*);
     void startAnimationsIfNotSuspended(Document*);
+    void detachFromDocument(Document*);
 
     bool isRunningAnimationOnRenderer(RenderElement&, CSSPropertyID, AnimationBase::RunningState) const;
@@ -132 +134 @@
     Vector<EventToDispatch> m_eventsToDispatch;
     Vector<Ref<Element>> m_elementChangesToDispatch;
-    
+    HashSet<Document*> m_suspendedDocuments;
+
     double m_beginAnimationUpdateTime;
 

trunk/Source/WebCore/testing/Internals.cpp

@@ -740 +740 @@
         return Exception { INVALID_ACCESS_ERR };
 
-    return document->frame()->animation().isSuspended();
+    return document->frame()->animation().animationsAreSuspendedForDocument(document);
 }
 
@@ -749 +749 @@
         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 { };
 }
@@ -759 +765 @@
         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 { };
 }