[Webkit-unassigned] [Bug 43477] Add speech input controller mock in WebKit and a layout test.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=43477


Jeremy Orlow <jorlow at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #63441|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #4 from Jeremy Orlow <jorlow at chromium.org>  2010-08-04 06:54:29 PST ---
(From update of attachment 63441)
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(arguments[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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.