You need to
before you can comment on or make changes to this bug.
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.
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:
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
Created an attachment (id=58995) [details]
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.
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.
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.
Created an attachment (id=59099) [details]
Addressed David Levin's comments in the latest patch.
Attachment 59099 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3325331
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)
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.
(From update of attachment 59099 [details])
Yes, please go ahead and split it up right off the batt. r- to remove from the review queue in the mean time.
(From update of attachment 59099 [details])
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/.
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.