Add chromium bridging for MediaPlayer::setAudioSource() method so we can know audio stream format
Created attachment 112022 [details] Patch
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment on attachment 112022 [details] Patch Attachment 112022 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10178982
Please see details of audio-related WebCore changes in: https://bugs.webkit.org/show_bug.cgi?id=70155 which describes the WebCore-side changes necessary to get the audio stream format. That patch must land before this one. This patch provides the appropriate bridging/wrappers to plumb this through into the chromium implementation.
Darin, could you please have a look? The builders are currently *red* because the associated WebCore patch has not yet landed. But, hopefully it should still be possible to make sense of this patch in relation to the other one. Thanks!
Comment on attachment 112022 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112022&action=review > Source/WebKit/chromium/public/WebAudioSourceProvider.h:35 > + class Client { nit: we usually go with top-level classes for clients. that helps so that you can just include WebAudioSourceProviderClient.h in header files where this interface is implemented instead of having to import WebAudioSourceProvider as well. it also enables you to forward declare the type. > Source/WebKit/chromium/public/WebMediaPlayer.h:34 > +#include "WebAudioSourceProvider.h" this is a good example where it would be nice to forward declare the Client interface instead. > Source/WebKit/chromium/public/WebMediaPlayer.h:164 > + virtual void setAudioSource(WebKit::WebAudioSourceProvider::Client*) { } given the name of the function, it looks like WebKit::WebAudioSourceProvider::Client could be referred to as an audio source. perhaps instead of WebAudioSourceProviderClient, we should just have WebAudioSource? otherwise, setAudioSource should probably be setAudioSourceProviderClient. i'd also consider making this be a setClient call on WebAudioSourceProvider.
Created attachment 112286 [details] Patch
Comment on attachment 112022 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112022&action=review >> Source/WebKit/chromium/public/WebAudioSourceProvider.h:35 >> + class Client { > > nit: we usually go with top-level classes for clients. that helps so that > you can just include WebAudioSourceProviderClient.h in header files where > this interface is implemented instead of having to import WebAudioSourceProvider > as well. it also enables you to forward declare the type. FIXED: I've split out WebAudioSourceProvider and WebAudioSourceProviderClient into separate header files. >> Source/WebKit/chromium/public/WebMediaPlayer.h:34 >> +#include "WebAudioSourceProvider.h" > > this is a good example where it would be nice to forward declare the Client interface instead. FIXED: there are now no changes to this file >> Source/WebKit/chromium/public/WebMediaPlayer.h:164 > > given the name of the function, it looks like WebKit::WebAudioSourceProvider::Client could be referred > to as an audio source. perhaps instead of WebAudioSourceProviderClient, we should just have > WebAudioSource? otherwise, setAudioSource should probably be setAudioSourceProviderClient. i'd > also consider making this be a setClient call on WebAudioSourceProvider. Darin, thanks for taking the time to talk me through this. As you've suggested, I've gotten rid of a direct setAudioSource() method on MediaPlayer / MediaPlayerPrivate / WebMediaPlayer, etc. and now have instead added a AudioSourceProvider::setClient() method. Hopefully, this will now be slightly easier to understand. I've also uploaded a new patch for the corresponding WebCore files: https://bugs.webkit.org/show_bug.cgi?id=70155
Comment on attachment 112286 [details] Patch Attachment 112286 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10212031
Comment on attachment 112286 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112286&action=review > Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:673 > + m_webAudioSourceProvider = provider; before replacing m_webAudioSourceProvider, should we perhaps null out the client of the existing provider? if (m_webAudioSourceProvider) m_webAudioSourceProvider->setClient(0); m_webAudioSourceProvider = provider; if (m_webAudioSourceProvider) m_webAudioSourceProvider->setClient(&m_client); This way we don't have to worry about the old provider holding a pointer back to m_client. > Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:678 > +void WebMediaPlayerClientImpl::AudioSourceProviderImpl::setClient(WebCore::AudioSourceProviderClient* client) nit: it looks like you can drop the WebCore:: prefixes in this file. R=me otherwise.
Committed r98514: <http://trac.webkit.org/changeset/98514>