Bug 43477 – Add speech input controller mock in WebKit and a layout test.

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Satish Sampath]

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.

[Satish Sampath]

Created attachment 63439 [details]
Patch

[WebKit Review Bot]

Attachment 63439 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3624485

[Satish Sampath]

Created attachment 63441 [details]
Patch

[Jeremy Orlow]

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

[Satish Sampath]

Created attachment 63449 [details]
Patch

Addressed Jeremy's comments, more detailed reply below

[Satish Sampath]

(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

[WebKit Review Bot]

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.

[Satish Sampath]

Created attachment 63450 [details]
Patch

Patch with style issues fixed

[Jeremy Orlow]

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

[Satish Sampath]

Created attachment 63469 [details]
Patch

Addressed Jeremy's comments.

[Jeremy Orlow]

Comment on attachment 63469 [details]
Patch

rubber stamp

[Jeremy Orlow]

Comment on attachment 63469 [details]
Patch

Clearing flags on attachment: 63469

Committed r64749: <http://trac.webkit.org/changeset/64749>

[Jeremy Orlow]

All reviewed patches have been landed.  Closing bug.