Introduced SpeechRecognitionDispatcher(Host) classes, handling dispatch of IPC messages for continu…

Introduced SpeechRecognitionDispatcher(Host) classes, handling dispatch of IPC messages for continuous speech recognition. (Speech CL2.1)


BUG=116954
TEST=none


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

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=139015

[hans, 2012-05-11 16:56:32]

http://codereview.chromium.org/10273006/diff/2001/content/browser/speech/spee...
File content/browser/speech/speech_recognition_dispatcher_host.cc (right):

http://codereview.chromium.org/10273006/diff/2001/content/browser/speech/spee...
content/browser/speech/speech_recognition_dispatcher_host.cc:22: bool
IsSameContext(int render_process_id,
i would personally move this function closer to where it's called

http://codereview.chromium.org/10273006/diff/2001/content/browser/speech/spee...
content/browser/speech/speech_recognition_dispatcher_host.cc:56: // We first
check if this dispatcher received any speech IPC requst so that
s/requst/request/

http://codereview.chromium.org/10273006/diff/2001/content/browser/speech/spee...
content/browser/speech/speech_recognition_dispatcher_host.cc:59: if
(may_have_pending_requests_)
couldn't we just do "if (manager_) ..." ? That would let us get rid of
may_have_pending_requests_, and give us one less thing to keep track of.

http://codereview.chromium.org/10273006/diff/2001/content/browser/speech/spee...
content/browser/speech/speech_recognition_dispatcher_host.cc:94: const
SpeechRecognitionHostMsg_StartRequest_Params &params) {
the & is misplaced

http://codereview.chromium.org/10273006/diff/2001/content/browser/speech/spee...
content/browser/speech/speech_recognition_dispatcher_host.cc:113: if (session_id
== content::SpeechRecognitionManager::kSessionIDInvalid)
when does this happen?

http://codereview.chromium.org/10273006/diff/2001/content/browser/speech/spee...
content/browser/speech/speech_recognition_dispatcher_host.cc:121:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
indent +1

http://codereview.chromium.org/10273006/diff/2001/content/browser/speech/spee...
content/browser/speech/speech_recognition_dispatcher_host.cc:127: if (session_id
!= 0)
should 0 be kSessionIDInvalid?

http://codereview.chromium.org/10273006/diff/2001/content/browser/speech/spee...
File content/browser/speech/speech_recognition_dispatcher_host.h (right):

http://codereview.chromium.org/10273006/diff/2001/content/browser/speech/spee...
content/browser/speech/speech_recognition_dispatcher_host.h:65: const
SpeechRecognitionHostMsg_StartRequest_Params &params);
the & should be on the type

http://codereview.chromium.org/10273006/diff/2001/content/browser/speech/spee...
content/browser/speech/speech_recognition_dispatcher_host.h:69: // Returns the
speech recognition manager to forward events to, creating one
hmm, maybe change the comment to say "requests" rather than events?

I mean, we forard the start/stop/abort requests to the manager, and the events
that we get from the manager are forwarded the other way

http://codereview.chromium.org/10273006/diff/2001/content/browser/speech/spee...
content/browser/speech/speech_recognition_dispatcher_host.h:74: bool
may_have_pending_requests_;  // Set if we received any speech IPC request
nit: period.

http://codereview.chromium.org/10273006/diff/2001/content/common/speech_recog...
File content/common/speech_recognition_messages.h (right):

http://codereview.chromium.org/10273006/diff/2001/content/common/speech_recog...
content/common/speech_recognition_messages.h:108: // Unique ID associated to the
JS object making the calls.
s/to/with/

http://codereview.chromium.org/10273006/diff/2001/content/common/speech_recog...
content/common/speech_recognition_messages.h:120: // Render -> Browser messages.
s/Render/Renderer/

http://codereview.chromium.org/10273006/diff/2001/content/common/speech_recog...
content/common/speech_recognition_messages.h:123: // of the given
|render_view_id|.
maybe skip the "on behalf of the given |render_view_id|." part, since that's
actually baked into the Params struct?

http://codereview.chromium.org/10273006/diff/2001/content/common/speech_recog...
content/common/speech_recognition_messages.h:142: // Browser -> Render messages.
renderer

http://codereview.chromium.org/10273006/diff/2001/content/renderer/render_vie...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/10273006/diff/2001/content/renderer/render_vie...
content/renderer/render_view_impl.h:1222: // The speech dispatcher attached to
this view, lazily initialized.
can we call it "The speech recognition dispatcher" instead?

http://codereview.chromium.org/10273006/diff/2001/content/renderer/speech_rec...
File content/renderer/speech_recognition_dispatcher.cc (right):

http://codereview.chromium.org/10273006/diff/2001/content/renderer/speech_rec...
content/renderer/speech_recognition_dispatcher.cc:66: for (size_t i=0; i <
params.grammars().size(); ++i) {
spaces around the =

http://codereview.chromium.org/10273006/diff/2001/content/renderer/speech_rec...
content/renderer/speech_recognition_dispatcher.cc:74: msg_params.origin_url =
"";  //TODO (primiano) we need an origin from WebKit.
i don't think we normally put space between TODO and (username)

http://codereview.chromium.org/10273006/diff/2001/content/renderer/speech_rec...
content/renderer/speech_recognition_dispatcher.cc:95: if (!event_proxy_)
can this happen?

http://codereview.chromium.org/10273006/diff/2001/content/renderer/speech_rec...
content/renderer/speech_recognition_dispatcher.cc:133: WebString(), //
TODO(primiano) error message?
hmm, i guess we should look at the error.details and grab a translated string
from somewhere, but we can do that in another patch.

http://codereview.chromium.org/10273006/diff/2001/content/renderer/speech_rec...
content/renderer/speech_recognition_dispatcher.cc:154: for (size_t i=0; i <
num_hypotheses; ++i) {
spaces around =

http://codereview.chromium.org/10273006/diff/2001/content/renderer/speech_rec...
content/renderer/speech_recognition_dispatcher.cc:163: 0, // result_index
maybe do "const int result_index = 0;" and pass in the variable instead?

http://codereview.chromium.org/10273006/diff/2001/content/renderer/speech_rec...
content/renderer/speech_recognition_dispatcher.cc:170: for(HandleMap::iterator
iter = handle_mappings_.begin();
space between for and (

http://codereview.chromium.org/10273006/diff/2001/content/renderer/speech_rec...
content/renderer/speech_recognition_dispatcher.cc:179: ++last_mapping_id_) {}
why do we need this loop? why not just

handle_map_[++last_mapping_id_] = handle; ?

http://codereview.chromium.org/10273006/diff/2001/content/renderer/speech_rec...
File content/renderer/speech_recognition_dispatcher.h (right):

http://codereview.chromium.org/10273006/diff/2001/content/renderer/speech_rec...
content/renderer/speech_recognition_dispatcher.h:6: #define
CONTENT_RENDERER_SPEECH_RECOGNITION_DISPATCHER_H_
#pragma once :)

http://codereview.chromium.org/10273006/diff/2001/content/renderer/speech_rec...
content/renderer/speech_recognition_dispatcher.h:60: int GetIDForHandle(const
WebKit::WebSpeechRecognitionHandle& handle);
i'd put a blank before this to separate from the On* functions

http://codereview.chromium.org/10273006/diff/2001/content/renderer/speech_rec...
content/renderer/speech_recognition_dispatcher.h:63:
WebKit::WebSpeechRecognizerClient* event_proxy_;
hmm, i'd just call it recognizer_client_, and maybe add a comment that this is
the client that we'll send events back to,or something like that

http://codereview.chromium.org/10273006/diff/2001/content/renderer/speech_rec...
content/renderer/speech_recognition_dispatcher.h:65: HandleMap handle_mappings_;
or just handle_map_

[Primiano Tucci (use gerrit), 2012-05-14 12:58:22]

http://codereview.chromium.org/10273006/diff/2001/content/browser/speech/spee...
File content/browser/speech/speech_recognition_dispatcher_host.cc (right):

http://codereview.chromium.org/10273006/diff/2001/content/browser/speech/spee...
content/browser/speech/speech_recognition_dispatcher_host.cc:22: bool
IsSameContext(int render_process_id,
On 2012/05/11 16:56:32, hans wrote:
> i would personally move this function closer to where it's called

Done.

http://codereview.chromium.org/10273006/diff/2001/content/browser/speech/spee...
content/browser/speech/speech_recognition_dispatcher_host.cc:56: // We first
check if this dispatcher received any speech IPC requst so that
On 2012/05/11 16:56:32, hans wrote:
> s/requst/request/

Done.

http://codereview.chromium.org/10273006/diff/2001/content/browser/speech/spee...
content/browser/speech/speech_recognition_dispatcher_host.cc:59: if
(may_have_pending_requests_)
On 2012/05/11 16:56:32, hans wrote:
> couldn't we just do "if (manager_) ..." ? That would let us get rid of
> may_have_pending_requests_, and give us one less thing to keep track of.
2 Observations:
1) Depending on CL1.10 it might be not needed anymore, added TODO.
2) I did copy/paste the former input_tag_speech_dispatcher code. However now
that I look at it thoroughly, I realize that the true name for manager_ field
should be "manager_for_tests_", which explains the need of the
may_have_pending_requests_.

http://codereview.chromium.org/10273006/diff/2001/content/browser/speech/spee...
content/browser/speech/speech_recognition_dispatcher_host.cc:94: const
SpeechRecognitionHostMsg_StartRequest_Params &params) {
On 2012/05/11 16:56:32, hans wrote:
> the & is misplaced
I see that is a copy/paste "heritage". Fixed also in
input_tag_speech_dispatcher_host.

http://codereview.chromium.org/10273006/diff/2001/content/browser/speech/spee...
content/browser/speech/speech_recognition_dispatcher_host.cc:113: if (session_id
== content::SpeechRecognitionManager::kSessionIDInvalid)
On 2012/05/11 16:56:32, hans wrote:
> when does this happen?
Hmm I remember that I turned this into a DCHECK in past CLs, but apparently it
is not so. Changed here and into input_tag_speech_dispatcher_host.

http://codereview.chromium.org/10273006/diff/2001/content/browser/speech/spee...
content/browser/speech/speech_recognition_dispatcher_host.cc:121:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
On 2012/05/11 16:56:32, hans wrote:
> indent +1

Done.

http://codereview.chromium.org/10273006/diff/2001/content/browser/speech/spee...
content/browser/speech/speech_recognition_dispatcher_host.cc:127: if (session_id
!= 0)
On 2012/05/11 16:56:32, hans wrote:
> should 0 be kSessionIDInvalid?

Right.

http://codereview.chromium.org/10273006/diff/2001/content/browser/speech/spee...
File content/browser/speech/speech_recognition_dispatcher_host.h (right):

http://codereview.chromium.org/10273006/diff/2001/content/browser/speech/spee...
content/browser/speech/speech_recognition_dispatcher_host.h:65: const
SpeechRecognitionHostMsg_StartRequest_Params &params);
On 2012/05/11 16:56:32, hans wrote:
> the & should be on the type
Oops. Done.

http://codereview.chromium.org/10273006/diff/2001/content/browser/speech/spee...
content/browser/speech/speech_recognition_dispatcher_host.h:69: // Returns the
speech recognition manager to forward events to, creating one
On 2012/05/11 16:56:32, hans wrote:
> hmm, maybe change the comment to say "requests" rather than events?
> 
> I mean, we forard the start/stop/abort requests to the manager, and the events
> that we get from the manager are forwarded the other way
Right. Furthermore I removed the "creating one if needed" since, due to CL1.8,
it might turn out to be not true anymore. Even in the current implementation, it
just proxies to the singleton GetInstance method, and does not directly create
an istance.

http://codereview.chromium.org/10273006/diff/2001/content/browser/speech/spee...
content/browser/speech/speech_recognition_dispatcher_host.h:74: bool
may_have_pending_requests_;  // Set if we received any speech IPC request
On 2012/05/11 16:56:32, hans wrote:
> nit: period.

Done.

http://codereview.chromium.org/10273006/diff/2001/content/common/speech_recog...
File content/common/speech_recognition_messages.h (right):

http://codereview.chromium.org/10273006/diff/2001/content/common/speech_recog...
content/common/speech_recognition_messages.h:108: // Unique ID associated to the
JS object making the calls.
On 2012/05/11 16:56:32, hans wrote:
> s/to/with/

Done.

http://codereview.chromium.org/10273006/diff/2001/content/common/speech_recog...
content/common/speech_recognition_messages.h:120: // Render -> Browser messages.
On 2012/05/11 16:56:32, hans wrote:
> s/Render/Renderer/

Done.

http://codereview.chromium.org/10273006/diff/2001/content/common/speech_recog...
content/common/speech_recognition_messages.h:123: // of the given
|render_view_id|.
On 2012/05/11 16:56:32, hans wrote:
> maybe skip the "on behalf of the given |render_view_id|." part, since that's
> actually baked into the Params struct?
Right.

http://codereview.chromium.org/10273006/diff/2001/content/common/speech_recog...
content/common/speech_recognition_messages.h:142: // Browser -> Render messages.
On 2012/05/11 16:56:32, hans wrote:
> renderer

Done.

http://codereview.chromium.org/10273006/diff/2001/content/renderer/render_vie...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/10273006/diff/2001/content/renderer/render_vie...
content/renderer/render_view_impl.h:1222: // The speech dispatcher attached to
this view, lazily initialized.
On 2012/05/11 16:56:32, hans wrote:
> can we call it "The speech recognition dispatcher" instead?

Done.

http://codereview.chromium.org/10273006/diff/2001/content/renderer/speech_rec...
File content/renderer/speech_recognition_dispatcher.cc (right):

http://codereview.chromium.org/10273006/diff/2001/content/renderer/speech_rec...
content/renderer/speech_recognition_dispatcher.cc:66: for (size_t i=0; i <
params.grammars().size(); ++i) {
On 2012/05/11 16:56:32, hans wrote:
> spaces around the =

Done.

http://codereview.chromium.org/10273006/diff/2001/content/renderer/speech_rec...
content/renderer/speech_recognition_dispatcher.cc:74: msg_params.origin_url =
"";  //TODO (primiano) we need an origin from WebKit.
On 2012/05/11 16:56:32, hans wrote:
> i don't think we normally put space between TODO and (username)
Uh, right. The space was misplaced.

http://codereview.chromium.org/10273006/diff/2001/content/renderer/speech_rec...
content/renderer/speech_recognition_dispatcher.cc:95: if (!event_proxy_)
On 2012/05/11 16:56:32, hans wrote:
> can this happen?
Hmm, agree. A DCHECK is more suitable.

http://codereview.chromium.org/10273006/diff/2001/content/renderer/speech_rec...
content/renderer/speech_recognition_dispatcher.cc:133: WebString(), //
TODO(primiano) error message?
On 2012/05/11 16:56:32, hans wrote:
> hmm, i guess we should look at the error.details and grab a translated string
> from somewhere, but we can do that in another patch.
Right.

http://codereview.chromium.org/10273006/diff/2001/content/renderer/speech_rec...
content/renderer/speech_recognition_dispatcher.cc:154: for (size_t i=0; i <
num_hypotheses; ++i) {
On 2012/05/11 16:56:32, hans wrote:
> spaces around =

Done.

http://codereview.chromium.org/10273006/diff/2001/content/renderer/speech_rec...
content/renderer/speech_recognition_dispatcher.cc:163: 0, // result_index
On 2012/05/11 16:56:32, hans wrote:
> maybe do "const int result_index = 0;" and pass in the variable instead?

I really agree with the pattern, but in a similar occasion in a previous review
was invited to avoid it
(http://codereview.chromium.org/9835049/diff/3001/content/browser/speech/speec...)

http://codereview.chromium.org/10273006/diff/2001/content/renderer/speech_rec...
content/renderer/speech_recognition_dispatcher.cc:170: for(HandleMap::iterator
iter = handle_mappings_.begin();
On 2012/05/11 16:56:32, hans wrote:
> space between for and (

Done.

http://codereview.chromium.org/10273006/diff/2001/content/renderer/speech_rec...
content/renderer/speech_recognition_dispatcher.cc:179: ++last_mapping_id_) {}
On 2012/05/11 16:56:32, hans wrote:
> why do we need this loop? why not just
> 
> handle_map_[++last_mapping_id_] = handle; ?
Just me being extremely paranoid about holes after wrapping the count.
Dropped it but now expecting less than 2^31 recognitions during the browser
lifetime. :)

http://codereview.chromium.org/10273006/diff/2001/content/renderer/speech_rec...
File content/renderer/speech_recognition_dispatcher.h (right):

http://codereview.chromium.org/10273006/diff/2001/content/renderer/speech_rec...
content/renderer/speech_recognition_dispatcher.h:6: #define
CONTENT_RENDERER_SPEECH_RECOGNITION_DISPATCHER_H_
On 2012/05/11 16:56:32, hans wrote:
> #pragma once :)
Again!

http://codereview.chromium.org/10273006/diff/2001/content/renderer/speech_rec...
content/renderer/speech_recognition_dispatcher.h:60: int GetIDForHandle(const
WebKit::WebSpeechRecognitionHandle& handle);
On 2012/05/11 16:56:32, hans wrote:
> i'd put a blank before this to separate from the On* functions
Right!

http://codereview.chromium.org/10273006/diff/2001/content/renderer/speech_rec...
content/renderer/speech_recognition_dispatcher.h:63:
WebKit::WebSpeechRecognizerClient* event_proxy_;
Right and it fits better the ClassName -> instance_name pattern.

http://codereview.chromium.org/10273006/diff/2001/content/renderer/speech_rec...
content/renderer/speech_recognition_dispatcher.h:65: HandleMap handle_mappings_;
On 2012/05/11 16:56:32, hans wrote:
> or just handle_map_

Done.

[hans, 2012-05-14 14:04:52]

LGTM

http://codereview.chromium.org/10273006/diff/2001/content/browser/speech/spee...
File content/browser/speech/speech_recognition_dispatcher_host.cc (right):

http://codereview.chromium.org/10273006/diff/2001/content/browser/speech/spee...
content/browser/speech/speech_recognition_dispatcher_host.cc:59: if
(may_have_pending_requests_)
On 2012/05/14 12:58:22, Primiano Tucci wrote:
> On 2012/05/11 16:56:32, hans wrote:
> > couldn't we just do "if (manager_) ..." ? That would let us get rid of
> > may_have_pending_requests_, and give us one less thing to keep track of.
> 2 Observations:
> 1) Depending on CL1.10 it might be not needed anymore, added TODO.
> 2) I did copy/paste the former input_tag_speech_dispatcher code. However now
> that I look at it thoroughly, I realize that the true name for manager_ field
> should be "manager_for_tests_", which explains the need of the
> may_have_pending_requests_.

Ah, thanks for clarifying.

http://codereview.chromium.org/10273006/diff/2001/content/renderer/speech_rec...
File content/renderer/speech_recognition_dispatcher.cc (right):

http://codereview.chromium.org/10273006/diff/2001/content/renderer/speech_rec...
content/renderer/speech_recognition_dispatcher.cc:163: 0, // result_index
On 2012/05/14 12:58:22, Primiano Tucci wrote:
> On 2012/05/11 16:56:32, hans wrote:
> > maybe do "const int result_index = 0;" and pass in the variable instead?
> 
> I really agree with the pattern, but in a similar occasion in a previous
review
> was invited to avoid it
>
(http://codereview.chromium.org/9835049/diff/3001/content/browser/speech/speec...)

OK. Satish can comment if he has opinions here :)

http://codereview.chromium.org/10273006/diff/16001/content/browser/speech/spe...
File content/browser/speech/speech_recognition_dispatcher_host.cc (right):

http://codereview.chromium.org/10273006/diff/16001/content/browser/speech/spe...
content/browser/speech/speech_recognition_dispatcher_host.cc:38: // This is
initialized by Browser. Do not add any non-trivial
not sure if "by Browser" makes sense.. everything in this class is done in the
browser process. Maybe "this is run during browser startup" or something like
that.

I'm not sure a motivation is even strictly necessary... "Do not add any
non-trivial initialization here" is probably good enough.

http://codereview.chromium.org/10273006/diff/16001/content/renderer/speech_re...
File content/renderer/speech_recognition_dispatcher.h (right):

http://codereview.chromium.org/10273006/diff/16001/content/renderer/speech_re...
content/renderer/speech_recognition_dispatcher.h:67: typedef std::map<int,
WebKit::WebSpeechRecognitionHandle> HandleMap;
maybe a blank before this line to separate the "handle stuff" from the
recognizer_client_

[Primiano Tucci (use gerrit), 2012-05-16 18:22:16]

Ok, I also made some small modifications:
- removed may_have_pending_requests_ from both input_tag_dispatcher_host and
speech_recognition_dispatcher_host since, due to CL1.10, the manager is created
anyway.

- Added some (temporary) to translate speech recognition error codes to
WebSpeechRecognitionClient::ErrorCode after the last WebKit patch
(https://bugs.webkit.org/show_bug.cgi?id=86353)
On 2012/05/14 14:04:52, hans wrote:
> LGTM
> 
>
http://codereview.chromium.org/10273006/diff/2001/content/browser/speech/spee...
> File content/browser/speech/speech_recognition_dispatcher_host.cc (right):
> 
>
http://codereview.chromium.org/10273006/diff/2001/content/browser/speech/spee...
> content/browser/speech/speech_recognition_dispatcher_host.cc:59: if
> (may_have_pending_requests_)
> On 2012/05/14 12:58:22, Primiano Tucci wrote:
> > On 2012/05/11 16:56:32, hans wrote:
> > > couldn't we just do "if (manager_) ..." ? That would let us get rid of
> > > may_have_pending_requests_, and give us one less thing to keep track of.
> > 2 Observations:
> > 1) Depending on CL1.10 it might be not needed anymore, added TODO.
> > 2) I did copy/paste the former input_tag_speech_dispatcher code. However now
> > that I look at it thoroughly, I realize that the true name for manager_
field
> > should be "manager_for_tests_", which explains the need of the
> > may_have_pending_requests_.
> 
> Ah, thanks for clarifying.
> 
>
http://codereview.chromium.org/10273006/diff/2001/content/renderer/speech_rec...
> File content/renderer/speech_recognition_dispatcher.cc (right):
> 
>
http://codereview.chromium.org/10273006/diff/2001/content/renderer/speech_rec...
> content/renderer/speech_recognition_dispatcher.cc:163: 0, // result_index
> On 2012/05/14 12:58:22, Primiano Tucci wrote:
> > On 2012/05/11 16:56:32, hans wrote:
> > > maybe do "const int result_index = 0;" and pass in the variable instead?
> > 
> > I really agree with the pattern, but in a similar occasion in a previous
> review
> > was invited to avoid it
> >
>
(http://codereview.chromium.org/9835049/diff/3001/content/browser/speech/speec...)
> 
> OK. Satish can comment if he has opinions here :)
> 
>
http://codereview.chromium.org/10273006/diff/16001/content/browser/speech/spe...
> File content/browser/speech/speech_recognition_dispatcher_host.cc (right):
> 
>
http://codereview.chromium.org/10273006/diff/16001/content/browser/speech/spe...
> content/browser/speech/speech_recognition_dispatcher_host.cc:38: // This is
> initialized by Browser. Do not add any non-trivial
> not sure if "by Browser" makes sense.. everything in this class is done in the
> browser process. Maybe "this is run during browser startup" or something like
> that.
> 
> I'm not sure a motivation is even strictly necessary... "Do not add any
> non-trivial initialization here" is probably good enough.
> 
>
http://codereview.chromium.org/10273006/diff/16001/content/renderer/speech_re...
> File content/renderer/speech_recognition_dispatcher.h (right):
> 
>
http://codereview.chromium.org/10273006/diff/16001/content/renderer/speech_re...
> content/renderer/speech_recognition_dispatcher.h:67: typedef std::map<int,
> WebKit::WebSpeechRecognitionHandle> HandleMap;
> maybe a blank before this line to separate the "handle stuff" from the
> recognizer_client_

[Primiano Tucci (use gerrit), 2012-05-17 10:43:40]

Fixed ENABLE_SPEECH_INPUT define guards.

[Primiano Tucci (use gerrit), 2012-05-17 13:51:20]

+avi@ for OWNERS approval on content/*gyp*, content/renderer and content/common/

[Satish, 2012-05-20 22:57:45]

https://chromiumcodereview.appspot.com/10273006/diff/25004/chrome/browser/ext...
File chrome/browser/extensions/extension_function_registry.cc (right):

https://chromiumcodereview.appspot.com/10273006/diff/25004/chrome/browser/ext...
chrome/browser/extensions/extension_function_registry.cc:248: #if
defined(ENABLE_INPUT_SPEECH)
the TTS extension api doesn't use the same ENABLE define so this guard should be
removed

https://chromiumcodereview.appspot.com/10273006/diff/25004/content/browser/re...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10273006/diff/25004/content/browser/re...
content/browser/renderer_host/render_process_host_impl.cc:506: GetID(),
could merge this and next line as the style guide doesn't require multiple
parameters in the call site to be split across lines

https://chromiumcodereview.appspot.com/10273006/diff/25004/content/browser/sp...
File content/browser/speech/speech_recognition_dispatcher_host.cc (right):

https://chromiumcodereview.appspot.com/10273006/diff/25004/content/browser/sp...
content/browser/speech/speech_recognition_dispatcher_host.cc:81:
context.js_handle_id = params.js_handle_id;
hmm I see my comment in the header file about the name 'js_handle_id' applies to
SpeechRecognitionSessionContext as well

https://chromiumcodereview.appspot.com/10273006/diff/25004/content/browser/sp...
content/browser/speech/speech_recognition_dispatcher_host.cc:98: namespace {
It is customary to have the anonymous namespace at top of file

https://chromiumcodereview.appspot.com/10273006/diff/25004/content/browser/sp...
content/browser/speech/speech_recognition_dispatcher_host.cc:112: int session_id
= manager()->LookupSessionByContext(
In an earlier CL we discussed about replacing the LookupSessionByContext with a
direct lookup function that takes in the 3 parameters instead of a closure. I
see there is a TODO above "SpeechRecognitionManagerImpl::LookupSessionByContext"
method about this. Since now it is clear that both InputTagSpeechDispatcherHost
and SpeechRecognitionDispatcherHost are having the same 3 parameters to lookup
sessions, can you address the todo and simplify this?

https://chromiumcodereview.appspot.com/10273006/diff/25004/content/browser/sp...
File content/browser/speech/speech_recognition_dispatcher_host.h (right):

https://chromiumcodereview.appspot.com/10273006/diff/25004/content/browser/sp...
content/browser/speech/speech_recognition_dispatcher_host.h:65: void
OnAbortRequest(int render_view_id, int js_handle_id);
I'd suggest replacing 'js_handle_id' with something opaque and not mentioning JS
(which is why we chose 'request_id' in the earlier code). Also, typically both
'handle' and 'id' are used interchangeably for an opaque value passed through an
API so it is somewhat funny to see both in the same name :)

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

https://chromiumcodereview.appspot.com/10273006/diff/25004/content/common/spe...
content/common/speech_recognition_messages.h:120: // Renderer -> Browser
messages.
move this to line 102 so the above struct is read in context of
renderer->browser messages

https://chromiumcodereview.appspot.com/10273006/diff/25004/content/renderer/s...
File content/renderer/speech_recognition_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10273006/diff/25004/content/renderer/s...
content/renderer/speech_recognition_dispatcher.cc:174:
webkit_result.assign(transcripts, confidences, !result.provisional);
Not related to this CL, but passing the transcript and confidence values as
separate vectors doesn't seem right. What is the reason for the WebKit API to
not accept an array of types such as WebSpeechRecognitionHypothesis just like
'result.hypotheses[i]' above?

https://chromiumcodereview.appspot.com/10273006/diff/25004/content/renderer/s...
File content/renderer/speech_recognition_dispatcher.h (right):

https://chromiumcodereview.appspot.com/10273006/diff/25004/content/renderer/s...
content/renderer/speech_recognition_dispatcher.h:29: //
SpeechRecognitionDispatcher is a delegate for messages used by WebKit for
messages -> methods or calls

https://chromiumcodereview.appspot.com/10273006/diff/25004/content/renderer/s...
content/renderer/speech_recognition_dispatcher.h:70: int last_mapping_id_;
suggest renaming to 'next_id_' or something similar and initialize from 1, as
the name 'mapping_id' isn't fitting well here

[hans, 2012-05-21 09:08:41]

https://chromiumcodereview.appspot.com/10273006/diff/25004/content/browser/sp...
File content/browser/speech/speech_recognition_dispatcher_host.cc (right):

https://chromiumcodereview.appspot.com/10273006/diff/25004/content/browser/sp...
content/browser/speech/speech_recognition_dispatcher_host.cc:98: namespace {
On 2012/05/20 22:57:46, Satish wrote:
> It is customary to have the anonymous namespace at top of file

I asked for the function to be moved here so it would be close to where it's
used.

Let's declare it "static" instead of using an anonymous namespace.

[Primiano Tucci (use gerrit), 2012-05-21 10:33:13]

>
https://chromiumcodereview.appspot.com/10273006/diff/25004/chrome/browser/ext...
> chrome/browser/extensions/extension_function_registry.cc:248: #if
> defined(ENABLE_INPUT_SPEECH)
> the TTS extension api doesn't use the same ENABLE define so this guard should
be
> removed
 
+dmazzoni@ regarding this: deassertion of ENABLE_INPUT_SPEECH flag causes, in
the chrome_browser.gypi file, the exclusion of /chrome/browser/speech folder,
which contains also TTS classes.
The current build does not compile if the ENABLE_INPUT_SPEECH flag is turned off
(i.e. setting 'input_speech%': 0 in build/common.gypi)
Is it ok to guard TTS extensions like I did in this CL? Or should we change the
chrome_browser.gypi rules?
Thanks

[Primiano Tucci (use gerrit), 2012-05-21 10:33:55]

https://chromiumcodereview.appspot.com/10273006/diff/25004/chrome/browser/ext...
File chrome/browser/extensions/extension_function_registry.cc (right):

https://chromiumcodereview.appspot.com/10273006/diff/25004/chrome/browser/ext...
chrome/browser/extensions/extension_function_registry.cc:248: #if
defined(ENABLE_INPUT_SPEECH)
On 2012/05/20 22:57:46, Satish wrote:
> the TTS extension api doesn't use the same ENABLE define so this guard should
be
> removed
+dmazzoni@ regarding this: deassertion of ENABLE_INPUT_SPEECH flag causes, in
the chrome_browser.gypi file, the exclusion of /chrome/browser/speech folder,
which contains also TTS classes.
The current build does not compile if the ENABLE_INPUT_SPEECH flag is turned off
(i.e. setting 'input_speech%': 0 in build/common.gypi)
Is it ok to guard TTS extensions like I did in this CL? Or should we change the
chrome_browser.gypi rules?

[Primiano Tucci (use gerrit), 2012-05-21 13:38:24]

https://chromiumcodereview.appspot.com/10273006/diff/25004/content/browser/re...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10273006/diff/25004/content/browser/re...
content/browser/renderer_host/render_process_host_impl.cc:506: GetID(),
On 2012/05/20 22:57:46, Satish wrote:
> could merge this and next line as the style guide doesn't require multiple
> parameters in the call site to be split across lines

Done.

https://chromiumcodereview.appspot.com/10273006/diff/25004/content/browser/sp...
File content/browser/speech/speech_recognition_dispatcher_host.cc (right):

https://chromiumcodereview.appspot.com/10273006/diff/25004/content/browser/sp...
content/browser/speech/speech_recognition_dispatcher_host.cc:81:
context.js_handle_id = params.js_handle_id;
On 2012/05/20 22:57:46, Satish wrote:
> hmm I see my comment in the header file about the name 'js_handle_id' applies
to
> SpeechRecognitionSessionContext as well
Ok, renamed here and everywhere else to request_id, so it is the same for both
x-webkit-speech and JS api, allowing to use a unified LookupSessionByContext
function.

https://chromiumcodereview.appspot.com/10273006/diff/25004/content/browser/sp...
content/browser/speech/speech_recognition_dispatcher_host.cc:98: namespace {
On 2012/05/21 09:08:41, hans wrote:
> On 2012/05/20 22:57:46, Satish wrote:
> > It is customary to have the anonymous namespace at top of file
> 
> I asked for the function to be moved here so it would be close to where it's
> used.
> 
> Let's declare it "static" instead of using an anonymous namespace.

Done.

https://chromiumcodereview.appspot.com/10273006/diff/25004/content/browser/sp...
content/browser/speech/speech_recognition_dispatcher_host.cc:112: int session_id
= manager()->LookupSessionByContext(
On 2012/05/20 22:57:46, Satish wrote:
> In an earlier CL we discussed about replacing the LookupSessionByContext with
a
> direct lookup function that takes in the 3 parameters instead of a closure. I
> see there is a TODO above
"SpeechRecognitionManagerImpl::LookupSessionByContext"
> method about this. Since now it is clear that both
InputTagSpeechDispatcherHost
> and SpeechRecognitionDispatcherHost are having the same 3 parameters to lookup
> sessions, can you address the todo and simplify this?

Done.

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

https://chromiumcodereview.appspot.com/10273006/diff/25004/content/common/spe...
content/common/speech_recognition_messages.h:120: // Renderer -> Browser
messages.
On 2012/05/20 22:57:46, Satish wrote:
> move this to line 102 so the above struct is read in context of
> renderer->browser messages

Done.

https://chromiumcodereview.appspot.com/10273006/diff/25004/content/renderer/s...
File content/renderer/speech_recognition_dispatcher.h (right):

https://chromiumcodereview.appspot.com/10273006/diff/25004/content/renderer/s...
content/renderer/speech_recognition_dispatcher.h:29: //
SpeechRecognitionDispatcher is a delegate for messages used by WebKit for
On 2012/05/20 22:57:46, Satish wrote:
> messages -> methods or calls

Done.

https://chromiumcodereview.appspot.com/10273006/diff/25004/content/renderer/s...
content/renderer/speech_recognition_dispatcher.h:70: int last_mapping_id_;
On 2012/05/20 22:57:46, Satish wrote:
> suggest renaming to 'next_id_' or something similar and initialize from 1, as
> the name 'mapping_id' isn't fitting well here

Done.

[Satish, 2012-05-21 15:18:58]

lgtm

https://chromiumcodereview.appspot.com/10273006/diff/40001/content/browser/sp...
File content/browser/speech/speech_recognition_manager_impl.h (right):

https://chromiumcodereview.appspot.com/10273006/diff/40001/content/browser/sp...
content/browser/speech/speech_recognition_manager_impl.h:70: virtual int
LookupSessionByContext(int render_process_id,
suggest renaming to 'GetSession' or 'GetSessionForRequest' etc. as there is no
context being passed around here.

[Primiano Tucci (use gerrit), 2012-05-21 16:37:12]

https://chromiumcodereview.appspot.com/10273006/diff/40001/content/browser/sp...
File content/browser/speech/speech_recognition_manager_impl.h (right):

https://chromiumcodereview.appspot.com/10273006/diff/40001/content/browser/sp...
content/browser/speech/speech_recognition_manager_impl.h:70: virtual int
LookupSessionByContext(int render_process_id,
On 2012/05/21 15:18:58, Satish wrote:
> suggest renaming to 'GetSession' or 'GetSessionForRequest' etc. as there is no
> context being passed around here.

Done.

[Primiano Tucci (use gerrit), 2012-05-23 14:27:19]

+jam@ for OWNERS approval on content/*gyp*, content/renderer and content/common/

[jam, 2012-05-23 15:41:24]

lgtm with nits

http://codereview.chromium.org/10273006/diff/21055/content/browser/speech/inp...
File content/browser/speech/input_tag_speech_dispatcher_host.cc (right):

http://codereview.chromium.org/10273006/diff/21055/content/browser/speech/inp...
content/browser/speech/input_tag_speech_dispatcher_host.cc:52:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
see my other message about this, same for below

http://codereview.chromium.org/10273006/diff/21055/content/browser/speech/spe...
File content/browser/speech/speech_recognition_dispatcher_host.cc (right):

http://codereview.chromium.org/10273006/diff/21055/content/browser/speech/spe...
content/browser/speech/speech_recognition_dispatcher_host.cc:59:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
nit: this dcheck, and all the ones below in message handlers, are unnecessary.
in effect you're testing content::BrowserMessageFilter, which doesn't/shouldn't
be tested in each derived class

http://codereview.chromium.org/10273006/diff/21055/content/browser/speech/spe...
content/browser/speech/speech_recognition_dispatcher_host.cc:139:
manager()->GetSessionContext(session_id);
nit: here and below, 4 spaces instead of 6

http://codereview.chromium.org/10273006/diff/21055/content/common/speech_reco...
File content/common/speech_recognition_messages.h (right):

http://codereview.chromium.org/10273006/diff/21055/content/common/speech_reco...
content/common/speech_recognition_messages.h:149:
IPC_MESSAGE_ROUTED2(SpeechRecognitionMsg_ErrorOccurred,
nit: blank line between each message

http://codereview.chromium.org/10273006/diff/21055/content/renderer/speech_re...
File content/renderer/speech_recognition_dispatcher.cc (right):

http://codereview.chromium.org/10273006/diff/21055/content/renderer/speech_re...
content/renderer/speech_recognition_dispatcher.cc:16: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebSpeechRecognizer.h"
nit: this is already in your header, so take out

http://codereview.chromium.org/10273006/diff/21055/content/renderer/speech_re...
content/renderer/speech_recognition_dispatcher.cc:95:
DCHECK(recognizer_client_);
nit: all these dchecks aren't necessary. if it's null, the next line will crash
which is just as useful of a signal to the developer

http://codereview.chromium.org/10273006/diff/21055/content/renderer/speech_re...
File content/renderer/speech_recognition_dispatcher.h (right):

http://codereview.chromium.org/10273006/diff/21055/content/renderer/speech_re...
content/renderer/speech_recognition_dispatcher.h:26: class
WebSpeechRecognizerClient;
these forward declarations are already in WebSPeechRecognizer.h, so don't repeat
it

http://codereview.chromium.org/10273006/diff/21055/content/renderer/speech_re...
content/renderer/speech_recognition_dispatcher.h:35:
SpeechRecognitionDispatcher(RenderViewImpl* render_view);
nit: explicit

http://codereview.chromium.org/10273006/diff/21055/content/renderer/speech_re...
content/renderer/speech_recognition_dispatcher.h:39: // RenderView::Observer
implementation.
nit: RenderViewObserver

[dmazzoni, 2012-05-23 15:42:19]

Looks good for tts

http://codereview.chromium.org/10273006/diff/37017/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/10273006/diff/37017/chrome/chrome_browser.gypi...
chrome/chrome_browser.gypi:4435:
/'third_party/mozilla_security_manager/nsNSSCertHelper.cpp',
typo?

[Primiano Tucci (use gerrit), 2012-05-23 17:11:56]

http://codereview.chromium.org/10273006/diff/21055/content/browser/speech/inp...
File content/browser/speech/input_tag_speech_dispatcher_host.cc (right):

http://codereview.chromium.org/10273006/diff/21055/content/browser/speech/inp...
content/browser/speech/input_tag_speech_dispatcher_host.cc:52:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
On 2012/05/23 15:41:24, John Abd-El-Malek wrote:
> see my other message about this, same for below

Done.

http://codereview.chromium.org/10273006/diff/21055/content/browser/speech/spe...
File content/browser/speech/speech_recognition_dispatcher_host.cc (right):

http://codereview.chromium.org/10273006/diff/21055/content/browser/speech/spe...
content/browser/speech/speech_recognition_dispatcher_host.cc:59:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
On 2012/05/23 15:41:24, John Abd-El-Malek wrote:
> nit: this dcheck, and all the ones below in message handlers, are unnecessary.
> in effect you're testing content::BrowserMessageFilter, which
doesn't/shouldn't
> be tested in each derived class

Done.

http://codereview.chromium.org/10273006/diff/21055/content/browser/speech/spe...
content/browser/speech/speech_recognition_dispatcher_host.cc:139:
manager()->GetSessionContext(session_id);
On 2012/05/23 15:41:24, John Abd-El-Malek wrote:
> nit: here and below, 4 spaces instead of 6

Done.

http://codereview.chromium.org/10273006/diff/21055/content/common/speech_reco...
File content/common/speech_recognition_messages.h (right):

http://codereview.chromium.org/10273006/diff/21055/content/common/speech_reco...
content/common/speech_recognition_messages.h:149:
IPC_MESSAGE_ROUTED2(SpeechRecognitionMsg_ErrorOccurred,
On 2012/05/23 15:41:24, John Abd-El-Malek wrote:
> nit: blank line between each message

Done.

http://codereview.chromium.org/10273006/diff/21055/content/renderer/speech_re...
File content/renderer/speech_recognition_dispatcher.cc (right):

http://codereview.chromium.org/10273006/diff/21055/content/renderer/speech_re...
content/renderer/speech_recognition_dispatcher.cc:16: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebSpeechRecognizer.h"
On 2012/05/23 15:41:24, John Abd-El-Malek wrote:
> nit: this is already in your header, so take out

Done.

http://codereview.chromium.org/10273006/diff/21055/content/renderer/speech_re...
content/renderer/speech_recognition_dispatcher.cc:95:
DCHECK(recognizer_client_);
On 2012/05/23 15:41:24, John Abd-El-Malek wrote:
> nit: all these dchecks aren't necessary. if it's null, the next line will
crash
> which is just as useful of a signal to the developer

Done.

http://codereview.chromium.org/10273006/diff/21055/content/renderer/speech_re...
File content/renderer/speech_recognition_dispatcher.h (right):

http://codereview.chromium.org/10273006/diff/21055/content/renderer/speech_re...
content/renderer/speech_recognition_dispatcher.h:26: class
WebSpeechRecognizerClient;
On 2012/05/23 15:41:24, John Abd-El-Malek wrote:
> these forward declarations are already in WebSPeechRecognizer.h, so don't
repeat
> it

Done.

http://codereview.chromium.org/10273006/diff/21055/content/renderer/speech_re...
content/renderer/speech_recognition_dispatcher.h:35:
SpeechRecognitionDispatcher(RenderViewImpl* render_view);
On 2012/05/23 15:41:24, John Abd-El-Malek wrote:
> nit: explicit

Done.

http://codereview.chromium.org/10273006/diff/21055/content/renderer/speech_re...
content/renderer/speech_recognition_dispatcher.h:39: // RenderView::Observer
implementation.
On 2012/05/23 15:41:24, John Abd-El-Malek wrote:
> nit: RenderViewObserver

Done.

http://codereview.chromium.org/10273006/diff/37017/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/10273006/diff/37017/chrome/chrome_browser.gypi...
chrome/chrome_browser.gypi:4435:
/'third_party/mozilla_security_manager/nsNSSCertHelper.cpp',
On 2012/05/23 15:42:19, Dominic Mazzoni wrote:
> typo?
Oops, thanks.

[commit-bot: I haz the power, 2012-05-24 14:04:05]

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

[commit-bot: I haz the power, 2012-05-24 15:36:31]

Change committed as 138801

[hans, 2012-05-24 16:21:05]

On 2012/05/24 15:36:31, I haz the power (commit-bot) wrote:
> Change committed as 138801

Unfortunately, this turned the Linux x64 bot red, complaining about "sizes". It
looks like the binary size jumped ~40k with this.. not sure what's going on
exactly.

Reverted in r138807.

[Primiano Tucci (use gerrit), 2012-05-25 06:34:11]

On 2012/05/24 16:21:05, hans wrote:
> On 2012/05/24 15:36:31, I haz the power (commit-bot) wrote:
> > Change committed as 138801
> 
> Unfortunately, this turned the Linux x64 bot red, complaining about "sizes".
It
> looks like the binary size jumped ~40k with this.. not sure what's going on
> exactly.
> 
> Reverted in r138807.

There was a change in the .gyp rules (to avoid excluding TTS extension with the
ENABLE_SPEECH_INPUT flag). However, I guess it should not affect Linux x64 since
it should be enabled anyway.

[hans, 2012-05-25 10:23:20]

Re-landed in r139015, bots looking good so far.