Bug 80260

Summary: Speech JavaScript API: bindings and client interface
Product: WebKit Reporter: Hans Wennborg <hans>
Component: New BugsAssignee: Hans Wennborg <hans>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ojan, satish, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 80410, 81096    
Bug Blocks: 80261    
Attachments:
Description Flags
Patch abarth: review-

Description Hans Wennborg 2012-03-05 03:47:32 PST
Speech JavaScript API: bindings and client interface
Comment 1 Hans Wennborg 2012-03-05 03:58:02 PST
Created attachment 130090 [details]
Patch
Comment 2 Hans Wennborg 2012-03-05 04:14:49 PST
I only updated Chromium's build files (.gyp/.gypi) for now, as I was unsure about which other systems I should update.

Looking at the xcode project, the only Modules I can see are geolocation and websockets. Looking at mediastream, I see it only in the .gypi files and GNUmakefile.list.am.

Some input into which build systems that need to be updated would be great.
Comment 3 Adam Barth 2012-03-05 10:00:35 PST
Comment on attachment 130090 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130090&action=review

The contents of the patch look pretty reasonable on a quick read through.  However, the patch itself is much, much too large.  Would it be possible to send patches for each interface separately?  That would allow us to review each patch carefully.  Also, you'll like need to add more tests throughout this process.

> Source/WebCore/ChangeLog:13
> +        * Modules/scripted_speech/DOMWindowSpeechRecognition.idl: Added.

scripted_speech <- WebKit doesn't use _ in its directory names.  A name like "speech" is probably more appropriate.

DOMWindowSpeechRecognition.idl <- The name of this file should follow the name of the module, so something like DOMWindowSpeech.idl

> Source/WebCore/Modules/scripted_speech/SpeechGrammarList.cpp:55
> +    // FIXME: Is this what we're supposed to do?
> +    m_grammars.append(SpeechGrammar::create(String("data:") + string, weight));

Is the first argument to SpeechGrammar::create intended to be a data URL (or is "data:" used here in another context)?
Comment 4 Adam Barth 2012-03-05 10:02:19 PST
I believe the upload tool should have warned you when you uploaded this patch that it was too large.  As I recall, it recommends patches be 20KB or less, which is 1/5th the size of this patch.
Comment 5 Adam Barth 2012-03-05 10:03:46 PST
> Some input into which build systems that need to be updated would be great.

If this feature is only enabled in Chromium, then we can start out only adding it to the Chromium build system.  When other folks want to enable the feature, we can add it to more build systems.
Comment 6 Hans Wennborg 2012-03-05 10:07:30 PST
(In reply to comment #4)
> I believe the upload tool should have warned you when you uploaded this patch that it was too large.  As I recall, it recommends patches be 20KB or less, which is 1/5th the size of this patch.

Hmm, I don't remember seeing any such warning, but I may be wrong. (I used "webkit-patch upload" from a git repository.)

Thanks for the input. I'll start breaking up the patch into smaller pieces and addressing the other issues tomorrow.