[webkit-reviews] review granted: [Bug 126530] Teach MediaSessionManager to manage interruptions : [Attachment 220548] Updated to address Sam's comments

[bugzilla-daemon at webkit.org]

Jer Noble <jer.noble at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 126530: Teach MediaSessionManager to manage interruptions
https://bugs.webkit.org/show_bug.cgi?id=126530

Attachment 220548: Updated to address Sam's comments
https://bugs.webkit.org/attachment.cgi?id=220548&action=review

------- Additional Comments from Jer Noble <jer.noble at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=220548&action=review


Even though this patch touches WebKit2 in only the most cursory way, you should
probably get Sam or another OWNER to check off on it.

> Source/WebCore/html/HTMLMediaElement.cpp:4116
>      bool pausedToBuffer = m_readyStateMaximum >= HAVE_FUTURE_DATA &&
m_readyState < HAVE_FUTURE_DATA;
> -    return (pausedToBuffer || m_readyState >= HAVE_FUTURE_DATA) &&
couldPlayIfEnoughData() && !isBlockedOnMediaController();
> +    
> +    if (!(pausedToBuffer || m_readyState >= HAVE_FUTURE_DATA))
> +	   return false;
> +
> +    return couldPlayIfEnoughData() && !isBlockedOnMediaController();

If you're cleaning this up to be more understandable, the if-clause could be
further cleaned up:

if (!pausedToBuffer && m_readyState < HAVE_FUTURE_DATA)
    return false;

> Source/WebCore/platform/audio/MediaSessionManager.cpp:29
> +#include "HTMLMediaElement.h"

You should remove this #include now that we don't pass in a HTMLMediaElement.

> LayoutTests/media/video-interruption-active-when-element-created.html:34
> +		   setTimeout(checkState, 250);

250ms? Can we bring this down to something slightly smaller?

> LayoutTests/media/video-interruption-active-when-element-created.html:39
> +		   if (window.internals) 

We should just fail the test immediately if window.internals isn't present.

> LayoutTests/media/video-interruption-active-when-element-created.html:40
> +		      
run("window.internals.beginMediaSessionInterruption()");;

The "window." is unnecessary.

> LayoutTests/media/video-interruption-with-resume-allowing-play.html:10
> +		   consoleWrite("250ms timer fired...");

Ditto about 250ms.

> LayoutTests/media/video-interruption-with-resume-allowing-play.html:34
> +		   if (window.internals) 

Ditto about failing early.

> LayoutTests/media/video-interruption-with-resume-not-allowing-play.html:23
> +		       setTimeout(checkState, 250);

Again with the 250ms.

> LayoutTests/media/video-interruption-with-resume-not-allowing-play.html:38
> +		   if (window.internals) 

Again with the failing early.