Input elements in WebCore invoke speech recognition via the WebCore::SpeechInputClient interface. WebKit receives calls on this interface and proxies the requests to the embedder via WebKit::WebSpeechInputController. https://bugs.webkit.org/show_bug.cgi?id=42603 added a mock class which implements WebCore::SpeechInputClient. In this patch we add WebKit::WebSpeechInputControllerMock which implements WebKit::WebSpeechInputController and proxies the mocking logic to the above WebCore mock class. The embedder (DRT in this case) instantiates this mock class and passes to WebKit as the speech input controller to use in tests. Also added a layout test which uses the above mock to test the webcore and webkit speech input plumbing code.
Created attachment 63439 [details] Patch
Attachment 63439 [details] did not build on chromium: Build output: http://queues.webkit.org/results/3624485
Created attachment 63441 [details] Patch
Comment on attachment 63441 [details] Patch 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
Created attachment 63449 [details] Patch Addressed Jeremy's comments, more detailed reply below
(In reply to comment #4) Addressed all comments, some replies below: > (From update of attachment 63441 [details]) > 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? Used forward declaration now, this required TestShell.cpp to also include the mock header file as it constructs the LayoutTestController object. > WebKit/chromium/public/WebSpeechInputControllerMock.h:45 > + virtual void setMockRecognitionResult(const WebString& result) = 0; > Why make this abstract/virtual? since it is an interface and the implementation lives in chromium/src/WebSpeechInputControllerMockImpl.h > 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::WebViewImpl creates WebKit::SpeechInputClientImpl during construction, which calls the DRT WebViewClient for the speech input controller. The mock gets created in this call and returned to WebKit. So by the time the page loads, the mock is valid and available. > 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. Made to an OwnPtr now since the class declaration has been moved to impl.h. > 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. Moved class declaration to impl.h
Attachment 63449 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKit/chromium/src/WebSpeechInputControllerMockImpl.h:36: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 63450 [details] Patch Patch with style issues fixed
Comment on attachment 63450 [details] Patch WebKit/chromium/src/WebSpeechInputControllerMockImpl.cpp:69 + const WebCore::String& result) put on one line WebKit/chromium/src/WebSpeechInputControllerMockImpl.cpp:52 + void WebSpeechInputControllerMockImpl::setMockRecognitionResult( ditto WebKit/chromium/src/WebSpeechInputControllerMockImpl.cpp:39 + // Factory method. comment not needed WebKit/chromium/public/WebSpeechInputControllerMock.h:43 + static WebSpeechInputControllerMock* create(WebSpeechInputListener* listener); Add virtual destructor. r=me
Created attachment 63469 [details] Patch Addressed Jeremy's comments.
Comment on attachment 63469 [details] Patch rubber stamp
Comment on attachment 63469 [details] Patch Clearing flags on attachment: 63469 Committed r64749: <http://trac.webkit.org/changeset/64749>
All reviewed patches have been landed. Closing bug.