We shouldn't waste bandwidth and energy downloading and decoding media if the user isn't playing the media, and has switched to a different tab.
<rdar://problem/15349921>
Created attachment 230011 [details] Patch
Comment on attachment 230011 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230011&action=review > Source/WebCore/platform/audio/MediaSession.h:105 > + bool m_elementIsHidden; This is never initialized, but I don't know how it can be. Maybe it makes more sense to not cache this state but to always ask the client?
Created attachment 230018 [details] Patch
Comment on attachment 230018 [details] Patch 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.
Created attachment 230024 [details] Patch
(In reply to comment #5) > (From update of attachment 230018 [details]) > 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. Agreed. I've changed it.
Comment on attachment 230024 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230024&action=review Thanks! > Source/WebCore/platform/audio/MediaSession.h:130 > +#if ENABLE(PAGE_VISIBILITY_API) > + virtual bool elementIsHidden() const { return false; } > +#endif I don't think this is necessary.
Committed r167734: <http://trac.webkit.org/changeset/167734>