[webkit-reviews] review denied: [Bug 81667] Speech JavaScript API: Plumbing for Chromium : [Attachment 135084] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Hans Wennborg
<hans at chromium.org>'s request for review:
Bug 81667: Speech JavaScript API: Plumbing for Chromium
https://bugs.webkit.org/show_bug.cgi?id=81667

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

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


Interface structure looks pretty good.	Just some minor nits now.

> Source/WebKit/chromium/public/WebSpeechRecognitionHandle.h:41
> +// A handle to a SpeechRecognition object. As long as there is a handle, the


nit: avoid making the user think about WebCore objects.  mentioning the
SpeechRecognition
type in this comment breaks that rule.	I would just drop the first two
sentences here,
and then I would change the third sentence to begin as
"WebSpeechRecognitionHandle is
used by the WebSpeechRecognizer..."

> Source/WebKit/chromium/public/WebSpeechRecognitionParams.h:39
> +    WebSpeechRecognitionParams(const WebVector<WebSpeechGrammar>&, const
WebString& language, bool continuous);

nit: implement the constructor inline or else it needs to be tagged with
WEBKIT_EXPORT.	usually we just implement inline.

> Source/WebKit/chromium/public/WebSpeechRecognizer.h:41
> +    virtual void start(WebSpeechRecognitionHandle, const
WebSpeechRecognitionParams&, WebSpeechRecognizerClient*) {
WEBKIT_ASSERT_NOT_REACHED(); }

nit: pass handles as "const WebSpeechRecognitionHandle&" ... you should see
other classes that are implemented using WebPrivatePtr being passed as const
ref.

> Source/WebKit/chromium/public/WebSpeechRecognizerClient.h:44
> +    virtual void didStartAudio(WebSpeechRecognitionHandle&) = 0;

nit: pass handles as "const WebSpeechRecognitionHandle&"

> Source/WebKit/chromium/src/SpeechRecognitionClientProxy.cpp:76
> +    RefPtr<SpeechRecognition> recognition =
static_cast<PassRefPtr<SpeechRecognition> >(handle);

the explicit cast to PassRefPtr<SpeechRecognition> should become unnecessary
once you
change the argument type to "const WebSpeechRecognitionHandle&" as the
conversion
operator defined on WebSpeechRecognitionHandle will then be used implicitly.