Bug 146766

Summary: Videos on apple.com don't show up when restored from page cache.
Product: WebKit Reporter: Andreas Kling <kling>
Component: MediaAssignee: Andreas Kling <kling>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, eric.carlson, jer.noble, kling, mmaxfield
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=147254
Attachments:
Description Flags
Patch (no test)
none
Patch darin: review+

Description Andreas Kling 2015-07-08 17:44:46 PDT
<rdar://problem/21712311>

Looks like page cache and media elements don't go that well together.
Comment 1 Andreas Kling 2015-07-08 17:45:34 PDT
Created attachment 256442 [details]
Patch (no test)
Comment 2 Andreas Kling 2015-07-08 19:57:27 PDT
Created attachment 256454 [details]
Patch

Now with layout test coverage.
Comment 3 Darin Adler 2015-07-08 20:07:22 PDT
Comment on attachment 256454 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256454&action=review

> Source/WebCore/html/HTMLMediaElement.cpp:4867
> +    LOG(Media, "HTMLMediaElement::stopWithoutDeletingMediaPlayer(%p)", this);

Name here doesn’t match the actual function name above.
Comment 4 Andreas Kling 2015-07-08 20:29:28 PDT
Committed r186569: <http://trac.webkit.org/changeset/186569>
Comment 5 Chris Dumez 2015-07-09 09:28:38 PDT
Comment on attachment 256454 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256454&action=review

> Source/WebCore/html/HTMLMediaElement.cpp:4870
>          exitFullscreen();

While this is a step in the right direction. I think there are still a few issues.

> Source/WebCore/html/HTMLMediaElement.cpp:4887
>      m_asyncEventQueue.close();

This completely drops pending events (including the one that may have been queued by exitFullscreen() above. What we really want is to delay the event firing until after resume() is called, not drop the pending events.
Comment 6 Chris Dumez 2015-07-09 09:43:42 PDT
(In reply to comment #5)
> Comment on attachment 256454 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=256454&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:4870
> >          exitFullscreen();
> 
> While this is a step in the right direction. I think there are still a few
> issues.
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:4887
> >      m_asyncEventQueue.close();
> 
> This completely drops pending events (including the one that may have been
> queued by exitFullscreen() above. What we really want is to delay the event
> firing until after resume() is called, not drop the pending events.

I mean delaying events is what we really want in the suspend() case. In the stop() case, dropping events like we currently do is fine.
Comment 7 Chris Dumez 2015-07-09 09:43:53 PDT
Comment on attachment 256454 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256454&action=review

>> Source/WebCore/html/HTMLMediaElement.cpp:4887
>>      m_asyncEventQueue.close();
> 
> This completely drops pending events (including the one that may have been queued by exitFullscreen() above. What we really want is to delay the event firing until after resume() is called, not drop the pending events.

The call to cancelPendingEventsAndCallbacks() above is also an issue (for the same reason).
Comment 8 Jer Noble 2015-07-15 09:04:15 PDT
We should make sure that HTMLMediaElement::setShouldBufferData(false) is called in this case, now that the MediaPlayerPrivate stays alive in the page cache.