Bug 40776

Summary: Add a bare bones speech UI for input elements with @speech attribute set
Product: WebKit Reporter: Satish Sampath <satish>
Component: FormsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED INVALID    
Severity: Normal CC: andreip, dimich, gustavo, jorlow, levin, mjs, satish, tkent, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: https://docs.google.com/View?id=dcfg79pz_5dhnp23f5
Bug Depends on:    
Bug Blocks: 39485    
Attachments:
Description Flags
Patch
none
Patch jorlow: review-

Satish Sampath
Reported 2010-06-17 07:13:46 PDT
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
Attachments
Patch (70.38 KB, patch)
2010-06-17 07:54 PDT, Satish Sampath 		no flags
DetailsFormatted DiffDiff
Patch (73.92 KB, patch)
2010-06-18 05:38 PDT, Satish Sampath 		jorlow: review-
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Satish Sampath
Comment 1 2010-06-17 07:54:59 PDT
Created attachment 58995 [details] Patch
David Levin
Comment 2 2010-06-17 09:53:05 PDT
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
Comment 3 2010-06-17 10:11:37 PDT
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
Comment 4 2010-06-17 14:49:20 PDT
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
Comment 5 2010-06-18 05:38:18 PDT
Created attachment 59099 [details] Patch
Satish Sampath
Comment 6 2010-06-18 05:48:08 PDT
Addressed David Levin's comments in the latest patch.
WebKit Review Bot
Comment 7 2010-06-18 06:56:13 PDT
Attachment 59099 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/3325331
Dmitry Titov
Comment 8 2010-06-18 15:58:25 PDT
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
Comment 9 2010-06-18 16:12:25 PDT
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 10 2010-06-18 17:50:27 PDT
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 11 2010-06-19 04:15:16 PDT
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
Comment 12 2010-06-19 05:44:57 PDT
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.
Note You need to log in before you can comment on or make changes to this bug.