[webkit-reviews] review denied: [Bug 41518] Speech input plumbing in webcore and webkit : [Attachment 60359] Patch

Tue Jul 6 07:15:11 PDT 2010

Steve Block <steveblock at google.com> has denied Satish Sampath
<satish at chromium.org>'s request for review:
Bug 41518: Speech input plumbing in webcore and webkit
https://bugs.webkit.org/show_bug.cgi?id=41518

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

------- Additional Comments from Steve Block <steveblock at google.com>
LayoutTests/fast/forms/input-text-speechbutton.html:22
 +		layoutTestController.notifyDone();
if (window.layoutTestController)

LayoutTests/fast/forms/input-text-speechbutton.html:1
 +  <html>
I think we should be using script-tests for these kind of tests.

WebCore/Android.mk:383
 +  ifeq ($(ENABLE_INPUT_SPEECH), true)
No need to guard this. Contents of file should be guarded.

WebCore/html/HTMLInputElement.cpp:2777
 +	    return hasAttribute(speechAttr) &&
document()->page()->isSpeechInputSupported();
Do we need this extra check? Surely any platform that sets ENABLE(INPUT_SPEECH)
will provide a speech client, and this entire method is guarded by
ENABLE(INPUT_SPEECH).

WebKitTools/DumpRenderTree/chromium/LayoutTestController.cpp:170
 +  #if ENABLE(INPUT_SPEECH)
I think this shouldn't be guarded

WebKitTools/ChangeLog:13
 +	    * DumpRenderTree/chromium/LayoutTestController.h:
I think that all new LayoutTestController methods need to be implemented for
Mac.

WebKit/chromium/ChangeLog:5
 +	    Speech input plumbing in webcore and webkit
Can we leave the WebKit/chromium changes for a separate patch to keep this one
simpler?

WebCore/page/SpeechInput.h:45
 +  class SpeechInputElementListener {
Shouldn't this be named SpeechInputListener, as it listens to SpeechInput?

WebCore/page/SpeechInputClient.h:39
 +  class SpeechInputListener {
Shouldn't this be named SpeechInputClientListener, as it listens to
SpeechInputClient?

WebCore/page/SpeechInputClient.h:42
 +	virtual void recognitionResult(const String& result) = 0;
setRecognitionResult()?

WebCore/page/Page.h:155
 +	    void setSpeechInputClient(SpeechInputClient* client) {
m_speechInputClient = client; }
Is there any reason for the client to be reset, other than for testing
purposes? If not, I don't think we need this method. The embedder can configure
their SpeechInputClient to be mocked out without having to call into WebCore.

WebCore/page/Page.cpp:133
 +	, m_speechInputClient(0)
For other services (eg Geolocation, DeviceOrientation), the Page owns a
FooController object, which holds a weak reference to the client, which is
owned by the embedder. The controller takes care of multiplexing multiple
requests from the page to the client. Here the Page holds a reference to the
client directly. I see that you have a separate SpeechInputElement object for
each input. Is there no need for a controller type object? Even if not, it
might be best to add a SpeechInputController just to keep the pattern the same.
It can have a single createSpeechElement(SpeechInputElementListener*) method.