Bug 41518 - Speech input plumbing in webcore
Summary: Speech input plumbing in webcore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 39485 42047
  Show dependency treegraph
 
Reported: 2010-07-02 04:05 PDT by Satish Sampath
Modified: 2010-07-13 12:16 PDT (History)
4 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Satish Sampath 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.
Comment 1 Satish Sampath 2010-07-02 04:33:32 PDT
Created attachment 60359 [details]
Patch
Comment 2 Satish Sampath 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
Comment 3 Satish Sampath 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.
Comment 4 Steve Block 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.
Comment 5 Satish Sampath 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.
Comment 6 Satish Sampath 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.
Comment 7 Satish Sampath 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.
Comment 8 Satish Sampath 2010-07-07 09:43:42 PDT
Created attachment 60744 [details]
Patch

merged with latest master
Comment 9 Steve Block 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+.
Comment 10 Jeremy Orlow 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
Comment 11 Satish Sampath 2010-07-12 09:14:28 PDT
Created attachment 61237 [details]
Patch

Addressed Jeremy's comments.
Comment 12 Steve Block 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?
Comment 13 Satish Sampath 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).
Comment 14 Satish Sampath 2010-07-13 08:13:15 PDT
Created attachment 61377 [details]
Patch

Addressed Steve's comments.
Comment 15 Steve Block 2010-07-13 08:18:06 PDT
Comment on attachment 61377 [details]
Patch

r=me
Comment 16 Steve Block 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.
Comment 17 Satish Sampath 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.
Comment 18 WebKit Commit Bot 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>
Comment 19 WebKit Commit Bot 2010-07-13 12:16:50 PDT
All reviewed patches have been landed.  Closing bug.