Bug 80260 - Speech JavaScript API: bindings and client interface
Summary: Speech JavaScript API: bindings and client interface
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hans Wennborg
URL:
Keywords:
Depends on: 80410 81096
Blocks: 80261
  Show dependency treegraph
 
Reported: 2012-03-05 03:47 PST by Hans Wennborg
Modified: 2012-03-20 08:59 PDT (History)
5 users (show)

See Also:


Attachments
Patch (100.71 KB, patch)
2012-03-05 03:58 PST, Hans Wennborg
abarth: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.