[webkit-reviews] review denied: [Bug 66230] Also release media resources when media is completely loaded. : [Attachment 104161] Proposed patch 2.

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has denied Hao Zheng
<zhenghao at chromium.org>'s request for review:
Bug 66230: Also release media resources when media is completely loaded.
https://bugs.webkit.org/show_bug.cgi?id=66230

Attachment 104161: Proposed patch 2.
https://bugs.webkit.org/attachment.cgi?id=104161&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=104161&action=review


> Source/WebCore/html/HTMLMediaElement.cpp:2654
> +void HTMLMediaElement::documentWillBecomeInactive()
> +{
> +#if !ENABLE(PLUGIN_PROXY_FOR_VIDEO)
> +    LOG(Media, "HTMLMediaElement::documentWillBecomeInactive");
> +    if (m_player)
> +	   m_player->willBecomeInactive();
> +#endif
> +}
> +
> +void HTMLMediaElement::documentDidBecomeActive()
> +{
> +#if !ENABLE(PLUGIN_PROXY_FOR_VIDEO)
> +    LOG(Media, "HTMLMediaElement::documentDidBecomeActive");
> +    if (m_player)
> +	   m_player->didBecomeActive();
> +#endif
> +}
> +

These new methods are unnecessary. documentWillBecomeInactive is called by
Document::detach, *after* it calls stopActiveDOMObjects(). HTMLMediaElement is
an ActiveDOMObject.

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:243
> +void WebMediaPlayerClientImpl::willBecomeInactive()
> +{
> +    if (m_webMediaPlayer.get())
> +	   m_webMediaPlayer->willBecomeInactive();
> +}
> +
> +void WebMediaPlayerClientImpl::didBecomeActive()
> +{
> +    if (m_webMediaPlayer.get())
> +	   m_webMediaPlayer->didBecomeActive();
> +}
> +

I assume that the Chromium media player sets network and ready states correctly
when it unloads cached media data? Where are the tests to show that the
behavior is correct.?