Bug 43477

Summary:

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

Product:

WebKit

Reporter:

Satish Sampath <satish>

Component:

Forms

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

hans, jorlow, webkit.review.bot

Priority:

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

Satish Sampath

Reported 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.

Attachments
Patch (27.68 KB, patch) 2010-08-04 04:24 PDT, Satish Sampath	no flags	Details Formatted Diff Diff
Patch (27.60 KB, patch) 2010-08-04 05:07 PDT, Satish Sampath	no flags	Details Formatted Diff Diff
Patch (30.45 KB, patch) 2010-08-04 07:37 PDT, Satish Sampath	no flags	Details Formatted Diff Diff
Patch (30.45 KB, patch) 2010-08-04 07:41 PDT, Satish Sampath	no flags	Details Formatted Diff Diff
Patch (30.37 KB, patch) 2010-08-04 11:11 PDT, Satish Sampath	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Satish Sampath

Comment 1 2010-08-04 04:24:33 PDT

Created attachment 63439 [details] Patch

WebKit Review Bot

Comment 2 2010-08-04 04:46:58 PDT

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

Satish Sampath

Comment 3 2010-08-04 05:07:03 PDT

Created attachment 63441 [details] Patch

Jeremy Orlow

Comment 4 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

Satish Sampath

Comment 5 2010-08-04 07:37:11 PDT

Created attachment 63449 [details] Patch Addressed Jeremy's comments, more detailed reply below

Satish Sampath

Comment 6 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

WebKit Review Bot

Comment 7 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.

Satish Sampath

Comment 8 2010-08-04 07:41:16 PDT

Created attachment 63450 [details] Patch Patch with style issues fixed

Jeremy Orlow

Comment 9 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

Satish Sampath

Comment 10 2010-08-04 11:11:08 PDT

Created attachment 63469 [details] Patch Addressed Jeremy's comments.

Jeremy Orlow

Comment 11 2010-08-05 04:20:49 PDT

Comment on attachment 63469 [details] Patch rubber stamp

Jeremy Orlow

Comment 12 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>

Jeremy Orlow

Comment 13 2010-08-05 07:41:16 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.