[webkit-reviews] review granted: [Bug 132077] [Mac, iOS] Stop buffering media when on an inactive tab : [Attachment 230018] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Apr 23 16:30:05 PDT 2014
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.
More information about the webkit-reviews
mailing list