Introducing new data types and IPC messages for scripted JS speech recognition APIs (Speech CL2.0)

Introducing new data types and IPC messages for scripted JS speech recognition APIs (Speech CL2.0)

Furthermore GoogleOneShotRemoteEngineConfig struct has been renamed and moved into the (more generic) SpeechRecognitionEngineConfig, that will be shared with the streaming recognition engine implementation.


BUG=116954
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137141

[Primiano Tucci (use gerrit), 2012-04-26 15:43:10]

Forgot to add also speech_recognition_session_context.h

[hans, 2012-04-26 16:23:25]

lgtm, with some nits below

also, i suppose you'll need owner's approval from jam?

https://chromiumcodereview.appspot.com/10233010/diff/3004/content/browser/spe...
File content/browser/speech/speech_recognition_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10233010/diff/3004/content/browser/spe...
content/browser/speech/speech_recognition_browsertest.cc:79: grammar_ =
config.grammars.at(0).url;
i'd just use grammars[0].url, but no big deal

https://chromiumcodereview.appspot.com/10233010/diff/3004/content/browser/spe...
File content/browser/speech/speech_recognizer_impl.cc (right):

https://chromiumcodereview.appspot.com/10233010/diff/3004/content/browser/spe...
content/browser/speech/speech_recognizer_impl.cc:23: using
content::SpeechRecognitionGrammar;
nit: sort order

https://chromiumcodereview.appspot.com/10233010/diff/3004/content/common/spee...
File content/common/speech_recognition_messages.h (right):

https://chromiumcodereview.appspot.com/10233010/diff/3004/content/common/spee...
content/common/speech_recognition_messages.h:112: // Speech grammar to use.
nit: s/grammar/grammars/

https://chromiumcodereview.appspot.com/10233010/diff/3004/content/common/spee...
content/common/speech_recognition_messages.h:144:
IPC_MESSAGE_ROUTED2(SpeechRecognitionMsg_ResultRetrieved,
we should probably have comments for this, and the messages below too..

https://chromiumcodereview.appspot.com/10233010/diff/3004/content/public/brow...
File content/public/browser/speech_recognition_session_context.h (right):

https://chromiumcodereview.appspot.com/10233010/diff/3004/content/public/brow...
content/public/browser/speech_recognition_session_context.h:15: // elements
involved with the it (e.g., the page element that requested the
"with the it"?

https://chromiumcodereview.appspot.com/10233010/diff/3004/content/public/comm...
File content/public/common/speech_recognition_error.h (right):

https://chromiumcodereview.appspot.com/10233010/diff/3004/content/public/comm...
content/public/common/speech_recognition_error.h:44:
SpeechRecognitionError(SpeechRecognitionErrorCode code_value)
i probably missed this before, but i believe this constructor should be marked
"explicit"

https://chromiumcodereview.appspot.com/10233010/diff/3004/content/public/comm...
File content/public/common/speech_recognition_grammar.h (right):

https://chromiumcodereview.appspot.com/10233010/diff/3004/content/public/comm...
content/public/common/speech_recognition_grammar.h:6: #define
CONTENT_PUBLIC_COMMON_SPEECH_RECOGNITION_GRAMMAR_H_
#pragma once

https://chromiumcodereview.appspot.com/10233010/diff/3004/content/public/comm...
content/public/common/speech_recognition_grammar.h:22:
SpeechRecognitionGrammar(std::string url_value)
explicit

https://chromiumcodereview.appspot.com/10233010/diff/3004/content/public/comm...
File content/public/common/speech_recognition_result.cc (right):

https://chromiumcodereview.appspot.com/10233010/diff/3004/content/public/comm...
content/public/common/speech_recognition_result.cc:17: 
looks like an extra blank line here

[Primiano Tucci (use gerrit), 2012-04-27 09:27:13]

https://chromiumcodereview.appspot.com/10233010/diff/3004/content/browser/spe...
File content/browser/speech/speech_recognition_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10233010/diff/3004/content/browser/spe...
content/browser/speech/speech_recognition_browsertest.cc:79: grammar_ =
config.grammars.at(0).url;
On 2012/04/26 16:23:25, hans wrote:
> i'd just use grammars[0].url, but no big deal

Done.

https://chromiumcodereview.appspot.com/10233010/diff/3004/content/browser/spe...
File content/browser/speech/speech_recognizer_impl.cc (right):

https://chromiumcodereview.appspot.com/10233010/diff/3004/content/browser/spe...
content/browser/speech/speech_recognizer_impl.cc:23: using
content::SpeechRecognitionGrammar;
On 2012/04/26 16:23:25, hans wrote:
> nit: sort order

Done.

https://chromiumcodereview.appspot.com/10233010/diff/3004/content/common/spee...
File content/common/speech_recognition_messages.h (right):

https://chromiumcodereview.appspot.com/10233010/diff/3004/content/common/spee...
content/common/speech_recognition_messages.h:112: // Speech grammar to use.
On 2012/04/26 16:23:25, hans wrote:
> nit: s/grammar/grammars/

Done.

https://chromiumcodereview.appspot.com/10233010/diff/3004/content/public/brow...
File content/public/browser/speech_recognition_session_context.h (right):

https://chromiumcodereview.appspot.com/10233010/diff/3004/content/public/brow...
content/public/browser/speech_recognition_session_context.h:15: // elements
involved with the it (e.g., the page element that requested the
On 2012/04/26 16:23:25, hans wrote:
> "with the it"?
Oops.

https://chromiumcodereview.appspot.com/10233010/diff/3004/content/public/comm...
File content/public/common/speech_recognition_error.h (right):

https://chromiumcodereview.appspot.com/10233010/diff/3004/content/public/comm...
content/public/common/speech_recognition_error.h:44:
SpeechRecognitionError(SpeechRecognitionErrorCode code_value)
On 2012/04/26 16:23:25, hans wrote:
> i probably missed this before, but i believe this constructor should be marked
> "explicit"
uh, right.

https://chromiumcodereview.appspot.com/10233010/diff/3004/content/public/comm...
File content/public/common/speech_recognition_grammar.h (right):

https://chromiumcodereview.appspot.com/10233010/diff/3004/content/public/comm...
content/public/common/speech_recognition_grammar.h:6: #define
CONTENT_PUBLIC_COMMON_SPEECH_RECOGNITION_GRAMMAR_H_
On 2012/04/26 16:23:25, hans wrote:
> #pragma once
Uh, for some strange reason I am missing #pragma once also in a lot of previous
files. I'll do a separate CL to fix it.

https://chromiumcodereview.appspot.com/10233010/diff/3004/content/public/comm...
content/public/common/speech_recognition_grammar.h:22:
SpeechRecognitionGrammar(std::string url_value)
On 2012/04/26 16:23:25, hans wrote:
> explicit

Done.

https://chromiumcodereview.appspot.com/10233010/diff/3004/content/public/comm...
File content/public/common/speech_recognition_result.cc (right):

https://chromiumcodereview.appspot.com/10233010/diff/3004/content/public/comm...
content/public/common/speech_recognition_result.cc:17: 
On 2012/04/26 16:23:25, hans wrote:
> looks like an extra blank line here

Done.

[Satish, 2012-04-27 10:04:41]

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/browser/sp...
File content/browser/speech/google_one_shot_remote_engine.cc (right):

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/browser/sp...
content/browser/speech/google_one_shot_remote_engine.cc:193:
parts.push_back("lm=" + net::EscapeQueryParamValue(config_.grammars[0].url,
can we add a DCHECK here to ensure only 1 grammar is present?

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/browser/sp...
File content/browser/speech/input_tag_speech_dispatcher_host.cc (right):

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/browser/sp...
content/browser/speech/input_tag_speech_dispatcher_host.cc:103: if
(!params.grammar.empty())
suggest using braces here since the body of the if block is multiple lines

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/browser/sp...
File content/browser/speech/speech_recognition_engine.cc (right):

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/browser/sp...
content/browser/speech/speech_recognition_engine.cc:11:
audio_sample_rate(16000),
can these be moved to constants in an anonymous namespace?

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/browser/sp...
File content/browser/speech/speech_recognition_engine.h (right):

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/browser/sp...
content/browser/speech/speech_recognition_engine.h:23: struct
SpeechRecognitionEngineConfig {
move this as an inner class of SpeechRecognitionEngine similar to Delegate ?

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/browser/sp...
content/browser/speech/speech_recognition_engine.h:45: //
SpeechRecognitionEnding only after calling EndRecognition.
Ending -> Engine

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/common/spe...
File content/common/speech_recognition_messages.h (right):

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/common/spe...
content/common/speech_recognition_messages.h:18:
IPC_ENUM_TRAITS(content::SpeechAudioErrorDetails)
I'd prefer to see these IPC message additions in the relevant future CLs where
they are used first.

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/public/bro...
File content/public/browser/speech_recognition_session_config.h (right):

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/public/bro...
content/public/browser/speech_recognition_session_config.h:30: bool continuous;
this name is very generic. perhaps add a comment here that this indicates
continuous recognition that doesn't stop automatically at silence

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/public/bro...
File content/public/browser/speech_recognition_session_context.h (right):

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/public/bro...
content/public/browser/speech_recognition_session_context.h:34: int
js_handle_id;
typically 'handle' and 'id' are used mutually exclusive. What is the purpose of
this field?

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/public/com...
File content/public/common/speech_recognition_error.h (right):

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/public/com...
content/public/common/speech_recognition_error.h:41: SpeechRecognitionError()
style guide forbids inlining constructors in header files, so can you add a .cc
and move the body of these into that?

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/public/com...
File content/public/common/speech_recognition_grammar.h (right):

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/public/com...
content/public/common/speech_recognition_grammar.h:20:
SpeechRecognitionGrammar()
instead of inlining these move them to a .cc file

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/public/com...
File content/public/common/speech_recognition_result.h (right):

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/public/com...
content/public/common/speech_recognition_result.h:21:
SpeechRecognitionHypothesis() : confidence(0.0) {}
move constructor definitions to .cc file

[hans, 2012-04-27 10:53:29]

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/public/com...
File content/public/common/speech_recognition_error.h (right):

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/public/com...
content/public/common/speech_recognition_error.h:41: SpeechRecognitionError()
On 2012/04/27 10:04:41, Satish wrote:
> style guide forbids inlining constructors in header files, so can you add a
.cc
> and move the body of these into that?

I think in practice it's fine to avoid the extra .cc file as long as the
style-checker plugin doesn't complain.

See for example jam's second comment in
http://codereview.chromium.org/9972011/diff/45003/content/public/browser/spee...
where he questions the need to out-of-line the ctor/dtor.

The style guide rule is there to avoid inlining complex constructors.

[Satish, 2012-04-27 11:08:02]

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/public/com...
File content/public/common/speech_recognition_error.h (right):

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/public/com...
content/public/common/speech_recognition_error.h:41: SpeechRecognitionError()
On 2012/04/27 10:53:30, hans wrote:
> On 2012/04/27 10:04:41, Satish wrote:
> > style guide forbids inlining constructors in header files, so can you add a
> .cc
> > and move the body of these into that?
> 
> I think in practice it's fine to avoid the extra .cc file as long as the
> style-checker plugin doesn't complain.
> 
> See for example jam's second comment in
>
http://codereview.chromium.org/9972011/diff/45003/content/public/browser/spee...
> where he questions the need to out-of-line the ctor/dtor.
> 
> The style guide rule is there to avoid inlining complex constructors.

Thats what I thought earlier, but the style guide above says "Even for simple
classes, putting the function in the .cc file helps avoid this when someone
later adds a new variable to the class." - which is true. I'm happy either way
but want to be consistent, and per style guide consistent means not having any
inline constructors.

jam@, do you have a strong preference to inline constructors? If so, should we
allow it in the style guide and not leave it as is?

[jam, 2012-04-27 14:43:26]

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/public/com...
File content/public/common/speech_recognition_error.h (right):

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/public/com...
content/public/common/speech_recognition_error.h:41: SpeechRecognitionError()
On 2012/04/27 11:08:02, Satish wrote:
> On 2012/04/27 10:53:30, hans wrote:
> > On 2012/04/27 10:04:41, Satish wrote:
> > > style guide forbids inlining constructors in header files, so can you add
a
> > .cc
> > > and move the body of these into that?
> > 
> > I think in practice it's fine to avoid the extra .cc file as long as the
> > style-checker plugin doesn't complain.
> > 
> > See for example jam's second comment in
> >
>
http://codereview.chromium.org/9972011/diff/45003/content/public/browser/spee...
> > where he questions the need to out-of-line the ctor/dtor.
> > 
> > The style guide rule is there to avoid inlining complex constructors.
> 
> Thats what I thought earlier, but the style guide above says "Even for simple
> classes, putting the function in the .cc file helps avoid this when someone
> later adds a new variable to the class." - which is true. I'm happy either way
> but want to be consistent, and per style guide consistent means not having any
> inline constructors.
> 
> jam@, do you have a strong preference to inline constructors? If so, should we
> allow it in the style guide and not leave it as is?

For files in content/public we have tried to avoid cc files as much as possible,
only adding them so that clank doesn't complain. The reasoning is that it's a
public directory, so (ideally) should just be headers

[Satish, 2012-04-27 14:45:34]

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/public/com...
File content/public/common/speech_recognition_error.h (right):

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/public/com...
content/public/common/speech_recognition_error.h:41: SpeechRecognitionError()
On 2012/04/27 14:43:26, John Abd-El-Malek wrote:
> On 2012/04/27 11:08:02, Satish wrote:
> > On 2012/04/27 10:53:30, hans wrote:
> > > On 2012/04/27 10:04:41, Satish wrote:
> > > > style guide forbids inlining constructors in header files, so can you
add
> a
> > > .cc
> > > > and move the body of these into that?
> > > 
> > > I think in practice it's fine to avoid the extra .cc file as long as the
> > > style-checker plugin doesn't complain.
> > > 
> > > See for example jam's second comment in
> > >
> >
>
http://codereview.chromium.org/9972011/diff/45003/content/public/browser/spee...
> > > where he questions the need to out-of-line the ctor/dtor.
> > > 
> > > The style guide rule is there to avoid inlining complex constructors.
> > 
> > Thats what I thought earlier, but the style guide above says "Even for
simple
> > classes, putting the function in the .cc file helps avoid this when someone
> > later adds a new variable to the class." - which is true. I'm happy either
way
> > but want to be consistent, and per style guide consistent means not having
any
> > inline constructors.
> > 
> > jam@, do you have a strong preference to inline constructors? If so, should
we
> > allow it in the style guide and not leave it as is?
> 
> For files in content/public we have tried to avoid cc files as much as
possible,
> only adding them so that clank doesn't complain. The reasoning is that it's a
> public directory, so (ideally) should just be headers

Cool, makes sense. Is it ok to mention this in the style guide as the one
exception to the inlining rule?

[Primiano Tucci (use gerrit), 2012-04-27 15:58:44]

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/browser/sp...
File content/browser/speech/google_one_shot_remote_engine.cc (right):

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/browser/sp...
content/browser/speech/google_one_shot_remote_engine.cc:193:
parts.push_back("lm=" + net::EscapeQueryParamValue(config_.grammars[0].url,
On 2012/04/27 10:04:41, Satish wrote:
> can we add a DCHECK here to ensure only 1 grammar is present?

Done.

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/browser/sp...
File content/browser/speech/input_tag_speech_dispatcher_host.cc (right):

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/browser/sp...
content/browser/speech/input_tag_speech_dispatcher_host.cc:103: if
(!params.grammar.empty())
On 2012/04/27 10:04:41, Satish wrote:
> suggest using braces here since the body of the if block is multiple lines

Done.

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/browser/sp...
File content/browser/speech/speech_recognition_engine.cc (right):

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/browser/sp...
content/browser/speech/speech_recognition_engine.cc:11:
audio_sample_rate(16000),
On 2012/04/27 10:04:41, Satish wrote:
> can these be moved to constants in an anonymous namespace?

Done. Even if IMHO doesn't seem such a big deal.

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/browser/sp...
File content/browser/speech/speech_recognition_engine.h (right):

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/browser/sp...
content/browser/speech/speech_recognition_engine.h:23: struct
SpeechRecognitionEngineConfig {
On 2012/04/27 10:04:41, Satish wrote:
> move this as an inner class of SpeechRecognitionEngine similar to Delegate ?

Done.

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/browser/sp...
content/browser/speech/speech_recognition_engine.h:45: //
SpeechRecognitionEnding only after calling EndRecognition.
On 2012/04/27 10:04:41, Satish wrote:
> Ending -> Engine

Done.

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/common/spe...
File content/common/speech_recognition_messages.h (right):

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/common/spe...
content/common/speech_recognition_messages.h:18:
IPC_ENUM_TRAITS(content::SpeechAudioErrorDetails)
On 2012/04/27 10:04:41, Satish wrote:
> I'd prefer to see these IPC message additions in the relevant future CLs where
> they are used first.

Done.

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/public/bro...
File content/public/browser/speech_recognition_session_config.h (right):

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/public/bro...
content/public/browser/speech_recognition_session_config.h:30: bool continuous;
On 2012/04/27 10:04:41, Satish wrote:
> this name is very generic. perhaps add a comment here that this indicates
> continuous recognition that doesn't stop automatically at silence
Renamed to is_one_shot.

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/public/bro...
File content/public/browser/speech_recognition_session_context.h (right):

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/public/bro...
content/public/browser/speech_recognition_session_context.h:34: int
js_handle_id;
On 2012/04/27 10:04:41, Satish wrote:
> typically 'handle' and 'id' are used mutually exclusive. What is the purpose
of
> this field?
It is the numeric id that is (will be) mapped to the webkit object
WebSpeechReecognitionHandle.

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/public/com...
File content/public/common/speech_recognition_grammar.h (right):

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/public/com...
content/public/common/speech_recognition_grammar.h:20:
SpeechRecognitionGrammar()
On 2012/04/27 10:04:41, Satish wrote:
> instead of inlining these move them to a .cc file
See jam@ reply.

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/public/com...
File content/public/common/speech_recognition_result.h (right):

https://chromiumcodereview.appspot.com/10233010/diff/15001/content/public/com...
content/public/common/speech_recognition_result.h:21:
SpeechRecognitionHypothesis() : confidence(0.0) {}
On 2012/04/27 10:04:41, Satish wrote:
> move constructor definitions to .cc file
See jam@ reply.

[jam, 2012-04-27 16:00:57]

On 2012/04/27 14:45:34, Satish wrote:

> Cool, makes sense. Is it ok to mention this in the style guide as the one
> exception to the inlining rule?

sure

[jam, 2012-04-27 16:03:04]

content/public lgtm with one nit

http://codereview.chromium.org/10233010/diff/25004/content/public/common/spee...
File content/public/common/speech_recognition_grammar.h (right):

http://codereview.chromium.org/10233010/diff/25004/content/public/common/spee...
content/public/common/speech_recognition_grammar.h:17: std::string url;
nit: i dont usually see constructors going after parameters, even for structs

[Primiano Tucci (use gerrit), 2012-04-27 16:17:54]

http://codereview.chromium.org/10233010/diff/25004/content/public/common/spee...
File content/public/common/speech_recognition_grammar.h (right):

http://codereview.chromium.org/10233010/diff/25004/content/public/common/spee...
content/public/common/speech_recognition_grammar.h:17: std::string url;
On 2012/04/27 16:03:04, John Abd-El-Malek wrote:
> nit: i dont usually see constructors going after parameters, even for structs

Done.

[hans, 2012-05-04 14:48:13]

What's the status of this one? Blocked on other CLs? Waiting for Satish?

[Primiano Tucci (use gerrit), 2012-05-04 15:40:46]

On 2012/05/04 14:48:13, hans wrote:
> What's the status of this one? Blocked on other CLs? Waiting for Satish?

Both. Mainly I prefer to commit in order, even if they are disjoint.

[Satish, 2012-05-15 10:52:22]

lgtm

[commit-bot: I haz the power, 2012-05-15 11:02:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/primiano@chromium.org/10233010/41003

[commit-bot: I haz the power, 2012-05-15 11:02:46]

Can't apply patch for file
content/public/browser/speech_recognition_event_listener.h.
While running patch -p1 --forward --force;
patching file content/public/browser/speech_recognition_event_listener.h
Hunk #1 FAILED at 4.
Hunk #2 succeeded at 31 (offset 1 line).
1 out of 2 hunks FAILED -- saving rejects to file
content/public/browser/speech_recognition_event_listener.h.rej

[commit-bot: I haz the power, 2012-05-15 11:19:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/primiano@chromium.org/10233010/51003

[commit-bot: I haz the power, 2012-05-15 12:06:20]

Try job failure for 10233010-51003 (previous was lost) (retry) on linux_chromeos
for step "compile" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-05-15 12:58:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/primiano@chromium.org/10233010/55010

[commit-bot: I haz the power, 2012-05-15 15:45:25]

Change committed as 137141