Summary: | [Mac, iOS] Stop buffering media when on an inactive tab | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brent Fulgham <bfulgham> | ||||||||
Component: | Media | Assignee: | Brent Fulgham <bfulgham> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bfulgham, calvaris, commit-queue, eric.carlson, esprehn+autocc, glenn, gyuyoung.kim, jer.noble, philipj, sergio | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Brent Fulgham
2014-04-23 14:23:56 PDT
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> |