Bug 147008

Created attachment 256916 [details] Patch

<rdar://problem/21858652>

Comment on attachment 256916 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256916&action=review > Source/WebCore/ChangeLog:9 > + * platform/audio/MediaSessionInterruptionProvider.cpp: Added base class to be used by MediaSessionManager to Nit: This isn't audio-specific, so why don't you put it into a new "platform/mediasession" directory? > Source/WebCore/platform/audio/MediaSessionInterruptionProvider.cpp:37 > +MediaSessionInterruptionProvider::MediaSessionInterruptionProvider(MediaSessionInterruptionProviderClient& client) > + : m_client(&client) > +{ > + beginListeningForInterruptions(); > +} Will there ever be more than one interruption provider? If so, what is the lifetime of an object - who creates it? If there will only ever be one, you need a static accessor method and you need a way to add/remove clients.

(In reply to comment #3) > Comment on attachment 256916 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=256916&action=review > > > Source/WebCore/ChangeLog:9 > > + * platform/audio/MediaSessionInterruptionProvider.cpp: Added base class to be used by MediaSessionManager to > > Nit: This isn't audio-specific, so why don't you put it into a new > "platform/mediasession" directory? Will do. > > > Source/WebCore/platform/audio/MediaSessionInterruptionProvider.cpp:37 > > +MediaSessionInterruptionProvider::MediaSessionInterruptionProvider(MediaSessionInterruptionProviderClient& client) > > + : m_client(&client) > > +{ > > + beginListeningForInterruptions(); > > +} > > Will there ever be more than one interruption provider? If so, what is the > lifetime of an object - who creates it? The interruption provider will only be used by the MediaSessionManager singleton (there will only be one client). > > If there will only ever be one, you need a static accessor method and you > need a way to add/remove clients.

Comment on attachment 256916 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256916&action=review > Source/WebCore/platform/audio/MediaSessionInterruptionProvider.h:57 > + // To be overriden by subclasses. > + virtual void beginListeningForInterruptions(); > + virtual void stopListeningForInterruptions(); If these must be overridden, why not make the pure-virtual? > Source/WebCore/platform/audio/mac/MediaSessionInterruptionProviderMac.h:37 > + using MediaSessionInterruptionProvider::MediaSessionInterruptionProvider; Is this necessary?

(In reply to comment #5) > Comment on attachment 256916 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=256916&action=review > > > Source/WebCore/platform/audio/MediaSessionInterruptionProvider.h:57 > > + // To be overriden by subclasses. > > + virtual void beginListeningForInterruptions(); > > + virtual void stopListeningForInterruptions(); > > If these must be overridden, why not make the pure-virtual? These methods are called from the base class's constructor and destructor so they cannot be pure virtual. > > > Source/WebCore/platform/audio/mac/MediaSessionInterruptionProviderMac.h:37 > > + using MediaSessionInterruptionProvider::MediaSessionInterruptionProvider; > > Is this necessary? Yes, constructors aren't inherited.

Committed r186917: <http://trac.webkit.org/changeset/186917>