[webkit-reviews] review denied: [Bug 70650] Add chromium bridging for MediaPlayer::setAudioSource() method so we can know audio stream format : [Attachment 112022] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Chris Rogers
<crogers at google.com>'s request for review:
Bug 70650: Add chromium bridging for MediaPlayer::setAudioSource() method so we
can know audio stream format
https://bugs.webkit.org/show_bug.cgi?id=70650

Attachment 112022: Patch
https://bugs.webkit.org/attachment.cgi?id=112022&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
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.