Changeset 229466 in webkit

Timestamp:

Mar 9, 2018 10:21:11 AM (6 years ago)

Author:

jer.noble@apple.com

Message:

webkitfullscreenchange event not fired at the same time as :-webkit-full-screen pseudo selector changes; causes glitchiness
​https://bugs.webkit.org/show_bug.cgi?id=183383

Source/WebCore:

Reviewed by Eric Carlson.

Fire the webkitfullscreenchange event at the same time as the pseudo class selector changes, during the handling
of webkitDidEnterFullScreenForElement. For WebKit2 clients, this is guaranteed to be asynchronous, since the
calling method originates in the UIProcess. For WebKit1 clients (and WKTR and DRT), there's the possibility that
webkitWillEnterFullScreenForElement will be called synchronously from within
Document::requestFullScreenForElement(), so break that synchronousness by starting the
ChromeClient::enterFullScreenForElement(...) process in a async task.

Previously, the firing of the fullscreenchange event was done through a zero-length timer. Use a
GenericTaskQueue instead.

A number of layout tests depend on the behavior that the element will be in fullscreen when the 'playing' event
fires. This was true for DRT (but not WKTR), since its fullscreen implementations were deliberately synchronous, but
won't necessarily be true for all ports. Fix this in a subsequent patch.

• dom/Document.cpp:

(WebCore::Document::requestFullScreenForElement):
(WebCore::Document::webkitExitFullscreen):
(WebCore::Document::webkitWillEnterFullScreenForElement):
(WebCore::Document::webkitDidEnterFullScreenForElement):
(WebCore::Document::webkitDidExitFullScreenForElement):
(WebCore::Document::dispatchFullScreenChangeEvents):

• dom/Document.h:
• html/HTMLMediaElement.cpp:

(WebCore::HTMLMediaElement::setReadyState):
(WebCore::HTMLMediaElement::playInternal):
(WebCore::HTMLMediaElement::mediaPlayerTimeChanged):
(WebCore::HTMLMediaElement::updatePlayState):
(WebCore::HTMLMediaElement::setPlaying):

LayoutTests:

Fix a couple tests that depended on non-standard behavior, and skip other tests to be fixed later.

Reviewed by Eric Carlson.

• media/fullscreen-video-going-into-pip.html:
• media/video-fullscreeen-only-playback.html:
• platform/mac/TestExpectations:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/media/fullscreen-video-going-into-pip.html (modified) (2 diffs)
• LayoutTests/media/video-fullscreeen-only-playback.html (modified) (2 diffs)
• LayoutTests/platform/mac/TestExpectations (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/dom/Document.cpp (modified) (8 diffs)
• Source/WebCore/dom/Document.h (modified) (3 diffs)
• Source/WebCore/html/HTMLMediaElement.cpp (modified) (6 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2018-03-09  Jer Noble  <jer.noble@apple.com>
+
+        webkitfullscreenchange event not fired at the same time as :-webkit-full-screen pseudo selector changes; causes glitchiness
+        https://bugs.webkit.org/show_bug.cgi?id=183383
+
+        Fix a couple tests that depended on non-standard behavior, and skip other tests to be fixed later.
+
+        Reviewed by Eric Carlson.
+
+        * media/fullscreen-video-going-into-pip.html:
+        * media/video-fullscreeen-only-playback.html:
+        * platform/mac/TestExpectations:
+
 2018-03-09  Frederic Wang  <fwang@igalia.com>
 

trunk/LayoutTests/media/fullscreen-video-going-into-pip.html

@@ -23 +23 @@
 
             consoleWrite("Going into Full Screen");
-            video.addEventListener('webkitfullscreenchange', onfullscreenchange);
+            video.addEventListener('webkitpresentationmodechanged', onfullscreenchange);
             runWithKeyDown(function(){ video.webkitRequestFullscreen(); });
         }
@@ -30 +30 @@
         {
             testExpected("document.webkitCurrentFullScreenElement", video);
-            video.removeEventListener('webkitfullscreenchange', onfullscreenchange);
+            video.removeEventListener('webkitpresentationmodechanged', onfullscreenchange);
 
             consoleWrite("Going into Picture-in-Picture from Full Screen");

trunk/LayoutTests/media/video-fullscreeen-only-playback.html

@@ -9 +9 @@
             function fullscreenchange()
             {
-                if (document.webkitIsFullScreen)
-                    beginfullscreen();
-                else
+                if (!document.webkitIsFullScreen)
                     endfullscreen();
             }
 
-            function beginfullscreen()
+            function playing()
             {
                 consoleWrite("<br>** Entered fullscreen");
@@ -53 +51 @@
                 video = document.getElementsByTagName('video')[0];
                 waitForEvent("canplaythrough", canplaythrough);
-                waitForEvent('playing');
+                waitForEvent('playing', playing);
 
-                video.addEventListener('webkitbeginfullscreen', beginfullscreen, true);
                 video.addEventListener('webkitendfullscreen', endfullscreen, true);
                 video.addEventListener('webkitfullscreenchange', fullscreenchange, true);

trunk/LayoutTests/platform/mac/TestExpectations

@@ -1728 +1728 @@
 
 webkit.org/b/173946 [ Debug ] media/modern-media-controls/fullscreen-support/fullscreen-support-press.html [ Pass Failure ]
+
+webkit.org/b/183490 media/modern-media-controls/controls-visibility-support/controls-visibility-support-fullscreen-on-video.html [ Failure ]
+webkit.org/b/183490 media/modern-media-controls/media-controller/media-controller-fade-controls-when-entering-fullscreen.html [ Failure ]
+webkit.org/b/183490 media/modern-media-controls/media-controller/media-controller-fullscreen-change.html [ Failure ]
+webkit.org/b/183490 media/modern-media-controls/media-controller/media-controller-inline-to-fullscreen-to-inline.html [ Failure ]
+webkit.org/b/183490 media/modern-media-controls/start-support/start-support-fullscreen.html [ Failure ]
+webkit.org/b/183490 media/video-playsinline.html [ Failure ]
+webkit.org/b/183490 media/video-webkit-playsinline.html [ Failure ]
+webkit.org/b/183490 media/modern-media-controls/media-controller/media-controller-inline-to-fullscreen-to-pip-to-inline.html [ Failure ]

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2018-03-09  Jer Noble  <jer.noble@apple.com>
+
+        webkitfullscreenchange event not fired at the same time as :-webkit-full-screen pseudo selector changes; causes glitchiness
+        https://bugs.webkit.org/show_bug.cgi?id=183383
+
+        Reviewed by Eric Carlson.
+
+        Fire the webkitfullscreenchange event at the same time as the pseudo class selector changes, during the handling
+        of webkitDidEnterFullScreenForElement. For WebKit2 clients, this is guaranteed to be asynchronous, since the
+        calling method originates in the UIProcess. For WebKit1 clients (and WKTR and DRT), there's the possibility that
+        webkitWillEnterFullScreenForElement will be called synchronously from within
+        Document::requestFullScreenForElement(), so break that synchronousness by starting the
+        ChromeClient::enterFullScreenForElement(...) process in a async task.
+
+        Previously, the firing of the fullscreenchange event was done through a zero-length timer. Use a
+        GenericTaskQueue instead.
+
+        A number of layout tests depend on the behavior that the element will be in fullscreen when the 'playing' event
+        fires. This was true for DRT (but not WKTR), since its fullscreen implementations were deliberately synchronous, but
+        won't necessarily be true for all ports. Fix this in a subsequent patch.
+
+        * dom/Document.cpp:
+        (WebCore::Document::requestFullScreenForElement):
+        (WebCore::Document::webkitExitFullscreen):
+        (WebCore::Document::webkitWillEnterFullScreenForElement):
+        (WebCore::Document::webkitDidEnterFullScreenForElement):
+        (WebCore::Document::webkitDidExitFullScreenForElement):
+        (WebCore::Document::dispatchFullScreenChangeEvents):
+        * dom/Document.h:
+        * html/HTMLMediaElement.cpp:
+        (WebCore::HTMLMediaElement::setReadyState):
+        (WebCore::HTMLMediaElement::playInternal):
+        (WebCore::HTMLMediaElement::mediaPlayerTimeChanged):
+        (WebCore::HTMLMediaElement::updatePlayState):
+        (WebCore::HTMLMediaElement::setPlaying):
+
 2018-03-09  Zan Dobersek  <zdobersek@igalia.com>
 

trunk/Source/WebCore/dom/Document.cpp

@@ -496 +496 @@
     , m_documentClasses(documentClasses)
     , m_eventQueue(*this)
-#if ENABLE(FULLSCREEN_API)
-    , m_fullScreenChangeDelayTimer(*this, &Document::fullScreenChangeDelayTimerFired)
-#endif
     , m_loadEventDelayTimer(*this, &Document::loadEventDelayTimerFired)
 #if PLATFORM(IOS)
@@ -6090 +6087 @@
         // 6. Optionally, perform some animation.
         m_areKeysEnabledInFullScreen = hasKeyboardAccess;
-        page()->chrome().client().enterFullScreenForElement(*element);
+        m_fullScreenTaskQueue.enqueueTask([this, element = makeRefPtr(element)] {
+            if (auto page = this->page())
+                page->chrome().client().enterFullScreenForElement(*element);
+        });
 
         // 7. Optionally, display a message indicating how the user can exit displaying the context object fullscreen.
@@ -6097 +6097 @@
 
     m_fullScreenErrorEventTargetQueue.append(element ? element : documentElement());
-    m_fullScreenChangeDelayTimer.startOneShot(0_s);
+    m_fullScreenTaskQueue.enqueueTask([this] {
+        dispatchFullScreenChangeEvents();
+    });
 }
 
@@ -6175 +6177 @@
     // 6. Return, and run the remaining steps asynchronously.
     // 7. Optionally, perform some animation.
-
-    if (!page())
-        return;
-
-    // Only exit out of full screen window mode if there are no remaining elements in the 
-    // full screen stack.
-    if (!newTop) {
-        page()->chrome().client().exitFullScreenForElement(m_fullScreenElement.get());
-        return;
-    }
-
-    // Otherwise, notify the chrome of the new full screen element.
-    page()->chrome().client().enterFullScreenForElement(*newTop);
+    m_fullScreenTaskQueue.enqueueTask([this, newTop = makeRefPtr(newTop), fullScreenElement = m_fullScreenElement] {
+        auto* page = this->page();
+        if (!page)
+            return;
+
+        // Only exit out of full screen window mode if there are no remaining elements in the 
+        // full screen stack.
+        if (!newTop) {
+            page->chrome().client().exitFullScreenForElement(fullScreenElement.get());
+            return;
+        }
+
+        // Otherwise, notify the chrome of the new full screen element.
+        page->chrome().client().enterFullScreenForElement(*newTop);
+    });
 }
 
@@ -6252 +6256 @@
 
     resolveStyle(ResolveStyleType::Rebuild);
-#if PLATFORM(IOS) && ENABLE(FULLSCREEN_API)
-    m_fullScreenChangeDelayTimer.startOneShot(0_s);
-#endif
+    dispatchFullScreenChangeEvents();
 }
 
@@ -6266 +6268 @@
 
     m_fullScreenElement->didBecomeFullscreenElement();
-
-#if !PLATFORM(IOS) || !ENABLE(FULLSCREEN_API)
-    m_fullScreenChangeDelayTimer.startOneShot(0_s);
-#endif
 }
 
@@ -6306 +6304 @@
     Document& exitingDocument = eventTargetQueuesEmpty ? topDocument() : *this;
 
-    exitingDocument.m_fullScreenChangeDelayTimer.startOneShot(0_s);
+    exitingDocument.dispatchFullScreenChangeEvents();
 }
 
@@ -6328 +6326 @@
 }
 
-void Document::fullScreenChangeDelayTimerFired()
+void Document::dispatchFullScreenChangeEvents()
 {
     // Since we dispatch events in this function, it's possible that the

trunk/Source/WebCore/dom/Document.h

@@ -36 +36 @@
 #include "FontSelectorClient.h"
 #include "FrameDestructionObserver.h"
+#include "GenericTaskQueue.h"
 #include "MediaProducer.h"
 #include "MutationObserver.h"
@@ -1137 +1138 @@
     RenderFullScreen* fullScreenRenderer() const { return m_fullScreenRenderer.get(); }
 
-    void fullScreenChangeDelayTimerFired();
+    void dispatchFullScreenChangeEvents();
     bool fullScreenIsAllowedForElement(Element*) const;
     void fullScreenElementRemoved();
@@ -1684 +1685 @@
     Vector<RefPtr<Element>> m_fullScreenElementStack;
     WeakPtr<RenderFullScreen> m_fullScreenRenderer { nullptr };
-    Timer m_fullScreenChangeDelayTimer;
+    GenericTaskQueue<Timer> m_fullScreenTaskQueue;
     Deque<RefPtr<Node>> m_fullScreenChangeEventTargetQueue;
     Deque<RefPtr<Node>> m_fullScreenErrorEventTargetQueue;

trunk/Source/WebCore/html/HTMLMediaElement.cpp

@@ -2516 +2516 @@
     }
 
-    bool isPotentiallyPlaying = potentiallyPlaying();
     if (m_readyState == HAVE_FUTURE_DATA && oldState <= HAVE_CURRENT_DATA && tracksAreReady) {
         scheduleEvent(eventNames().canplayEvent);
-        if (isPotentiallyPlaying)
-            scheduleNotifyAboutPlaying();
         shouldUpdateDisplayState = true;
     }
@@ -2529 +2526 @@
 
         scheduleEvent(eventNames().canplaythroughEvent);
-
-        if (isPotentiallyPlaying && oldState <= HAVE_CURRENT_DATA)
-            scheduleNotifyAboutPlaying();
 
         auto success = canTransitionFromAutoplayToPlay();
@@ -2540 +2534 @@
             m_playbackStartedTime = currentMediaTime().toDouble();
             scheduleEvent(eventNames().playEvent);
-            scheduleNotifyAboutPlaying();
         } else if (success.value() == MediaPlaybackDenialReason::UserGestureRequired) {
             ALWAYS_LOG(LOGIDENTIFIER, "Autoplay blocked, user gesture required");
@@ -3498 +3491 @@
         if (m_readyState <= HAVE_CURRENT_DATA)
             scheduleEvent(eventNames().waitingEvent);
-        else if (m_readyState >= HAVE_FUTURE_DATA)
-            scheduleNotifyAboutPlaying();
     } else if (m_readyState >= HAVE_FUTURE_DATA)
         scheduleResolvePendingPlayPromises();
@@ -4714 +4705 @@
                 if (!wasSeeking)
                     addBehaviorRestrictionsOnEndIfNecessary();
-
                 setPlaybackWithoutUserGesture(PlaybackWithoutUserGesture::None);
             }
+            setPlaying(false);
             // If the media element has a current media controller, then report the controller state
             // for the media element's current media controller.
@@ -5329 +5320 @@
 
     m_playing = playing;
+
+    if (m_playing)
+        scheduleNotifyAboutPlaying();
 
 #if ENABLE(MEDIA_SESSION)