Created attachment 256442 [details]
Patch (no test)

Created attachment 256454 [details]
Patch

Now with layout test coverage.

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.

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

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.

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

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