Summary: | Media Session: add support for platform-specific interruption providers | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matt Rajca <mrajca> | ||||
Component: | Media | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | conrad_shultz, eric.carlson, mrajca, webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 145411 | ||||||
Attachments: |
|
Description
Matt Rajca
2015-07-16 11:12:14 PDT
Created attachment 256916 [details]
Patch
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> |