Refactoring of chrome speech recognition architecture (CL1.2)

Removed the SpeechRecognizerDelegate interface and replaced with SpeechRecognitionEventListener. By now it might seem just a rename of the former interface, however this interface will gradually incorporate, in the next CLs, the other delegates and become the reference interface used by all clients to interact with SpeechRecognition inside Chrome.

BUG=116954
TEST=none

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

[hans, 2012-03-13 14:32:46]

looks good! just a couple of nits

thanks for breaking this out from the big patch

https://chromiumcodereview.appspot.com/9688012/diff/1/chrome/browser/speech/s...
File chrome/browser/speech/speech_input_extension_manager.cc (right):

https://chromiumcodereview.appspot.com/9688012/diff/1/chrome/browser/speech/s...
chrome/browser/speech/speech_input_extension_manager.cc:610:
content::SpeechRecognitionEventListener* delegate,
event_listener?

https://chromiumcodereview.appspot.com/9688012/diff/1/chrome/browser/speech/s...
File chrome/browser/speech/speech_input_extension_manager.h (right):

https://chromiumcodereview.appspot.com/9688012/diff/1/chrome/browser/speech/s...
chrome/browser/speech/speech_input_extension_manager.h:39:
content::SpeechRecognitionEventListener* delegate,
rename the parameter to "listener" or "event_listener"?

https://chromiumcodereview.appspot.com/9688012/diff/1/chrome/browser/speech/s...
chrome/browser/speech/speech_input_extension_manager.h:144:
content::SpeechRecognitionEventListener* delegate,
dito

https://chromiumcodereview.appspot.com/9688012/diff/1/content/public/browser/...
File content/public/browser/speech_recognition_event_listener.h (right):

https://chromiumcodereview.appspot.com/9688012/diff/1/content/public/browser/...
content/public/browser/speech_recognition_event_listener.h:21: // Invoked when
the first audio capture is initiated
nit: period.

https://chromiumcodereview.appspot.com/9688012/diff/1/content/public/browser/...
content/public/browser/speech_recognition_event_listener.h:66: } // namespace
content
nit: two spaces between } and //

https://chromiumcodereview.appspot.com/9688012/diff/1/content/public/browser/...
content/public/browser/speech_recognition_event_listener.h:68: #endif //
CONTENT_PUBLIC_BROWSER_SPEECH_RECOGNITION_EVENT_LISTENER_H_
nit: two spaces

[Satish, 2012-03-13 15:30:15]

Good stuff.

https://chromiumcodereview.appspot.com/9688012/diff/1/chrome/browser/speech/s...
File chrome/browser/speech/speech_input_extension_manager.cc (right):

https://chromiumcodereview.appspot.com/9688012/diff/1/chrome/browser/speech/s...
chrome/browser/speech/speech_input_extension_manager.cc:241: int caller_id,
change 'caller_id' to 'session_id' throughout to match the change in the
signatures in the header

https://chromiumcodereview.appspot.com/9688012/diff/1/chrome/browser/speech/s...
File chrome/browser/speech/speech_input_extension_manager.h (right):

https://chromiumcodereview.appspot.com/9688012/diff/1/chrome/browser/speech/s...
chrome/browser/speech/speech_input_extension_manager.h:41: int caller_id,
change caller_id to session_id throughout

https://chromiumcodereview.appspot.com/9688012/diff/1/content/browser/speech/...
File content/browser/speech/speech_recognizer_impl.h (right):

https://chromiumcodereview.appspot.com/9688012/diff/1/content/browser/speech/...
content/browser/speech/speech_recognizer_impl.h:43: int caller_id,
can we also rename all 'caller_id's to 'session_id' to be consistent between the
listener interface and implementations invoking it?

https://chromiumcodereview.appspot.com/9688012/diff/1/content/browser/speech/...
File content/browser/speech/speech_recognizer_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/9688012/diff/1/content/browser/speech/...
content/browser/speech/speech_recognizer_impl_unittest.cc:99: audio_closed_ =
true;
use audio_ended_ to match audio_started_ above?

https://chromiumcodereview.appspot.com/9688012/diff/1/content/browser/speech/...
content/browser/speech/speech_recognizer_impl_unittest.cc:127: 
remove extra newline

https://chromiumcodereview.appspot.com/9688012/diff/1/content/public/browser/...
File content/public/browser/speech_recognition_event_listener.h (right):

https://chromiumcodereview.appspot.com/9688012/diff/1/content/public/browser/...
content/public/browser/speech_recognition_event_listener.h:12: namespace content
{
add newline after

https://chromiumcodereview.appspot.com/9688012/diff/1/content/public/browser/...
content/public/browser/speech_recognition_event_listener.h:53: virtual void
OnAudioLevelsChanged(int session_id, float volume,
this should be OnAudioLevelsChange to match others such as
onAudioStart/OnRecognitionEnd etc.

https://chromiumcodereview.appspot.com/9688012/diff/1/content/public/browser/...
content/public/browser/speech_recognition_event_listener.h:58: // |success|
indicates weather the recognition concluded with at least a
Since there are separate OnRecognitionResult and OnRecognitionError methods, why
do we need this second parameter?

https://chromiumcodereview.appspot.com/9688012/diff/1/content/public/browser/...
content/public/browser/speech_recognition_event_listener.h:66: } // namespace
content
2 spaces before //

https://chromiumcodereview.appspot.com/9688012/diff/1/content/public/browser/...
content/public/browser/speech_recognition_event_listener.h:68: #endif //
CONTENT_PUBLIC_BROWSER_SPEECH_RECOGNITION_EVENT_LISTENER_H_
ditto

https://chromiumcodereview.appspot.com/9688012/diff/1/content/public/browser/...
File content/public/browser/speech_recognizer.h (right):

https://chromiumcodereview.appspot.com/9688012/diff/1/content/public/browser/...
content/public/browser/speech_recognizer.h:55: // Stops recording audio and
fianlizes recognition, possibly getting results.
fianlizes -> finalizes

https://chromiumcodereview.appspot.com/9688012/diff/1/content/public/browser/...
content/public/browser/speech_recognizer.h:58: // Checks wether the recognizer
is active.
please define what 'active' means so the comment becomes useful :)

[Primiano Tucci (use gerrit), 2012-03-14 09:40:35]

http://codereview.chromium.org/9688012/diff/1/chrome/browser/speech/speech_in...
File chrome/browser/speech/speech_input_extension_manager.cc (right):

http://codereview.chromium.org/9688012/diff/1/chrome/browser/speech/speech_in...
chrome/browser/speech/speech_input_extension_manager.cc:241: int caller_id,
On 2012/03/13 15:30:15, Satish wrote:
> change 'caller_id' to 'session_id' throughout to match the change in the
> signatures in the header
See previous reply

http://codereview.chromium.org/9688012/diff/1/chrome/browser/speech/speech_in...
chrome/browser/speech/speech_input_extension_manager.cc:610:
content::SpeechRecognitionEventListener* delegate,
On 2012/03/13 14:32:46, hans wrote:
> event_listener?
Right.
Renaming to just "listener", as agreed offline.

http://codereview.chromium.org/9688012/diff/1/chrome/browser/speech/speech_in...
File chrome/browser/speech/speech_input_extension_manager.h (right):

http://codereview.chromium.org/9688012/diff/1/chrome/browser/speech/speech_in...
chrome/browser/speech/speech_input_extension_manager.h:39:
content::SpeechRecognitionEventListener* delegate,
On 2012/03/13 14:32:46, hans wrote:
> rename the parameter to "listener" or "event_listener"?

Done.

http://codereview.chromium.org/9688012/diff/1/chrome/browser/speech/speech_in...
chrome/browser/speech/speech_input_extension_manager.h:41: int caller_id,
On 2012/03/13 15:30:15, Satish wrote:
> change caller_id to session_id throughout

Hmm, I think I'll name all caller_id (below) for now. This is a spoiler,
"Session" will be invented only in the next CLs.

http://codereview.chromium.org/9688012/diff/1/chrome/browser/speech/speech_in...
chrome/browser/speech/speech_input_extension_manager.h:144:
content::SpeechRecognitionEventListener* delegate,
On 2012/03/13 14:32:46, hans wrote:
> dito

Done.

http://codereview.chromium.org/9688012/diff/1/content/browser/speech/speech_r...
File content/browser/speech/speech_recognizer_impl.h (right):

http://codereview.chromium.org/9688012/diff/1/content/browser/speech/speech_r...
content/browser/speech/speech_recognizer_impl.h:43: int caller_id,
On 2012/03/13 15:30:15, Satish wrote:
> can we also rename all 'caller_id's to 'session_id' to be consistent between
the
> listener interface and implementations invoking it?
See previous comment. Until the dispatcher host keeps track of "Callers" we
should name it caller_id.

http://codereview.chromium.org/9688012/diff/1/content/browser/speech/speech_r...
File content/browser/speech/speech_recognizer_impl_unittest.cc (right):

http://codereview.chromium.org/9688012/diff/1/content/browser/speech/speech_r...
content/browser/speech/speech_recognizer_impl_unittest.cc:99: audio_closed_ =
true;
On 2012/03/13 15:30:15, Satish wrote:
> use audio_ended_ to match audio_started_ above?

Right.

http://codereview.chromium.org/9688012/diff/1/content/browser/speech/speech_r...
content/browser/speech/speech_recognizer_impl_unittest.cc:127: 
On 2012/03/13 15:30:15, Satish wrote:
> remove extra newline

Done.

http://codereview.chromium.org/9688012/diff/1/content/public/browser/speech_r...
File content/public/browser/speech_recognition_event_listener.h (right):

http://codereview.chromium.org/9688012/diff/1/content/public/browser/speech_r...
content/public/browser/speech_recognition_event_listener.h:12: namespace content
{
On 2012/03/13 15:30:15, Satish wrote:
> add newline after

Done.

http://codereview.chromium.org/9688012/diff/1/content/public/browser/speech_r...
content/public/browser/speech_recognition_event_listener.h:21: // Invoked when
the first audio capture is initiated
On 2012/03/13 14:32:46, hans wrote:
> nit: period.

Done.

http://codereview.chromium.org/9688012/diff/1/content/public/browser/speech_r...
content/public/browser/speech_recognition_event_listener.h:53: virtual void
OnAudioLevelsChanged(int session_id, float volume,
On 2012/03/13 15:30:15, Satish wrote:
> this should be OnAudioLevelsChange to match others such as
> onAudioStart/OnRecognitionEnd etc.
Right.

http://codereview.chromium.org/9688012/diff/1/content/public/browser/speech_r...
content/public/browser/speech_recognition_event_listener.h:58: // |success|
indicates weather the recognition concluded with at least a
On 2012/03/13 15:30:15, Satish wrote:
> Since there are separate OnRecognitionResult and OnRecognitionError methods,
why
> do we need this second parameter?
Right, it was just left from an aborted CL, I did't noted it.

http://codereview.chromium.org/9688012/diff/1/content/public/browser/speech_r...
content/public/browser/speech_recognition_event_listener.h:66: } // namespace
content
On 2012/03/13 14:32:46, hans wrote:
> nit: two spaces between } and //

Done.

http://codereview.chromium.org/9688012/diff/1/content/public/browser/speech_r...
content/public/browser/speech_recognition_event_listener.h:66: } // namespace
content
On 2012/03/13 15:30:15, Satish wrote:
> 2 spaces before //

Done.

http://codereview.chromium.org/9688012/diff/1/content/public/browser/speech_r...
content/public/browser/speech_recognition_event_listener.h:68: #endif //
CONTENT_PUBLIC_BROWSER_SPEECH_RECOGNITION_EVENT_LISTENER_H_
On 2012/03/13 14:32:46, hans wrote:
> nit: two spaces

Done.

http://codereview.chromium.org/9688012/diff/1/content/public/browser/speech_r...
content/public/browser/speech_recognition_event_listener.h:68: #endif //
CONTENT_PUBLIC_BROWSER_SPEECH_RECOGNITION_EVENT_LISTENER_H_
On 2012/03/13 15:30:15, Satish wrote:
> ditto

Done.

http://codereview.chromium.org/9688012/diff/1/content/public/browser/speech_r...
File content/public/browser/speech_recognizer.h (right):

http://codereview.chromium.org/9688012/diff/1/content/public/browser/speech_r...
content/public/browser/speech_recognizer.h:55: // Stops recording audio and
fianlizes recognition, possibly getting results.
On 2012/03/13 15:30:15, Satish wrote:
> fianlizes -> finalizes

Done.

http://codereview.chromium.org/9688012/diff/1/content/public/browser/speech_r...
content/public/browser/speech_recognizer.h:58: // Checks wether the recognizer
is active.
On 2012/03/13 15:30:15, Satish wrote:
> please define what 'active' means so the comment becomes useful :)
Right!

[Satish, 2012-03-14 09:48:49]

lgtm

[commit-bot: I haz the power, 2012-03-14 09:53:04]

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

[commit-bot: I haz the power, 2012-03-14 09:53:09]

Presubmit check for 9688012-11001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content
    content/public/browser

Presubmit checks took 1.3s to calculate.

[jam, 2012-03-14 15:11:55]

lgtm

[commit-bot: I haz the power, 2012-03-14 15:23:41]

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

[commit-bot: I haz the power, 2012-03-14 16:22:00]

Try job failure for 9688012-14005 (retry) on linux_rel for step
"remoting_unittests".
It's a second try, previously, step "remoting_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-03-14 16:23:42]

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

[commit-bot: I haz the power, 2012-03-14 17:09:56]

Try job failure for 9688012-14005 (retry) on linux_rel for step
"remoting_unittests".
It's a second try, previously, step "remoting_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-03-14 17:13:55]

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

[commit-bot: I haz the power, 2012-03-14 20:29:53]

Change committed as 126721