[webkit-reviews] review granted: [Bug 132077] [Mac, iOS] Stop buffering media when on an inactive tab : [Attachment 230018] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 132077: [Mac, iOS] Stop buffering media when on an inactive tab
https://bugs.webkit.org/show_bug.cgi?id=132077

Attachment 230018: Patch
https://bugs.webkit.org/attachment.cgi?id=230018&action=review

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


> Source/WebCore/html/HTMLMediaElement.cpp:6147
> +void HTMLMediaElement::shouldBufferData(bool shouldBuffer)

I should have noticed this before, but "shouldBufferData" sounds like it is a
getter. Maybe "setShouldBufferData"?

> Source/WebCore/html/HTMLMediaElement.cpp:6150
> +	   return m_player->shouldBufferData(shouldBuffer);

Ditto.

> Source/WebCore/platform/audio/MediaSession.cpp:64
> +    , m_elementIsHidden(false)

This isn't used.

> Source/WebCore/platform/audio/MediaSession.cpp:111
> +    clientShouldBufferDataIfNotHiddenOrPlaying();

Nit: this name is a mouthful!

> Source/WebCore/platform/audio/MediaSession.cpp:175
> +void MediaSession::clientShouldBufferDataIfNotHiddenOrPlaying()
> +{
> +    if (m_client.elementIsHidden() && m_state != Playing)
> +	   m_client.shouldBufferData(false);
> +    else
> +	   m_client.shouldBufferData(true);
> +}

I think part of the reason the name is so long is because it is used by the
client and internally by the session. I would prefer something like
"visibilityChanged()" for the client and "updateClientDataBuffering" for use
within the session.