[webkit-reviews] review denied: [Bug 81667] Speech JavaScript API: Plumbing for Chromium : [Attachment 135084] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Apr 2 09:56:33 PDT 2012
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.
More information about the webkit-reviews
mailing list