Bug 41518

Summary:

Speech input plumbing in webcore

Product:

WebKit

Reporter:

Satish Sampath <satish>

Component:

Forms

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, jorlow, sam, steveblock

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

39485, 42047

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Satish Sampath

Reported 2010-07-02 04:05:03 PDT

Adds the following: - a SpeechInput class in WebCore to be used by the speech enabled HTML elements (which implement SpeechInputElementListener) - a SpeechInputClient interface (defined in WebCore, implemented by WebKit) for WebCore to call into WebKit. This is available as a member of WebCore::Page, set by the platforms which support speech input. - a SpeechInputListener interface for WebCore to receive events from WebKit Also adds a click handler to the speech button which uses the above, and testing code with related DumpRenderTree additions and a layout test.

Attachments
Patch (49.95 KB, patch) 2010-07-02 04:33 PDT, Satish Sampath	no flags	Details Formatted Diff Diff
Patch (25.00 KB, patch) 2010-07-06 15:41 PDT, Satish Sampath	no flags	Details Formatted Diff Diff
Patch (25.53 KB, patch) 2010-07-07 09:37 PDT, Satish Sampath	no flags	Details Formatted Diff Diff
Patch (25.64 KB, patch) 2010-07-07 09:43 PDT, Satish Sampath	no flags	Details Formatted Diff Diff
Patch (25.63 KB, patch) 2010-07-12 09:14 PDT, Satish Sampath	no flags	Details Formatted Diff Diff
Patch (25.77 KB, patch) 2010-07-13 08:13 PDT, Satish Sampath	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Satish Sampath

Comment 1 2010-07-02 04:33:32 PDT

Created attachment 60359 [details] Patch

Satish Sampath

Comment 2 2010-07-02 04:34:26 PDT

All code changes 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

Satish Sampath

Comment 3 2010-07-02 05:44:22 PDT

Adding Sam Weinig and Steve Block to review as they are familiar with similar plumbing code in client based geolocation.

Steve Block

Comment 4 2010-07-06 07:15:11 PDT

Comment on attachment 60359 [details] Patch LayoutTests/fast/forms/input-text-speechbutton.html:22 + layoutTestController.notifyDone(); if (window.layoutTestController) LayoutTests/fast/forms/input-text-speechbutton.html:1 + <html> I think we should be using script-tests for these kind of tests. WebCore/Android.mk:383 + ifeq ($(ENABLE_INPUT_SPEECH), true) No need to guard this. Contents of file should be guarded. WebCore/html/HTMLInputElement.cpp:2777 + return hasAttribute(speechAttr) && document()->page()->isSpeechInputSupported(); Do we need this extra check? Surely any platform that sets ENABLE(INPUT_SPEECH) will provide a speech client, and this entire method is guarded by ENABLE(INPUT_SPEECH). WebKitTools/DumpRenderTree/chromium/LayoutTestController.cpp:170 + #if ENABLE(INPUT_SPEECH) I think this shouldn't be guarded WebKitTools/ChangeLog:13 + * DumpRenderTree/chromium/LayoutTestController.h: I think that all new LayoutTestController methods need to be implemented for Mac. WebKit/chromium/ChangeLog:5 + Speech input plumbing in webcore and webkit Can we leave the WebKit/chromium changes for a separate patch to keep this one simpler? WebCore/page/SpeechInput.h:45 + class SpeechInputElementListener { Shouldn't this be named SpeechInputListener, as it listens to SpeechInput? WebCore/page/SpeechInputClient.h:39 + class SpeechInputListener { Shouldn't this be named SpeechInputClientListener, as it listens to SpeechInputClient? WebCore/page/SpeechInputClient.h:42 + virtual void recognitionResult(const String& result) = 0; setRecognitionResult()? WebCore/page/Page.h:155 + void setSpeechInputClient(SpeechInputClient* client) { m_speechInputClient = client; } Is there any reason for the client to be reset, other than for testing purposes? If not, I don't think we need this method. The embedder can configure their SpeechInputClient to be mocked out without having to call into WebCore. WebCore/page/Page.cpp:133 + , m_speechInputClient(0) For other services (eg Geolocation, DeviceOrientation), the Page owns a FooController object, which holds a weak reference to the client, which is owned by the embedder. The controller takes care of multiplexing multiple requests from the page to the client. Here the Page holds a reference to the client directly. I see that you have a separate SpeechInputElement object for each input. Is there no need for a controller type object? Even if not, it might be best to add a SpeechInputController just to keep the pattern the same. It can have a single createSpeechElement(SpeechInputElementListener*) method.

Satish Sampath

Comment 5 2010-07-06 15:41:52 PDT

Created attachment 60659 [details] Patch Removed non-WebCore code from this patch and addressed Steve Block's other comments.

Satish Sampath

Comment 6 2010-07-06 16:08:20 PDT

Thanks for the review comments Steve. I have now removed all non-WebCore files from this patch and made most of the suggested changes. Here are replies for those which are not implemented: WebCore/page/Page.h:155 + void setSpeechInputClient(SpeechInputClient* client) { m_speechInputClient = client; } > Is there any reason for the client to be reset, other than for testing > purposes? If not, I don't think we need this method. The embedder can > configure their SpeechInputClient to be mocked out without having to > call into WebCore. Testing was one of the main reasons, since there was a need to enable the mock SpeechInputClient when a layout test's JS code says so. Was your suggestion to always use the mock SpeechInputClient in LayoutTestController? If not, I am not sure how else the layout test can enable the mock client object without an additional layer of indirection in WebKit. However, the other reason was the growing number of arguments to the constructor (8 already) and some of the clients passed in as null by majority of the callers. It seemed cleaner to move away from that model as new features get added. WebCore/page/Page.cpp:133 + , m_speechInputClient(0) > For other services (eg Geolocation, DeviceOrientation), the Page > owns a FooController object, which holds a weak reference to the > client, which is owned by the embedder. The controller model did not look like a good fit here since the speech interface requires 2-way communication between WebKit and WebCore, rather than how Geolocation for e.g. works (WebCore calling WebKit to get current location, the result would be same irrespective of how many ever callers invoke this method in WebCore, and all of them get broadcasted the same new location when the result comes in). Here we have each SpeechInput object getting invoked separately by SpeechInputClient when it has new events and no two SpeechInput objects will receive the same speech recognized result data. Effectively the SpeechInput class is the controller here, though there are multiple instances of it.

Satish Sampath

Comment 7 2010-07-07 09:37:50 PDT

Created attachment 60742 [details] Patch per Sam Weinig's suggestion, removed TextControlInnerElements.* from this patch and moved the 2 Listener interfaces to their own files.

Satish Sampath

Comment 8 2010-07-07 09:43:42 PDT

Created attachment 60744 [details] Patch merged with latest master

Steve Block

Comment 9 2010-07-08 03:08:17 PDT

> Testing was one of the main reasons, since there was a need to enable the > mock SpeechInputClient when a layout test's JS code says so. Was your > suggestion to always use the mock SpeechInputClient in LayoutTestController? Yes, or alternatively the embedder can respond to LayoutTestController calls and modify their own SpeechInputClient to mock things appropriately. > However, the other reason was the growing number of arguments to the > constructor (8 already) and some of the clients passed in as null by majority > of the callers. Makes sense > Effectively the SpeechInput class is the controller here, though there are > multiple instances of it. Makes sense This patch looks fine to me. The decision on the last two points above are, I think, policy decisions about how closely we want to adhere to existing patterns. I don't think I have enough WebKit experience to make these decisions, so it would be good if Sam or somebody else with the necessary background could give the r+.

Jeremy Orlow

Comment 10 2010-07-12 07:21:18 PDT

Comment on attachment 60744 [details] Patch steve is probably the best reviewer, but here are some drive by comments WebCore/page/SpeechInputClientListener.h:14 + * * Neither the name of Google Inc. nor the names of its use 2 clause WebCore/page/SpeechInputClient.h:14 + * * Neither the name of Google Inc. nor the names of its use 2 clause WebCore/page/SpeechInput.h:14 + * * Neither the name of Google Inc. nor the names of its use 2 clause WebCore/Android.mk:383 + page/SpeechInput.cpp need \ WebCore/GNUmakefile.am:2687 + WebCore/page/SpeechInputListener.h \ alpha order WebCore/WebCore.gypi:2034 + 'page/SpeechInputListener.h', alpha order WebCore/WebCore.pro:1605 + page/SpeechInputListener.h \ alpha WebCore/WebCore.vcproj/WebCore.vcproj:22476 + RelativePath="..\page\SpeechInputListener.h" alpha WebCore/page/SpeechInput.h:45 + extra new line WebCore/page/SpeechInput.h:37 + #if ENABLE(INPUT_SPEECH) put includes inside this

Satish Sampath

Comment 11 2010-07-12 09:14:28 PDT

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

Steve Block

Comment 12 2010-07-13 03:49:18 PDT

Comment on attachment 61237 [details] Patch It looks like you have guards in some headers, but not all. Is there a reason for this? If is builds either way, we should at least be consistent. Also, why do you guard the inclusion of new files in GNUmakefile.am and WebCore.pro? If the source is guarded, there's no need for this. Looking at these build files, is seems that some features use guards, some don't. Jeremy, do you know what's expected?

Satish Sampath

Comment 13 2010-07-13 03:55:58 PDT

Oops, yes I'll add ENABLE(INPUT_SPEECH) guards for the 2 headers which don't have it now. Regd the makefiles, I was trying to follow existing convention in those files and happy to remove the guards if you think they should be removed (yes the source files build either way since the code is under the guard).

Satish Sampath

Comment 14 2010-07-13 08:13:15 PDT

Created attachment 61377 [details] Patch Addressed Steve's comments.

Steve Block

Comment 15 2010-07-13 08:18:06 PDT

Comment on attachment 61377 [details] Patch r=me

Steve Block

Comment 16 2010-07-13 09:24:13 PDT

Comment on attachment 61377 [details] Patch Looks like the EWS bots have given up on your patch. cq+ but please watch the build bot.

Satish Sampath

Comment 17 2010-07-13 09:25:34 PDT

Thanks. Apparently the bots stop building the patch once it receives r+, so it looks like typically r+ and cq+ go together.

WebKit Commit Bot

Comment 18 2010-07-13 12:16:45 PDT

Comment on attachment 61377 [details] Patch Clearing flags on attachment: 61377 Committed r63230: <http://trac.webkit.org/changeset/63230>

WebKit Commit Bot

Comment 19 2010-07-13 12:16:50 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.