[webkit-reviews] review denied: [Bug 42367] Speech input plumbing in webkit : [Attachment 61809] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Satish Sampath
<satish at chromium.org>'s request for review:
Bug 42367: Speech input plumbing in webkit
https://bugs.webkit.org/show_bug.cgi?id=42367

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
WebKit/chromium/public/WebViewClient.h:340
 +	 virtual WebKit::WebSpeechInputClient* speechInputClient() { return 0;
}
indentation is wrong.  no need for WebKit:: prefix

WebKit/chromium/public/WebViewClient.h:340
 +	 virtual WebKit::WebSpeechInputClient* speechInputClient() { return 0;
}
instead of introducing a new interface here, just add the methods
from WebSpeechInputClient to WebViewClient.

Then rename WebSpeechInputClientListener to WebSpeechInputListener.

WebKit/chromium/public/WebSpeechInputClientListener.h:44
 +	virtual void recordingComplete() { WEBKIT_ASSERT_NOT_REACHED(); }
nit: didCompleteRecording

WebKit/chromium/public/WebSpeechInputClientListener.h:49
 +	virtual void setRecognitionResult(const WebString&) {
WEBKIT_ASSERT_NOT_REACHED(); }
didCompleteRecognition?

WebKit/chromium/public/WebSpeechInputClient.h:45
 +	virtual bool attachListener(WebSpeechInputClientListener*) {
WEBKIT_ASSERT_NOT_REACHED(); }
why do we need multiple listeners?

WebKit/chromium/public/WebSpeechInputClient.h:57
 +	virtual void stopRecording() { WEBKIT_ASSERT_NOT_REACHED(); }
why do we need the recordingComplete (didCompleteRecording) event if
there is an explicit stopRecording method?  are there other ways that
a recording could stop such that WebKit would require the notification
that recording stopped?

WebKit/chromium/public/WebSpeechInputClient.h:53
 +	virtual bool startRecognition() { WEBKIT_ASSERT_NOT_REACHED(); }
nit: might be nice to put the startRecognition / cancelRecognition methods
next to each other since they seem related.

WebKit/chromium/public/WebSpeechInputClient.h:57
 +	virtual void stopRecording() { WEBKIT_ASSERT_NOT_REACHED(); }
should there be some way to start recording again?

WebKit/chromium/public/WebSpeechInputClientListener.h:44
 +	virtual void recordingComplete() { WEBKIT_ASSERT_NOT_REACHED(); }
since these methods are implemented by WebKit, they should be pure virtual.

what if there are multiple pages independently listening for speech
input?	given these interfaces, it seems like one of the pages could
cancel the speech recognition process, hampering the efforts of the
other page.  or, is that dealt with at a lower level within WebCore?