[webkit-reviews] review denied: [Bug 43477] Add speech input controller mock in WebKit and a layout test. : [Attachment 63441] Patch

[bugzilla-daemon at webkit.org]

Jeremy Orlow <jorlow at chromium.org> has denied Satish Sampath
<satish at chromium.org>'s request for review:
Bug 43477: Add speech input controller mock in WebKit and a layout test.
https://bugs.webkit.org/show_bug.cgi?id=43477

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

------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
WebKitTools/DumpRenderTree/chromium/WebViewHost.cpp:504
 +  WebKit::WebSpeechInputController*
WebViewHost::speechInputController(WebKit::WebSpeechInputListener* listener)
No need for WEbKit::

WebKitTools/DumpRenderTree/chromium/LayoutTestController.h:46
 +  #include "public/WebSpeechInputControllerMock.h"
Are you sure you need to do the include in this file rather than forward
declare?

WebKit/chromium/public/WebSpeechInputControllerMock.h:45
 +	virtual void setMockRecognitionResult(const WebString& result) = 0;
Why make this abstract/virtual?

WebKitTools/DumpRenderTree/chromium/LayoutTestController.cpp:1381
 +     
m_speechInputControllerMock->setMockRecognitionResult(cppVariantToWebString(arg
uments[0]));
Are you sure m_speechInputControllerMock will always be created in time?  I'm
not sure it will.

WebKit/chromium/src/WebSpeechInputControllerMockImpl.cpp:44
 +  class WebSpeechInputControllerMockImpl
no need to put on the same line

WebKit/chromium/src/WebSpeechInputControllerMockImpl.cpp:40
 +  #if ENABLE(INPUT_SPEECH)
move above the initial includes....actually delete the guards since it'll
always be on for Chromium

WebKit/chromium/src/WebSpeechInputControllerMockImpl.cpp:64
 +	WebCore::SpeechInputClientMock m_webcoreMock;
Hmm....usually we use an OwnPtr, but I don't have a good reason why you'd need
to.

WebKit/chromium/src/WebSpeechInputControllerMockImpl.cpp:116
 +  #if ENABLE(INPUT_SPEECH)
Just assume INPUT_SPEECH is always on for Chromium

WebKit/chromium/src/WebSpeechInputControllerMockImpl.cpp:44
 +  class WebSpeechInputControllerMockImpl
I'd slightly lean towards moving this impl stuff to a header file.  It just
seems weird to have an Impl.cpp whose corresponding .h is of a different name
and I don't know of any other similar examples.

WebKit/chromium/src/WebSpeechInputControllerMockImpl.cpp:113
 +  // Factory method.
I'd put the factory more towards the top.


looks pretty good