Add a bare bones speech UI for input elements with @speech attribute set

• Component: Forms
• Status: RESOLVED INVALID

[Satish Sampath]

This is the first in a series of patches to enable speech in HTML input elements. A new '@speech' attribute is used to explicitly enable individual form input elements for speech input, and the input element will allow users to start/stop speech recognition and insert the recognition results as the control's value. Please see the parent bug https://bugs.webkit.org/show_bug.cgi?id=39485 for more information.

Backwards compatibility:
1. Web developers will use a new '@speech' attribute to explicitly enable individual
   form input fields for speech input. UAs which don't recognize this new attribute
   will render the input element in it's normal form.
2. Once the initial implementation is ready we intend to enable this API in Chrome
   behind a run-time flag, which will let web developers turn on the feature in their
   own machines and experiment with it to give useful feedback.

Full API proposal: https://docs.google.com/View?id=dcfg79pz_5dhnp23f5

Relevant discussions in the WHATWG lists:
  - 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

Changes in this patch:
  Add bare bones '@speech' attribute support to the text input element and associated rendering
  code. Input elements with '@speech' set will show a clickable button within the text field
  area similar to the clear buttons that appear in <input type='search'> elements.
  a. Compile flag in the build script and makefiles to conditionally enable (default:off) the code
  b. Add speech input button styles to the UA style sheet and CSS parsing code.
  c. Recognize '@speech' as a valid attribute
  d. RenderTextControlSingleLine will check if @speech is set and if so create the speech
     input button in the same fashion as the search field's cancel button.
  e. Platform and UA specific themed rendering (RenderThemeXxxxx files) for the above speech
     input button

[Satish Sampath]

Created attachment 58995 [details]
Patch

[David Levin]

I won't r- this, but this patch is pretty massive. It would be better to have more incremental patches. For example, a first patch could just add support for the define like this: http://trac.webkit.org/changeset/60543

I glanced at the patch, and you had an awesome attention to detail in the code styling. (This is much much better than most folks on first patch including my own.) 

A few minor items that are standard but probably not stated in some doc:
1. It is standard practice to add a copyright line when you change or add more than ~10 lines to a file like this "Copyright (C) 2010 Google Inc. All rights reserved." in your case.
2. You don't need to add " Copyright (C) 2010 Apple Inc. All rights reserved." to new files. In fact the reference to APPLE COMPUTER isn't needed below that (and is out of date as it is Apple, Inc. now anyway). You can use WebCore/loader/ThreadableLoader.h as a good template for the copyright boilerplate.
3. FIXME's don't get names attached to them. (Source history is consider good enough to identify who added it if needed.)
4. Please end comments with "." and only one space after . in comments.
5. Your patch is having trouble being applied by the bots so it would be good to update your local code before submitting a new version.

[Satish Sampath]

Thanks for the comments, I'll address them in my next patch shortly. Regarding starting with a smaller patch - since the speech input API is a proposal/being discussed, I felt this patch was a good way to introduce the feature proposal to reviewers as well. Once a couple of reviewers had a look at it and feel positive about including the API in webkit, I'm happy to split it up if required.

[Jeremy Orlow]

Suggested split points:
1) Adding ENABLE defines
2) Input stuff (minus Platform specific stuff)
3) Chromium specific stuff
4) Layout test

Now, ideally a reviewer would look at all of this together, but splitting things into their own patches will still make it a bit easier for people to grok.

[Satish Sampath]

Created attachment 59099 [details]
Patch

[Satish Sampath]

Addressed David Levin's comments in the latest patch.

[WebKit Review Bot]

Attachment 59099 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3325331

[Dmitry Titov]

So, if your plan is to publish this patch to facilitate the discussion on  the proposal, then I'm not sure why you need a r? flag - since this is a request for a reviewer to do full-blown review that normally precedes landing. 

I'd definitely second David's and Jeremy's suggestion on splitting the patch. Usually, "add API" kind of changes are done in smaller pieces, 'patch per bug' using dependent bugs for implementation patches (see bug 39905 or bug 34990 for example)

[Satish Sampath]

Sorry I didn't mean to say this patch was purely to facilitate discussion. 
Since the speech API proposal is nascent and perhaps the reviewers viewing this patch would be coming across the proposal for the first time, I thought a patch that shows how it would function with a mock implementation was more suitable to apply on their local branch and see what is being proposed. It would be useful to receive some high level feedback on the proposed design (as shown by this patch). And if the feature & design look reasonably ok but the reviewers prefer smaller patches for doing a thorough review, I'm happy to split it up.

[Jeremy Orlow]

Comment on attachment 59099 [details]
Patch

Yes, please go ahead and split it up right off the batt.  r- to remove from the review queue in the mean time.

[Kent Tamura]

Comment on attachment 59099 [details]
Patch

WebCore/WebCore.gypi:3161
 +              'rendering/RenderInputSpeechChromium.cpp',
I guess you introduced RenderInputSppechChromium to share code in RenderThemeChromiumMac and RenderThemeSkia.
But I don't think rendering/RenderInputSpeechChromium is a right place.  Probably it should be in WebCore/platform/chromium/.

[Satish Sampath]

As suggested I have created bug https://bugs.webkit.org/show_bug.cgi?id=40878 which starts with adding support for the compile time feature define, and will break up the remaining parts of this patch as well into smaller patches. Closing this bug as resolved.