Bug 146766 – Videos on apple.com don't show up when restored from page cache.

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Andreas Kling]

<rdar://problem/21712311>

Looks like page cache and media elements don't go that well together.

[Andreas Kling]

Created attachment 256442 [details]
Patch (no test)

[Andreas Kling]

Created attachment 256454 [details]
Patch

Now with layout test coverage.

[Darin Adler]

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.

[Andreas Kling]

Committed r186569: <http://trac.webkit.org/changeset/186569>

[Chris Dumez]

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.

[Chris Dumez]

(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.

[Chris Dumez]

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).

[Jer Noble]

We should make sure that HTMLMediaElement::setShouldBufferData(false) is called in this case, now that the MediaPlayerPrivate stays alive in the page cache.