[webkit-reviews] review denied: [Bug 66441] Implement WebMediaPlayerClientImpl::audioSourceProvider() and interface into chromium : [Attachment 104287] 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 66441: Implement WebMediaPlayerClientImpl::audioSourceProvider() and
interface into chromium
https://bugs.webkit.org/show_bug.cgi?id=66441

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=104287&action=review


> Source/WebKit/chromium/public/WebMediaPlayer.h:158
> +    virtual WebAudioSourceProvider* webAudioSourceProvider() { return 0; }

nit: we drop the "web" prefix on methods like this.  see for example
WebFrame::dataSource().  that returns a WebDataSource.

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:626
> +void
WebMediaPlayerClientImpl::AudioSourceProviderImpl::provideInput(WebCore::AudioB
us* bus, size_t framesToProcess)

it seems like this function should be listed after we are done defining
all of the WebMediaPlayerClientImpl functions, and methods in the .cpp
file should appear in the same order as they are declared in the .h file.

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.h:173
> +	   void set(WebAudioSourceProvider* provider) {
m_webAudioSourceProvider = provider; }

nit: set -> initialize?  (it seems like "set" is a bit too vague, and
setProvider would be funny too since your implementing a provider.  initialize
seems to capture what you are doing.)