Bug 42603

Summary:

Add a mock in WebCore for testing speech input

Product:

WebKit

Reporter:

Satish Sampath <satish>

Component:

Forms

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, commit-queue, jorlow, morrita, steveblock

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-07-19 15:57:06 PDT

Adds a SpeechInputClientMock class to WebCore and LayoutTestController + WebKit bindings to let layout tests enable the mock class. The mock class lets layout tests specify a string which will be returned as the result when an input element in WebCore invokes speech recognition. Also adds a layout test based on this mock to test the WebCore part of speech input. All possible code changes (except ones in LayoutTestController) are behind the speech input compile time flag (disabled by default). More information about the speech input API proposal can be found in the following links: Parent bug: https://bugs.webkit.org/show_bug.cgi?id=39485 Full API proposal: https://docs.google.com/View?id=dcfg79pz_5dhnp23f5 Relevant discussions in the WHATWG list: - http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2010-May/026338.html - http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2010-June/026747.html

Attachments
Patch (37.22 KB, patch) 2010-07-19 16:19 PDT, Satish Sampath	no flags	Details Formatted Diff Diff
Patch (33.06 KB, patch) 2010-07-27 14:52 PDT, Satish Sampath	no flags	Details Formatted Diff Diff
Patch (14.58 KB, patch) 2010-07-29 12:51 PDT, Satish Sampath	no flags	Details Formatted Diff Diff
Patch (14.58 KB, patch) 2010-07-30 03:58 PDT, Satish Sampath	no flags	Details Formatted Diff Diff
Patch (14.58 KB, patch) 2010-07-30 04:14 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-07-19 16:19:40 PDT

Created attachment 62009 [details] Patch

Steve Block

Comment 2 2010-07-20 01:51:23 PDT

Comment on attachment 62009 [details] Patch WebKit/mac/WebView/WebViewPrivate.h:171 + @method setMockSpeechInputResult: Should this be in its own '@interface WebView (WebViewSpeech)' ? WebCore/page/SpeechInputClientMock.cpp:51 + m_timer.startOneShot(0); This will restart the timer if it's already running. Is this desired? Does the SpeechInput offer any protection against this? LayoutTests/ChangeLog: + * platform/mac/editing/pasteboard/subframe-dragndrop-1-expected.che ?? LayoutTests/fast/forms/script-tests/input-text-speechbutton.js:1 + description('Tests for speech button click with <input type=text speech>.'); Do you need to use &lt here? LayoutTests/fast/forms/script-tests/input-text-speechbutton.js:6 + } It might be good to add a debug message that this test will fail outside DumpRenderTree, when layoutTestController is not available. LayoutTests/fast/forms/script-tests/input-text-speechbutton.js:16 + layoutTestController.notifyDone(); You can use window.jsTestIsAsync and finishJSTest(), in place of waitUntilDone() and notifyDone(). This also adds the 'TEST COMPLETE' output which is currently missing. See fast/dom/Geolocation/script-tests/success.js. LayoutTests/platform/chromium/test_expectations.txt:2925 + BUG44844: fast/forms/input-text-speechbutton.html = TEXT Would it be better to add a new directory for the speech tests to keep them grouped together? You could then just add the directory to the skipped lists (other than for GTK).

Adam Barth

Comment 3 2010-07-27 07:14:18 PDT

Comment on attachment 62009 [details] Patch LayoutTests/ChangeLog:33885 + * platform/mac/editing/pasteboard/subframe-dragndrop-1-expected.che This part of the diff looks strange. LayoutTests/fast/forms/input-text-speechbutton.html:1 + <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> This doctype is very strange. WebCore/Android.mk:385 + page/SpeechInputClientMock.cpp \ I'm not sure where the Mock file should go. We have platform/mock, but this looks like a mock client... WebKit/chromium/public/WebView.h:343 + virtual void setMockSpeechInputResult(const WebString& result) = 0; This will require fishd review. WebKit/mac/WebView/WebViewData.h:180 + OwnPtr<WebCore::SpeechInputClientMock> speechInputClientMock; It's kind of lame that the mock object is owned by each port. We don't have a good pattern established for these, so we might want to think about what will scale nicely to N mocks.

Satish Sampath

Comment 4 2010-07-27 07:19:31 PDT

Thanks Adam. Steve Block has a more scalable proposal in https://bugs.webkit.org/show_bug.cgi?id=39589 , could you please have a look at that and share your comments?

Jeremy Orlow

Comment 5 2010-07-27 07:49:48 PDT

Comment on attachment 62009 [details] Patch Leaving r=? because others may stil wish to comment and none of these issus are terribly bad. LayoutTests/ChangeLog:33884 + \ No newline at end of file Probably best to leave the end of the changelog as is. WebKitTools/DumpRenderTree/mac/LayoutTestControllerMac.mm:345 + only one new line WebKitTools/DumpRenderTree/gtk/LayoutTestControllerGtk.cpp:733 + // Implement this when the Gtk port gets speech input feature. Prefix with "FIXME: "..and maybe add an ASSERT_NOT_IMPLEMENTED().WebKitTools/DumpRenderTree/chromium/LayoutTestController.cpp:1345 + result->setNull(); most methods seem to put this at the end WebKitTools/DumpRenderTree/chromium/LayoutTestController.cpp:1346 + if (arguments.size() != 1 || !arguments[0].isString()) Most methods seem to use > 0 instead of != 1 (which matches javascript semantics better) and seem to put the set inside the if argument...I'd say follow the patterns. WebKit/chromium/src/WebViewImpl.h:303 + void setMockSpeechInputResult(const WebString& result); nit: "result" as the param name might be redundant Btw, SpeechInputClient should inherit form Noncopyable. WebCore/page/SpeechInputClientMock.cpp:61 + } else { no {}'s. WebCore/page/SpeechInputClientMock.cpp:55 + void SpeechInputClientMock::timerFired(Timer<SpeechInputClientMock>*) you might want to consider using 2 timers instead of one since they're really for fairly different purposes.

Satish Sampath

Comment 6 2010-07-27 14:52:49 PDT

Created attachment 62750 [details] Patch Thanks for the reviews. The earlier patch was not suitable in many ways so I have tried to change the design. This new patch is not a complete implementation but just builds and runs on the chromium port. I've uploaded it so I can get feedback on the design before proceeding further. Please see next comment for detailed design.

Satish Sampath

Comment 7 2010-07-27 15:05:33 PDT

The goal is to mock WebCore::SpeechInputClient and use the mock in layout tests for testing the webcore code. - Platform independant mock of WebCore::SpeechInputClient available in WebCore/platform/mock/SpeechInputClientMock.* (the non-mock implementation for chromium port is in WebKit/chromium/src/SpeechInputClientImpl.* for reference) - WebViewImpl constructor decides whether to pass in the mock client or real client to WebCore::Page() based on the 'shouldUseMocks' flag. In the chromium port this is given by WebViewClient::shouldUseMocks() - If the above shouldUseMocks flag is set, WebViewImpl creates a WebViewMocks object which holds all the mock objects, and it will use the mocks in place of real objects during this run. - DumpRenderTree can access the mock object from WebView (e.g. to set mock results) by calling WebView::getMockObjects which returns the whole set of mocks. Adding new mocks requires no change in this call. DRT can call the respective mock interface to set the values for testing. - The mocks themselves along with the WebViewMocks container object reside in a new platform independent WebKit/common directory, with /public holding headers which can be accessed by DRT and other embedders and /src holding the implementation. This code can be used across all c++ ports and new mocks can be added to this location along with other platform-independent code. The Mac port would probably have similar code in Obj-C under WebKit/mac. I'd appreciate feedback on the overall design and code organisation. My reasoning to let WebKit decide to use mocks or real WebCore clients is that WebKit is the embedder for WebCore and the dependency injection decision is cleaner if done at this level. I also felt using a WebViewMocks-style test specific class would let us aggregate all tests related calls from DRT in a single place going forward, instead of polluting WebView directly.

Steve Block

Comment 8 2010-07-28 01:49:10 PDT

Hi Satish, I like the idea of a common mock in WebCore and providing common convenience wrappers in WebKit. A few comments. - I still think that the decision of whether to use the mock or the real implementation should be left to the embedder, not handled by the WebKit layer. I think it's true to say that currently, all testing with LayoutTestController is done this way - implemented in the embedder only. This also avoids polluting WebKit with testing logic. - For the WebKit mock wrapper, I like the Chromum-style approach of an interface in 'public' that replicates the WebCore interface and an impl in 'src' that implements both interfaces. It seems neater than the Mac-style approach of having a 'core()' function which returns the WebCore type being wrapped. One problem with this approach, however, is how to handle interfaces that use WebCore types as method arguments. For speech, this is not a problem, but DeviceOrientationClient, for example, has methods which use the WebCore DeviceOrientation type. The Chromium-style approach would require us to write wrappers for these types too, which means a lot of boilerplate. I guess we need to decide which is best overall. - I'm not sure about the idea of using WebViewMocks to bundle together all the mocks. It seems like it assumes too much about how each platform wants to handle its mocks. I guess there's no requirement for a platform to use it, but I'm not sure it's useful.

Satish Sampath

Comment 9 2010-07-28 02:01:27 PDT

Thanks Steve. > One problem with this approach, however, is how to handle interfaces > that use WebCore types as method arguments This is already happening in my patch, as WebCore::SpeechInputClientMock::setRecognitionResult takes in a WebCore::String and layoutTestController passes in a WebKit::WebString to the webkit mock wrapper, with conversion being done in webkit (transparently). > has methods which use the WebCore DeviceOrientation type In this case wouldn't you have to do a conversion in the WebKit mock, since DRT shouldn't access the WebCore types anyway?

Satish Sampath

Comment 10 2010-07-28 02:03:44 PDT

> I still think that the decision of whether to use the mock or the > real implementation should be left to the embedder, not handled by > the WebKit layer Agreed on the embedder part, but I see WebKit as the embedder for WebCore, not DRT (since DRT can't access WebCore directly). Hence my idea to have WebKit manage injection of WebCore types, and DRT manage injection of WebKit types.

Satish Sampath

Comment 11 2010-07-29 07:22:22 PDT

I discussed this patch with a few people including Steve Block and decided we should go for a simpler approach without all the platform common code stuff in WebKit, so I'll submit a new patch for review once ready.

Satish Sampath

Comment 12 2010-07-29 12:46:16 PDT

I'll submit this as a 2 patches, the first one adding a mock for SpeechInputClient in WebCore which can be shared across ports if required and the next wrapping it in WebKit and using in DRT. I've updated the title of this bug to reflect this, a patch following shortly.

Satish Sampath

Comment 13 2010-07-29 12:51:03 PDT

Created attachment 62977 [details] Patch

Jeremy Orlow

Comment 14 2010-07-30 03:50:12 PDT

Comment on attachment 62977 [details] Patch WebCore/platform/mock/SpeechInputClientMock.cpp:34 + #include "SpeechInputListener.h" put inside the #if ENABLE() (from now on) WebCore/platform/mock/SpeechInputClientMock.cpp:53 + m_timer.startOneShot(0.1); Why the delay? Normally we do with 0 delay to make it harder to have flaky tests. WebCore/platform/mock/SpeechInputClientMock.cpp:85 + m_timer.startOneShot(0.1); ditto r=me please make just the changes I mentioned and then I'll rubber stamp that + commit queue it

Satish Sampath

Comment 15 2010-07-30 03:58:57 PDT

Created attachment 63045 [details] Patch Moved the header inside ifdef, will remember to do it in future as well. The delay of 0.1 seconds was added in the mock because without them the events fire even before the JS engine has a chance to finish parsing all the JS in the layout test.. which resulted in out of order event delivery and hanging.

Jeremy Orlow

Comment 16 2010-07-30 04:03:05 PDT

(In reply to comment #15) > Created an attachment (id=63045) [details] > Patch > > Moved the header inside ifdef, will remember to do it in future as well. The delay of 0.1 seconds was added in the mock because without them the events fire even before the JS engine has a chance to finish parsing all the JS in the layout test.. which resulted in out of order event delivery and hanging. You'll need to come up with a different solution or stipulate that you can only use the mock after the onload has fired in the page. Anything else is racy and will produce a flaky test.

Satish Sampath

Comment 17 2010-07-30 04:06:30 PDT

Using the mock only after onload event sounds fine and will eliminate races in the layout test. However the mock implementation here doesn't have to change since the race is not in the c++ code. I'll mention in the layout test html/js the reason why it is using onload.

Satish Sampath

Comment 18 2010-07-30 04:14:10 PDT

Created attachment 63046 [details] Patch Sets timeout to zero, will use onload in layout test to create the mock after page loads.

Jeremy Orlow

Comment 19 2010-07-30 04:15:52 PDT

Comment on attachment 63046 [details] Patch rubber stamp = me

WebKit Commit Bot

Comment 20 2010-07-30 05:22:07 PDT

Comment on attachment 63046 [details] Patch Clearing flags on attachment: 63046 Committed r64351: <http://trac.webkit.org/changeset/64351>

WebKit Commit Bot

Comment 21 2010-07-30 05:22:13 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.