Attachment 257065 Details for Bug 147020 – Proposed patch

[patch] Proposed patch

bug-147020.diff (text/plain), 12.76 KB, created by Andreas Kling on 2015-07-19 13:33:25 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Andreas Kling

Created: 2015-07-19 13:33:25 PDT

Size: 12.76 KB

patch

obsolete

>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 5669f17..b90545e 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,16 @@
>+2015-07-19  Andreas Kling  <akling@apple.com>
>+
>+        Improve behavior of media elements in page cache.
>+        <https://webkit.org/b/147020>
>+        <rdar://problem/21712311>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add some coverage for suspend/resume of queued events on cached media elements.
>+
>+        * media/restore-from-page-cache-expected.txt:
>+        * media/restore-from-page-cache.html:
>+
> 2015-07-16  David Kilzer  <ddkilzer@apple.com>
> 
>         Mark fast/canvas/canvas-too-large-to-draw.html as crashing on El Capitan
>diff --git a/LayoutTests/media/restore-from-page-cache-expected.txt b/LayoutTests/media/restore-from-page-cache-expected.txt
>index e04b1ed..3c616e1 100644
>--- a/LayoutTests/media/restore-from-page-cache-expected.txt
>+++ b/LayoutTests/media/restore-from-page-cache-expected.txt
>@@ -5,8 +5,12 @@ EVENT(loadstart)
> EVENT(canplaythrough)
> EXPECTED (loadCount == '0') OK
> *** Page going into cache
>+*** Changing playbackRate just before going into cache, to schedule a ratechange event.
>+*** Changing volume just before going into cache, to schedule a volumechange event.
> *** Page returned from cache
> EXPECTED (loadCount == '1') OK
> EXPECTED (videoComputedStyle.width == '320px') OK
> EXPECTED (videoComputedStyle.height == '240px') OK
>+*** ratechange event fired. This should happen AFTER returning from cache.
>+*** volumechange event fired. This should happen AFTER returning from cache.
> 
>diff --git a/LayoutTests/media/restore-from-page-cache.html b/LayoutTests/media/restore-from-page-cache.html
>index 4833622..5c1454c 100644
>--- a/LayoutTests/media/restore-from-page-cache.html
>+++ b/LayoutTests/media/restore-from-page-cache.html
>@@ -33,17 +33,33 @@
>                     setTimeout(function() { if (window.testRunner) testRunner.notifyDone(); }, 200);
>                 }
>             }
>-                
>+
>+            function pagehide()
>+            {
>+                // Have the video element generate some events that need to be suspended.
>+                // We use multiple events to verify that they fire in the correct order.
>+                consoleWrite("*** Changing playbackRate just before going into cache, to schedule a ratechange event.");
>+                document.getElementsByTagName("video")[0].playbackRate = 2; 
>+                consoleWrite("*** Changing volume just before going into cache, to schedule a volumechange event.");
>+                document.getElementsByTagName("video")[0].volume = 0.5; 
>+            }
>+
>             function canplaythrough() 
>             {
>                 testExpected("loadCount", 0);
>                 if (!loadCount) {
>+                    video = document.getElementsByTagName('video')[0];
>                     consoleWrite("*** Page going into cache");
>                     setTimeout('window.location = "data:text/html,<script>history.back()<" + "/script>"', 0);
>                 }
>                 ++loadCount;
>             }
> 
>+            function propertyChangeCallback(evt)
>+            {
>+                consoleWrite("*** " + evt.type +  " event fired. This should happen AFTER returning from cache.");
>+            }
>+
>             function setup()
>             {
>                 video = mediaElement = document.getElementsByTagName('video')[0];
>@@ -52,9 +68,13 @@
>                 waitForEvent('canplaythrough', canplaythrough);
>                 
>                 video.src = mediaFile;
>+
>+                video.onratechange = propertyChangeCallback;
>+                video.onvolumechange = propertyChangeCallback;
>             }
> 
>             window.onpageshow = pageshow;
>+            window.onpagehide = pagehide;
>         </script>
>     </head>
>     <body>
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 2b2963b..00ec296 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,53 @@
>+2015-07-19  Andreas Kling  <akling@apple.com>
>+
>+        Improve behavior of media elements in page cache.
>+        <https://webkit.org/b/147020>
>+        <rdar://problem/21712311>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Make improvements for media elements when transitioning in/out of page cache:
>+
>+        - Events that were scheduled when going into cache will now be delivered
>+          when the page is restored from cache.
>+
>+        - Data buffering is turned off while in the cache. This reduces the memory
>+          cost of cached pages with media elements on iOS (where mediaserverd would
>+          keep upcoming video frames in memory for cached pages.)
>+
>+        Test: media/restore-from-page-cache.html (amended)
>+
>+        * dom/GenericEventQueue.h:
>+        * dom/GenericEventQueue.cpp:
>+        (WebCore::GenericEventQueue::enqueueEvent):
>+        (WebCore::GenericEventQueue::suspend):
>+        (WebCore::GenericEventQueue::resume):
>+
>+            Add a simple suspend/resume mechanism to GenericEventQueue that can
>+            be used to support page caching.
>+
>+        * html/HTMLMediaElement.cpp:
>+        (WebCore::HTMLMediaElement::stop):
>+        (WebCore::HTMLMediaElement::suspend):
>+        (WebCore::HTMLMediaElement::resume):
>+        (WebCore::HTMLMediaElement::stopWithoutDestroyingMediaPlayer):
>+
>+            Adapt to event queueing changes and add calls to setShouldBufferData().
>+
>+        * html/HTMLSourceElement.h:
>+        * html/HTMLSourceElement.cpp:
>+        (WebCore::HTMLSourceElement::HTMLSourceElement):
>+        (WebCore::HTMLSourceElement::create):
>+        (WebCore::HTMLSourceElement::activeDOMObjectName):
>+        (WebCore::HTMLSourceElement::canSuspendForPageCache):
>+        (WebCore::HTMLSourceElement::suspend):
>+        (WebCore::HTMLSourceElement::resume):
>+        (WebCore::HTMLSourceElement::stop):
>+
>+            Turn HTMLSourceElement into an ActiveDOMObject so it gets all the
>+            appropriate page cache notifications directly. Suspend the delayed
>+            error event delivery timer when cached.
>+
> 2015-07-16  Brady Eidson  <beidson@apple.com>
> 
>         WebKit document.cookie mis-parsing.
>diff --git a/Source/WebCore/dom/GenericEventQueue.cpp b/Source/WebCore/dom/GenericEventQueue.cpp
>index 10c034c..fa397d2 100644
>--- a/Source/WebCore/dom/GenericEventQueue.cpp
>+++ b/Source/WebCore/dom/GenericEventQueue.cpp
>@@ -45,20 +45,22 @@ GenericEventQueue::~GenericEventQueue()
> {
> }
> 
>-bool GenericEventQueue::enqueueEvent(PassRefPtr<Event> event)
>+void GenericEventQueue::enqueueEvent(PassRefPtr<Event> event)
> {
>     if (m_isClosed)
>-        return false;
>+        return;
> 
>     if (event->target() == &m_owner)
>         event->setTarget(0);
> 
>     m_pendingEvents.append(event);
>+
>+    if (m_isSuspended)
>+        return;
>+
>     pendingQueues().append(m_weakPtrFactory.createWeakPtr());
>     if (!sharedTimer().isActive())
>         sharedTimer().startOneShot(0);
>-
>-    return true;
> }
> 
> Timer& GenericEventQueue::sharedTimer()
>@@ -120,4 +122,26 @@ bool GenericEventQueue::hasPendingEvents() const
>     return !m_pendingEvents.isEmpty();
> }
> 
>+void GenericEventQueue::suspend()
>+{
>+    ASSERT(!m_isSuspended);
>+    m_isSuspended = true;
>+    m_weakPtrFactory.revokeAll();
>+}
>+
>+void GenericEventQueue::resume()
>+{
>+    ASSERT(m_isSuspended);
>+    m_isSuspended = false;
>+
>+    if (m_pendingEvents.isEmpty())
>+        return;
>+
>+    for (unsigned i = 0; i < m_pendingEvents.size(); ++i)
>+        pendingQueues().append(m_weakPtrFactory.createWeakPtr());
>+
>+    if (!sharedTimer().isActive())
>+        sharedTimer().startOneShot(0);
>+}
>+
> }
>diff --git a/Source/WebCore/dom/GenericEventQueue.h b/Source/WebCore/dom/GenericEventQueue.h
>index 6ba86e2..2b5e48a 100644
>--- a/Source/WebCore/dom/GenericEventQueue.h
>+++ b/Source/WebCore/dom/GenericEventQueue.h
>@@ -42,12 +42,15 @@ public:
>     explicit GenericEventQueue(EventTarget&);
>     ~GenericEventQueue();
> 
>-    bool enqueueEvent(PassRefPtr<Event>);
>+    void enqueueEvent(PassRefPtr<Event>);
>     void close();
> 
>     void cancelAllEvents();
>     bool hasPendingEvents() const;
> 
>+    void suspend();
>+    void resume();
>+
> private:
>     static Timer& sharedTimer();
>     static void sharedTimerFired();
>@@ -59,6 +62,7 @@ private:
>     Deque<RefPtr<Event>> m_pendingEvents;
>     WeakPtrFactory<GenericEventQueue> m_weakPtrFactory;
>     bool m_isClosed;
>+    bool m_isSuspended { false };
> };
> 
> }
>diff --git a/Source/WebCore/html/HTMLMediaElement.cpp b/Source/WebCore/html/HTMLMediaElement.cpp
>index c72d4f7..aac8bb6 100644
>--- a/Source/WebCore/html/HTMLMediaElement.cpp
>+++ b/Source/WebCore/html/HTMLMediaElement.cpp
>@@ -4890,9 +4890,6 @@ void HTMLMediaElement::stopWithoutDestroyingMediaPlayer()
>         renderer()->updateFromElement();
>     
>     stopPeriodicTimers();
>-    cancelPendingEventsAndCallbacks();
>-
>-    m_asyncEventQueue.close();
> 
>     updateSleepDisabling();
> }
>@@ -4903,6 +4900,8 @@ void HTMLMediaElement::stop()
> 
>     stopWithoutDestroyingMediaPlayer();
> 
>+    m_asyncEventQueue.close();
>+
>     // Once an active DOM object has been stopped it can not be restarted, so we can deallocate
>     // the media player now. Note that userCancelledLoad will already called clearMediaPlayer
>     // if the media was not fully loaded, but we need the same cleanup if the file was completely
>@@ -4918,6 +4917,8 @@ void HTMLMediaElement::suspend(ReasonForSuspension why)
>     {
>         case PageCache:
>             stopWithoutDestroyingMediaPlayer();
>+            m_asyncEventQueue.suspend();
>+            setShouldBufferData(false);
>             m_mediaSession->addBehaviorRestriction(MediaElementSession::RequirePageConsentToResumeMedia);
>             break;
>         case DocumentWillBePaused:
>@@ -4935,6 +4936,10 @@ void HTMLMediaElement::resume()
> 
>     m_inActiveDocument = true;
> 
>+    m_asyncEventQueue.resume();
>+
>+    setShouldBufferData(true);
>+
>     if (!m_mediaSession->pageAllowsPlaybackAfterResuming(*this))
>         document().addMediaCanStartListener(this);
>     else
>diff --git a/Source/WebCore/html/HTMLSourceElement.cpp b/Source/WebCore/html/HTMLSourceElement.cpp
>index 301951c..2931ce0 100644
>--- a/Source/WebCore/html/HTMLSourceElement.cpp
>+++ b/Source/WebCore/html/HTMLSourceElement.cpp
>@@ -40,6 +40,7 @@ using namespace HTMLNames;
> 
> inline HTMLSourceElement::HTMLSourceElement(const QualifiedName& tagName, Document& document)
>     : HTMLElement(tagName, document)
>+    , ActiveDOMObject(&document)
>     , m_errorEventTimer(*this, &HTMLSourceElement::errorEventTimerFired)
> {
>     LOG(Media, "HTMLSourceElement::HTMLSourceElement - %p", this);
>@@ -48,7 +49,9 @@ inline HTMLSourceElement::HTMLSourceElement(const QualifiedName& tagName, Docume
> 
> Ref<HTMLSourceElement> HTMLSourceElement::create(const QualifiedName& tagName, Document& document)
> {
>-    return adoptRef(*new HTMLSourceElement(tagName, document));
>+    Ref<HTMLSourceElement> sourceElement = adoptRef(*new HTMLSourceElement(tagName, document));
>+    sourceElement->suspendIfNeeded();
>+    return sourceElement;
> }
> 
> Node::InsertionNotificationRequest HTMLSourceElement::insertedInto(ContainerNode& insertionPoint)
>@@ -121,6 +124,37 @@ bool HTMLSourceElement::isURLAttribute(const Attribute& attribute) const
>     return attribute.name() == srcAttr || HTMLElement::isURLAttribute(attribute);
> }
> 
>+const char* HTMLSourceElement::activeDOMObjectName() const
>+{
>+    return "HTMLSourceElement";
>+}
>+
>+bool HTMLSourceElement::canSuspendForPageCache() const
>+{
>+    return true;
>+}
>+
>+void HTMLSourceElement::suspend(ReasonForSuspension why)
>+{
>+    if (why == PageCache) {
>+        m_shouldRescheduleErrorEventOnResume = m_errorEventTimer.isActive();
>+        m_errorEventTimer.stop();
>+    }
>+}
>+
>+void HTMLSourceElement::resume()
>+{
>+    if (m_shouldRescheduleErrorEventOnResume) {
>+        m_errorEventTimer.startOneShot(0);
>+        m_shouldRescheduleErrorEventOnResume = false;
>+    }
>+}
>+
>+void HTMLSourceElement::stop()
>+{
>+    cancelPendingErrorEvent();
>+}
>+
> }
> 
> #endif
>diff --git a/Source/WebCore/html/HTMLSourceElement.h b/Source/WebCore/html/HTMLSourceElement.h
>index 1df31c1..f373fb9 100644
>--- a/Source/WebCore/html/HTMLSourceElement.h
>+++ b/Source/WebCore/html/HTMLSourceElement.h
>@@ -32,7 +32,7 @@
> 
> namespace WebCore {
> 
>-class HTMLSourceElement final : public HTMLElement {
>+class HTMLSourceElement final : public HTMLElement, public ActiveDOMObject {
> public:
>     static Ref<HTMLSourceElement> create(const QualifiedName&, Document&);
> 
>@@ -52,9 +52,17 @@ private:
>     virtual void removedFrom(ContainerNode&) override;
>     virtual bool isURLAttribute(const Attribute&) const override;
> 
>+    // ActiveDOMObject.
>+    const char* activeDOMObjectName() const override;
>+    bool canSuspendForPageCache() const override;
>+    void suspend(ReasonForSuspension) override;
>+    void resume() override;
>+    void stop() override;
>+
>     void errorEventTimerFired();
> 
>     Timer m_errorEventTimer;
>+    bool m_shouldRescheduleErrorEventOnResume { false };
> };
> 
> } //namespace

Actions: View | Formatted Diff | Diff

Attachments on bug 147020: 256936 | 256985 | 257065