Bug 43477

Summary: Add speech input controller mock in WebKit and a layout test.
Product: WebKit Reporter: Satish Sampath <satish>
Component: FormsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: hans, jorlow, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 39485    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Satish Sampath 2010-08-04 04:13:51 PDT
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.
Comment 1 Satish Sampath 2010-08-04 04:24:33 PDT
Created attachment 63439 [details]
Patch
Comment 2 WebKit Review Bot 2010-08-04 04:46:58 PDT
Attachment 63439 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3624485
Comment 3 Satish Sampath 2010-08-04 05:07:03 PDT
Created attachment 63441 [details]
Patch
Comment 4 Jeremy Orlow 2010-08-04 06:54:29 PDT
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
Comment 5 Satish Sampath 2010-08-04 07:37:11 PDT
Created attachment 63449 [details]
Patch

Addressed Jeremy's comments, more detailed reply below
Comment 6 Satish Sampath 2010-08-04 07:37:40 PDT
(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
Comment 7 WebKit Review Bot 2010-08-04 07:39:11 PDT
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.
Comment 8 Satish Sampath 2010-08-04 07:41:16 PDT
Created attachment 63450 [details]
Patch

Patch with style issues fixed
Comment 9 Jeremy Orlow 2010-08-04 09:56:12 PDT
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
Comment 10 Satish Sampath 2010-08-04 11:11:08 PDT
Created attachment 63469 [details]
Patch

Addressed Jeremy's comments.
Comment 11 Jeremy Orlow 2010-08-05 04:20:49 PDT
Comment on attachment 63469 [details]
Patch

rubber stamp
Comment 12 Jeremy Orlow 2010-08-05 07:41:06 PDT
Comment on attachment 63469 [details]
Patch

Clearing flags on attachment: 63469

Committed r64749: <http://trac.webkit.org/changeset/64749>
Comment 13 Jeremy Orlow 2010-08-05 07:41:16 PDT
All reviewed patches have been landed.  Closing bug.