Refactoring of chrome speech recognition architecture (CL1.3)

speech_recognition_request
- Renamed speech_recognition_request to google_one_shot_remote_engine
- Audio encoder moved here (previously was in speech_recognizer).
Rationale: Audio encoding is not a requirement of speech recognition. It is a need of the google_one_shot_remote_engine, due to its remote nature.

speech_recognition_engine
- Extracted the interface SpeechRecognitionEngine (and, accordingly, Delegate) from the former speech_recognition_request.
Rationale: google_one_shot_remote_engine is just one of the possible engines (yet currently the only one) that can be exploited to recognize speech.

speech_recognition_result
- Added SpeechRecognitionError, encapsulating the error code and further error details (for specializing audio errors, since chrome needs them).

speech_recognition_manager
- Minor adaptions to the new speech_recognizer.cc
- Replaced the SpeechRecognitionRequestDelegate interface with the brand new speech_recognition_event_listener

BUG=116954
TEST=none

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

[Primiano Tucci (use gerrit), 2012-03-15 09:14:36]

I added some comments in the codereview interface in order to help the review
process.

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
File content/browser/speech/speech_recognition_manager_impl.cc (left):

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
content/browser/speech/speech_recognition_manager_impl.cc:206: 
These checks have been moved into the speech_recognizer_impl, since it is the
one that takes care entirely of the audio capture process.

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
File content/browser/speech/speech_recognition_manager_impl.cc (right):

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
content/browser/speech/speech_recognition_manager_impl.cc:279: if
(recording_caller_id_ != caller_id)
Basically we need these checks because the new event_listener interface contract
is slightly changes, and the AudioEnd and RecognitionEnd events are always
raised even in case of error.

Spoiler: this code is transitional (just to make this CL work correctly). This
class will be reimplemented in the next CL as a FSM.

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
File content/browser/speech/speech_recognizer_impl_unittest.cc (right):

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl_unittest.cc:205:
EXPECT_TRUE(recognition_ended_);
The event interface is slightly changed. AudioEnded and RecognitionEnded now are
always raised, in order to guarantee start/end closure.

[hans, 2012-03-16 11:12:56]

Patch looks good; as usual i just have a bunch of nits.

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/goog...
File content/browser/speech/google_ssfe_remote_engine.h (right):

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.h:48: // Google Speech API
1.3.2.
it's not immediately obvious what the SSFE part of the name means; maybe that
could be baked into this comment?

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.h:57:
GoogleSSFERemoteEngine(net::URLRequestContextGetter* context);
one-parameter constructors should be explicit

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/goog...
File content/browser/speech/google_ssfe_remote_engine_unittest.cc (right):

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine_unittest.cc:21:
GoogleSSFERemoteEngineTest() { }
not really your code, but while you're here, there shouldn't be space inside
empty braces

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
File content/browser/speech/speech_recognition_engine.h (right):

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:74: Delegate* sr_delegate()
const {return delegate_;}
should this just be called delegate() ?

nit: spaces around the statement in {}

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
File content/browser/speech/speech_recognition_manager_impl.cc (right):

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
content/browser/speech/speech_recognition_manager_impl.cc:279: if
(recording_caller_id_ != caller_id)
On 2012/03/15 09:14:36, Primiano Tucci wrote:
> Basically we need these checks because the new event_listener interface
contract
> is slightly changes, and the AudioEnd and RecognitionEnd events are always
> raised even in case of error.
> 
> Spoiler: this code is transitional (just to make this CL work correctly). This
> class will be reimplemented in the next CL as a FSM.

maybe put a comment on the if statement about when this can happen?

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
content/browser/speech/speech_recognition_manager_impl.cc:315:
delegate_->ShowMicError(caller_id,
indentation looks wrong

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
File content/browser/speech/speech_recognizer_impl.cc (right):

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.cc:18: #define
UNREACHABLE_CONDITION() do{ NOTREACHED(); return state_; } while(0)
ultra nit: there should be a space between the "do" and {

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.cc:136: // of causalilty between
events.
s/causalilty/causality/

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.cc:140: // asynchronous event
processing mixed with syncrhonous callback can cause very
s/syncrhonous/synchronous/

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.cc:244: // current one.
should the comment be moved up a little, or put in the if statement (which
should then have braces)? i assume the comment refers to the
ProcessAudioPipeline() call above, but it's confusing that the comment comes
afterwards, and at a different indent level...

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.cc:369:
SpeechRecognizerImpl::InitializeAndStartRecording() {
i'm unsure about the indentation here..
spontaneously i would write

SpeechRecognizerImpl::FSMState
SpeechRecognizerImpl::InitializeAndStartRecording() {

but maybe that's just me :) this would apply for a couple of more functions
below

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.cc:384: if
(!audio_manager->HasAudioInputDevices())
i would put { around the body of the if since it's covering more than two lines
(same for the next ifs below)

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.cc:608: } // namespace speech
ultra nit: two spaces between } and //

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
File content/browser/speech/speech_recognizer_impl.h (right):

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.h:28: // TODO(primiano) Current
CL: check CONTENT_EXPORT and NON_EXPORTED_BASE
should this be addressed before this cl is landed?

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.h:35: // SpeechRecognitionEngine
(that must be injected through set_delegate(...)).
i'm confused by the last bit here.. "SpeechRecognitionEngine (that must be
injected through set_delegate)"

isn't the SpeechRecognitionEngine passed into the constructor?

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
File content/browser/speech/speech_recognizer_impl_unittest.cc (right):

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl_unittest.cc:86: } // namespace
the extra space should be there

[hans, 2012-03-16 13:58:03]

Oh, and the description of the CL should have a header line, maybe "Speech
refactoring: reimplement SpeechRecognizerImpl as an FSM"

[Primiano Tucci (use gerrit), 2012-03-16 15:03:42]

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/goog...
File content/browser/speech/google_ssfe_remote_engine.h (right):

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.h:48: // Google Speech API
1.3.2.
On 2012/03/16 11:12:56, hans wrote:
> it's not immediately obvious what the SSFE part of the name means; maybe that
> could be baked into this comment?
Absolutely right.

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.h:57:
GoogleSSFERemoteEngine(net::URLRequestContextGetter* context);
On 2012/03/16 11:12:56, hans wrote:
> one-parameter constructors should be explicit

Done.

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/goog...
File content/browser/speech/google_ssfe_remote_engine_unittest.cc (right):

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine_unittest.cc:21:
GoogleSSFERemoteEngineTest() { }
On 2012/03/16 11:12:56, hans wrote:
> not really your code, but while you're here, there shouldn't be space inside
> empty braces

Done.

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
File content/browser/speech/speech_recognition_engine.h (right):

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:74: Delegate* sr_delegate()
const {return delegate_;}
On 2012/03/16 11:12:56, hans wrote:
> should this just be called delegate() ?
> 
> nit: spaces around the statement in {}
Hmm I named it so because there was a name clash into GoogleSSFERemoteEngine
that implemented also URLFetcherDelegate. But now it seems to be gone (probably
URLFetcherDelegate has changed in the meanwhile).

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
File content/browser/speech/speech_recognition_manager_impl.cc (right):

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
content/browser/speech/speech_recognition_manager_impl.cc:279: if
(recording_caller_id_ != caller_id)
On 2012/03/16 11:12:56, hans wrote:
> On 2012/03/15 09:14:36, Primiano Tucci wrote:
> > Basically we need these checks because the new event_listener interface
> contract
> > is slightly changes, and the AudioEnd and RecognitionEnd events are always
> > raised even in case of error.
> > 
> > Spoiler: this code is transitional (just to make this CL work correctly).
This
> > class will be reimplemented in the next CL as a FSM.
> 
> maybe put a comment on the if statement about when this can happen?

Spoiler: this code is going to disappear in the next CL :)

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
content/browser/speech/speech_recognition_manager_impl.cc:315:
delegate_->ShowMicError(caller_id,
On 2012/03/16 11:12:56, hans wrote:
> indentation looks wrong

Done.

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
File content/browser/speech/speech_recognizer_impl.cc (right):

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.cc:18: #define
UNREACHABLE_CONDITION() do{ NOTREACHED(); return state_; } while(0)
On 2012/03/16 11:12:56, hans wrote:
> ultra nit: there should be a space between the "do" and {

Done.

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.cc:136: // of causalilty between
events.
On 2012/03/16 11:12:56, hans wrote:
> s/causalilty/causality/

Done.

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.cc:140: // asynchronous event
processing mixed with syncrhonous callback can cause very
On 2012/03/16 11:12:56, hans wrote:
> s/syncrhonous/synchronous/

Done.

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.cc:244: // current one.
On 2012/03/16 11:12:56, hans wrote:
> should the comment be moved up a little, or put in the if statement (which
> should then have braces)? i assume the comment refers to the
> ProcessAudioPipeline() call above, but it's confusing that the comment comes
> afterwards, and at a different indent level...

Mmm It is not referred to the if () statement itself. It is just a warning to
"don't move process audio pipeline after this point". I modified the comment a
bit so it might sound more like a warning rather than a comment.

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.cc:369:
SpeechRecognizerImpl::InitializeAndStartRecording() {
On 2012/03/16 11:12:56, hans wrote:
> i'm unsure about the indentation here..
> spontaneously i would write
> 
> SpeechRecognizerImpl::FSMState
> SpeechRecognizerImpl::InitializeAndStartRecording() {
> 
> but maybe that's just me :) this would apply for a couple of more functions
> below

Done.

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.cc:384: if
(!audio_manager->HasAudioInputDevices())
On 2012/03/16 11:12:56, hans wrote:
> i would put { around the body of the if since it's covering more than two
lines
> (same for the next ifs below)

Done.

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.cc:608: } // namespace speech
On 2012/03/16 11:12:56, hans wrote:
> ultra nit: two spaces between } and //

Done.

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
File content/browser/speech/speech_recognizer_impl.h (right):

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.h:28: // TODO(primiano) Current
CL: check CONTENT_EXPORT and NON_EXPORTED_BASE
On 2012/03/16 11:12:56, hans wrote:
> should this be addressed before this cl is landed?

Done.

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.h:35: // SpeechRecognitionEngine
(that must be injected through set_delegate(...)).
On 2012/03/16 11:12:56, hans wrote:
> i'm confused by the last bit here.. "SpeechRecognitionEngine (that must be
> injected through set_delegate)"
> 
> isn't the SpeechRecognitionEngine passed into the constructor?

Uh, definitely right. Probably it was just a wrong copy/paste.

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
File content/browser/speech/speech_recognizer_impl_unittest.cc (right):

http://codereview.chromium.org/9663066/diff/11030/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl_unittest.cc:86: } // namespace
On 2012/03/16 11:12:56, hans wrote:
> the extra space should be there

Done.

[Satish, 2012-03-16 17:00:35]

First set of comments below, will review the rest of files next.

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
File content/browser/speech/google_ssfe_remote_engine.cc (right):

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.cc:40: const int
SPEECH_API_STATUS_NO_ERROR = 0;
add comment here on what these are (i.e. codes returned by the one shot speech
service). Also would be nice to have the names uniquely prefixed instead of a
generic SPEECH_API

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.cc:80: case
SPEECH_API_STATUS_NO_ERROR:
should we set the error code to none in this case?

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.cc:151: 
remove extra newline

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.cc:158: :
filter_profanities(false),
4 spaces before :

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.cc:159:
audio_sample_rate(8000),
these two values should probably be constants at the top of this file

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.cc:169: encoder_(NULL) {
this should get autoinitialized?

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.cc:182:
std::vector<std::string> parts;
move this to line 203 where it is getting used for the first time

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.cc:236: url_fetcher_.reset();
do you expect callers to reuse this object across multiple recognition sessions
or create a new one each time? If it is the latter then this method is probably
not required and the destructor could clean up.

If you want to keep this method as is, should you also clean up other data
members such as encoder_ ?

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.cc:240:
DCHECK(url_fetcher_.get());
also check for encoder_ as done below

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.cc:252: std::vector<short>
samples(
short -> int16 ?

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.cc:255: samples.size() *
sizeof(short),
ditto

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.cc:272: // The default error
code in case of parse errors is NETWORK_FAILURE, however
add a newline before full line comments such as this for better readability

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.cc:274: if
(!source->GetStatus().is_success()   ||
no need to align the ||, could just leave 1 space before them

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
File content/browser/speech/google_ssfe_remote_engine.h (right):

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.h:41: int
audio_num_bits_per_sample;
could add a newline below this

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.h:49: class
GoogleSSFERemoteEngine : public SpeechRecognitionEngine,
I don't think SSFERemoteEngine is a good name for the class and file. Can you
use a different name, e.g. GoogleOneShotRecognitionEngine since this performs
recognition only after all the audio has been given? Please update comment above
accordingly

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.h:53: static const int
kAudioPacketIntervalMs;
are these statics used outside this class? If not, suggest moving to the .cc
file into an anonymous namespace

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.h:58: void
SetConfiguration(const GoogleSSFERemoteEngineConfig& config);
suggest moving this below the constructor/destructor pair

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.h:76: AudioEncoder::Codec
codec_;
is this member required since it is hardcodec to FLAC? You could remove it now
and if necessary to switch between codecs add later

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/spee...
File content/browser/speech/speech_recognition_engine.h (right):

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:36: virtual void
OnSpeechEngineResult(
OnSpeech -> OnSpeechRecognition ?

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:38: virtual void
OnSpeechEngineError(
ditto

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:48: virtual void
SpeechRecognitionBegins() = 0;
perhaps 'StartSession' or something similar?

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:50: // Notify the engine that
we have ended (either gracefully or aborting) the
I don't see a way to abort in this class. Should there be one?

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:52: virtual void
SpeechRecognitionEnds() = 0;
EndSession ?

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:57: virtual void
PushSpeechAudio(const AudioChunk& data) = 0;
AddAudioChunk or something similar?

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:60: // will be pushed. The
engine can finalize the recognition in order to provide
"finalize the recognition in order to provide a final result" - sounds quite
abstract :) Can you clarify more or if we can't just remove this line?

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:62: virtual void
SpeechAudioStreamComplete() = 0;
AudioCompleted or something similar?

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:68: virtual int
DesiredAudioChunkDurationMs() const = 0;
Desired -> GetDesired ?

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:76: private:
add DISALLOW_COPY_AND_ASSIGN(SpeechRecognitionEngine);

[Primiano Tucci (use gerrit), 2012-03-20 13:14:50]

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
File content/browser/speech/google_ssfe_remote_engine.cc (right):

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.cc:40: const int
SPEECH_API_STATUS_NO_ERROR = 0;
On 2012/03/16 17:00:35, Satish wrote:
> add comment here on what these are (i.e. codes returned by the one shot speech
> service). Also would be nice to have the names uniquely prefixed instead of a
> generic SPEECH_API

Done.

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.cc:80: case
SPEECH_API_STATUS_NO_ERROR:
On 2012/03/16 17:00:35, Satish wrote:
> should we set the error code to none in this case?

Done.

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.cc:151: 
On 2012/03/16 17:00:35, Satish wrote:
> remove extra newline

Done.

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.cc:158: :
filter_profanities(false),
On 2012/03/16 17:00:35, Satish wrote:
> 4 spaces before :

Done.

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.cc:159:
audio_sample_rate(8000),
On 2012/03/16 17:00:35, Satish wrote:
> these two values should probably be constants at the top of this file

Done.

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.cc:169: encoder_(NULL) {
On 2012/03/16 17:00:35, Satish wrote:
> this should get autoinitialized?

Done.

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.cc:182:
std::vector<std::string> parts;
On 2012/03/16 17:00:35, Satish wrote:
> move this to line 203 where it is getting used for the first time

Done.

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.cc:236: url_fetcher_.reset();
In some occasions it can be reused (e.g, the user clicks on "try again" button).
The encoder, however, is already reset in SpeechAudioStreamComplete(), so there
is IMHO no need to reset it again here.

On 2012/03/16 17:00:35, Satish wrote:
> do you expect callers to reuse this object across multiple recognition
sessions
> or create a new one each time? If it is the latter then this method is
probably
> not required and the destructor could clean up.
> 
> If you want to keep this method as is, should you also clean up other data
> members such as encoder_ ?

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.cc:240:
DCHECK(url_fetcher_.get());
On 2012/03/16 17:00:35, Satish wrote:
> also check for encoder_ as done below

Done.

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.cc:252: std::vector<short>
samples(
On 2012/03/16 17:00:35, Satish wrote:
> short -> int16 ?

Done.

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.cc:255: samples.size() *
sizeof(short),
On 2012/03/16 17:00:35, Satish wrote:
> ditto

Done.

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.cc:272: // The default error
code in case of parse errors is NETWORK_FAILURE, however
On 2012/03/16 17:00:35, Satish wrote:
> add a newline before full line comments such as this for better readability

Done.

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.cc:274: if
(!source->GetStatus().is_success()   ||
On 2012/03/16 17:00:35, Satish wrote:
> no need to align the ||, could just leave 1 space before them

Done.

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
File content/browser/speech/google_ssfe_remote_engine.h (right):

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.h:41: int
audio_num_bits_per_sample;
On 2012/03/16 17:00:35, Satish wrote:
> could add a newline below this

Done.

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.h:53: static const int
kAudioPacketIntervalMs;
Yes, they are needed by the recognizer unittest.

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.h:58: void
SetConfiguration(const GoogleSSFERemoteEngineConfig& config);
On 2012/03/16 17:00:35, Satish wrote:
> suggest moving this below the constructor/destructor pair

Done.

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/goog...
content/browser/speech/google_ssfe_remote_engine.h:76: AudioEncoder::Codec
codec_;
Moved to a constant into the anonymous namespace (in the corresponding .cc
file).

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/spee...
File content/browser/speech/speech_recognition_engine.h (right):

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:36: virtual void
OnSpeechEngineResult(
I'd prefer OnSpeechEngine since this interface models the events notified by the
Engine. It makes it more easy to understand the code in the class implementing
this delegate (recognizer).

On 2012/03/16 17:00:35, Satish wrote:
> OnSpeech -> OnSpeechRecognition ?

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:38: virtual void
OnSpeechEngineError(
Same as above.
On 2012/03/16 17:00:35, Satish wrote:
> ditto

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:48: virtual void
SpeechRecognitionBegins() = 0;
Agree with the ambiguity of names.
However I don't like using the term session, since there is no notion of session
in the speech engine. Sessions are introduced into the manager/recognizer in
order to distinguish different requests.
I renamed all the engine methods, now should be more clear.

On 2012/03/16 17:00:35, Satish wrote:
> perhaps 'StartSession' or something similar?

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:50: // Notify the engine that
we have ended (either gracefully or aborting) the
Basically, Cleanup() does it, since can be called at any time (after an
initialize)
On 2012/03/16 17:00:35, Satish wrote:
> I don't see a way to abort in this class. Should there be one?

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:52: virtual void
SpeechRecognitionEnds() = 0;
Cleanup?

On 2012/03/16 17:00:35, Satish wrote:
> EndSession ?

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:57: virtual void
PushSpeechAudio(const AudioChunk& data) = 0;
TakeAudioChunk?
On 2012/03/16 17:00:35, Satish wrote:
> AddAudioChunk or something similar?

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:60: // will be pushed. The
engine can finalize the recognition in order to provide
On 2012/03/16 17:00:35, Satish wrote:
> "finalize the recognition in order to provide a final result" - sounds quite
> abstract :) Can you clarify more or if we can't just remove this line?

Done.

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:62: virtual void
SpeechAudioStreamComplete() = 0;
AudioChunksEnded?
On 2012/03/16 17:00:35, Satish wrote:
> AudioCompleted or something similar?

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:68: virtual int
DesiredAudioChunkDurationMs() const = 0;
On 2012/03/16 17:00:35, Satish wrote:
> Desired -> GetDesired ?

Done.

http://codereview.chromium.org/9663066/diff/10025/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:76: private:
On 2012/03/16 17:00:35, Satish wrote:
> add DISALLOW_COPY_AND_ASSIGN(SpeechRecognitionEngine);

Done.

[Satish, 2012-03-21 13:29:48]

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/goog...
File content/browser/speech/google_one_shot_remote_engine.cc (right):

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/goog...
content/browser/speech/google_one_shot_remote_engine.cc:35: const int
kGoogleSSFEStatusNoError = 0;
SSFE to something else?

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/goog...
content/browser/speech/google_one_shot_remote_engine.cc:39: const int
kDefaultConfigBPS = 16;
BPS -> BitsPerSample

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/goog...
content/browser/speech/google_one_shot_remote_engine.cc:40: const
speech::AudioEncoder::Codec kCodec = speech::AudioEncoder::CODEC_FLAC;
kCodec > kAudioCodec or kDefaultAudioCodec

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/goog...
content/browser/speech/google_one_shot_remote_engine.cc:173: void
GoogleOneShotRemoteEngine::SetConfiguration(
since the type name is Config how about renaming to SetConfig

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/goog...
content/browser/speech/google_one_shot_remote_engine.cc:178: void
GoogleOneShotRemoteEngine::Initialize() {
Reading this I got confused a bit why Initialize was doing all these checks,
then realised it was for starting a new recognition and it is not the object's
initialization. StartXxx seems clearer, e.g. StartRecognition/EndRecognition

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/goog...
content/browser/speech/google_one_shot_remote_engine.cc:181:
std::vector<std::string> parts;
move this to line 199 where it is first used

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/goog...
content/browser/speech/google_one_shot_remote_engine.cc:233: // Called only
after the results have been retrieved.
this comment isn't correct as this could be called to abort as well.

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/goog...
content/browser/speech/google_one_shot_remote_engine.cc:250: // UploadAudioChunk
requires a non-empty final buffer. So we encode a packet
suggest leaving a newline above full line comments like this

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/goog...
content/browser/speech/google_one_shot_remote_engine.cc:272: // The default
error code in case of parse errors is NETWORK_FAILURE, however
ditto

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/goog...
File content/browser/speech/google_one_shot_remote_engine.h (right):

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/goog...
content/browser/speech/google_one_shot_remote_engine.h:48: // Google Speech
Server Front End (SSFE) webservice, according to the HTTP-based
not sure if these two lines help. may be switch this to a short url
(goog.le/...) to the doc you mention?

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/goog...
content/browser/speech/google_one_shot_remote_engine.h:50: class
GoogleOneShotRemoteEngine : public SpeechRecognitionEngine,
I think if these don't fit in a single line the ':' goes to the next line
indented to 4 spaces

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/spee...
File content/browser/speech/speech_recognition_engine.h (right):

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:27: // Initialize           
Mandatory at beginning of SR.
Do you allow SpeechRecognitionEngine to be reused for multiple recognitions,
i.e. call Initialize>...>Cleanup and then do the same again later? If yes, seems
like Initialize/Cleanup should be renamed Start/StopRecognition to indicate they
are not object level initialization to be done once. If however the idea is to
create an engine object each time you want a new recognition to happen, then
Initialize/Cleanup should probably be part of the constructor and destructor.

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:37: virtual void
OnSpeechEngineResult(
SpeechEngine -> SpeechRecognitionEngine.
Add comment describing when this is called, same for the next method

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:41: protected:
add newline above

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:47: // Prepare for upcoming
TakeAudioChunk() calls.
reword comment to explain this should be called when a new recognition process
is about to begin

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:54: //
GetDesiredAudioChunkDurationMs(), and bytes_per_sample agrees to the engine
> 'where chunk length ...'
No need to mention this in the comment if they are DCHECKed in the code

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/spee...
File content/browser/speech/speech_recognition_manager_impl.cc (right):

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/spee...
content/browser/speech/speech_recognition_manager_impl.cc:208:
requests_[caller_id].recognizer->StartRecognition();
I see the checks present here before have now moved to OnRecognitionError. Does
this preserve the callbacks as they work today? In particular are these no-mic
and mic-in-use errors handled transparently allowing user to retry without
issuing callbacks to the listeners/webkit/javascript as they are today?

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/spee...
content/browser/speech/speech_recognition_manager_impl.cc:279: if
(recording_caller_id_ != caller_id)
this check shouldn't be here as we have a DCHECK for the same below

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/spee...
content/browser/speech/speech_recognition_manager_impl.cc:283: if
(!requests_[caller_id].is_active)
any reason why this check was added? seems like an error case.. may be a DCHECK
is more appropriate, but then if caller_id is the same as recording_caller_id_
then it should be active anyway so are we doing two checks for the same time?

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/spee...
content/browser/speech/speech_recognition_manager_impl.cc:292: if
(!HasPendingRequest(caller_id) || !requests_[caller_id].is_active)
ditto, could be a DCHECK if you really want to check for this

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/spee...
File content/browser/speech/speech_recognizer_impl.h (right):

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.h:19: class  AudioInputController;
2 spaces to 1 space

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.h:28: // TODO(primiano) Next CL:
Remove the Impl suffix and the exported
add newline above

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.h:63: enum FSMState {
could the FSM changes be moved to a separate CL as that is conceptually separate
from the rest of renames and cleanup? Also would be easier to review

http://codereview.chromium.org/9663066/diff/14001/content/public/common/speec...
File content/public/common/speech_recognition_result.h (right):

http://codereview.chromium.org/9663066/diff/14001/content/public/common/speec...
content/public/common/speech_recognition_result.h:60: int details;
I see the above enum is being passed via this member. Can we change the int to
the enum instead?

[Primiano Tucci (use gerrit), 2012-03-22 11:20:40]

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/goog...
File content/browser/speech/google_one_shot_remote_engine.cc (right):

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/goog...
content/browser/speech/google_one_shot_remote_engine.cc:35: const int
kGoogleSSFEStatusNoError = 0;
On 2012/03/21 13:29:48, Satish wrote:
> SSFE to something else?

Done.

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/goog...
content/browser/speech/google_one_shot_remote_engine.cc:39: const int
kDefaultConfigBPS = 16;
On 2012/03/21 13:29:48, Satish wrote:
> BPS -> BitsPerSample

Done.

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/goog...
content/browser/speech/google_one_shot_remote_engine.cc:40: const
speech::AudioEncoder::Codec kCodec = speech::AudioEncoder::CODEC_FLAC;
On 2012/03/21 13:29:48, Satish wrote:
> kCodec > kAudioCodec or kDefaultAudioCodec

Done.

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/goog...
content/browser/speech/google_one_shot_remote_engine.cc:173: void
GoogleOneShotRemoteEngine::SetConfiguration(
On 2012/03/21 13:29:48, Satish wrote:
> since the type name is Config how about renaming to SetConfig

Done.

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/goog...
content/browser/speech/google_one_shot_remote_engine.cc:178: void
GoogleOneShotRemoteEngine::Initialize() {
On 2012/03/21 13:29:48, Satish wrote:
> Reading this I got confused a bit why Initialize was doing all these checks,
> then realised it was for starting a new recognition and it is not the object's
> initialization. StartXxx seems clearer, e.g. StartRecognition/EndRecognition 

Done.

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/goog...
content/browser/speech/google_one_shot_remote_engine.cc:181:
std::vector<std::string> parts;
On 2012/03/21 13:29:48, Satish wrote:
> move this to line 199 where it is first used

Done.

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/goog...
content/browser/speech/google_one_shot_remote_engine.cc:233: // Called only
after the results have been retrieved.
On 2012/03/21 13:29:48, Satish wrote:
> this comment isn't correct as this could be called to abort as well.

Done.

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/goog...
content/browser/speech/google_one_shot_remote_engine.cc:250: // UploadAudioChunk
requires a non-empty final buffer. So we encode a packet
On 2012/03/21 13:29:48, Satish wrote:
> suggest leaving a newline above full line comments like this

Done.

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/goog...
content/browser/speech/google_one_shot_remote_engine.cc:272: // The default
error code in case of parse errors is NETWORK_FAILURE, however
On 2012/03/21 13:29:48, Satish wrote:
> ditto

Done.

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/goog...
File content/browser/speech/google_one_shot_remote_engine.h (right):

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/goog...
content/browser/speech/google_one_shot_remote_engine.h:48: // Google Speech
Server Front End (SSFE) webservice, according to the HTTP-based
On 2012/03/21 13:29:48, Satish wrote:
> not sure if these two lines help. may be switch this to a short url
> (goog.le/...) to the doc you mention?

Done.

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/goog...
content/browser/speech/google_one_shot_remote_engine.h:50: class
GoogleOneShotRemoteEngine : public SpeechRecognitionEngine,
On 2012/03/21 13:29:48, Satish wrote:
> I think if these don't fit in a single line the ':' goes to the next line
> indented to 4 spaces

Done.

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/spee...
File content/browser/speech/speech_recognition_engine.h (right):

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:27: // Initialize           
Mandatory at beginning of SR.
On 2012/03/21 13:29:48, Satish wrote:
> Do you allow SpeechRecognitionEngine to be reused for multiple recognitions,
> i.e. call Initialize>...>Cleanup and then do the same again later? If yes,
seems
> like Initialize/Cleanup should be renamed Start/StopRecognition to indicate
they
> are not object level initialization to be done once. If however the idea is to
> create an engine object each time you want a new recognition to happen, then
> Initialize/Cleanup should probably be part of the constructor and destructor.

Done.

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:37: virtual void
OnSpeechEngineResult(
On 2012/03/21 13:29:48, Satish wrote:
> SpeechEngine -> SpeechRecognitionEngine.
> Add comment describing when this is called, same for the next method

Done.

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:41: protected:
On 2012/03/21 13:29:48, Satish wrote:
> add newline above

Done.

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:47: // Prepare for upcoming
TakeAudioChunk() calls.
On 2012/03/21 13:29:48, Satish wrote:
> reword comment to explain this should be called when a new recognition process
> is about to begin

Done.

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:54: //
GetDesiredAudioChunkDurationMs(), and bytes_per_sample agrees to the engine
Removed the bytes_per_sample as it is DCHECKed. The DesiredAudioChunkDuration,
however, is not a strong requirement (e.g. it is violated in the last frame).
On 2012/03/21 13:29:48, Satish wrote:
> > 'where chunk length ...'
> No need to mention this in the comment if they are DCHECKed in the code

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/spee...
File content/browser/speech/speech_recognition_manager_impl.cc (right):

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/spee...
content/browser/speech/speech_recognition_manager_impl.cc:208:
requests_[caller_id].recognizer->StartRecognition();
On 2012/03/21 13:29:48, Satish wrote:
> I see the checks present here before have now moved to OnRecognitionError.
Does
> this preserve the callbacks as they work today? In particular are these no-mic
> and mic-in-use errors handled transparently allowing user to retry without
> issuing callbacks to the listeners/webkit/javascript as they are today?

Yes. Errors are not propagated to the dispatcher.

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/spee...
content/browser/speech/speech_recognition_manager_impl.cc:279: if
(recording_caller_id_ != caller_id)
On 2012/03/21 13:29:48, Satish wrote:
> this check shouldn't be here as we have a DCHECK for the same below
Basically this checks were introduced because the new recognizer implementation
issues the *End events also in case of errors (in that case recording_caller_id
becomes 0).
This code is, however, transitional and will be much more clean in the next CLs.

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/spee...
content/browser/speech/speech_recognition_manager_impl.cc:283: if
(!requests_[caller_id].is_active)
On 2012/03/21 13:29:48, Satish wrote:
> any reason why this check was added? seems like an error case.. may be a
DCHECK
> is more appropriate, but then if caller_id is the same as recording_caller_id_
> then it should be active anyway so are we doing two checks for the same time?

Same as above. We could receive an AudioEnd of a recognition that ended with an
error, just a while after we started a new recognition (this happens basically
if the user clicks very fast multiple times on the UI).
As above, this is just a transitional, not-so-clean patch to mix the new
recognizer code while keeping the current manager (that will be replaced in the
next CLs).

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/spee...
content/browser/speech/speech_recognition_manager_impl.cc:292: if
(!HasPendingRequest(caller_id) || !requests_[caller_id].is_active)
On 2012/03/21 13:29:48, Satish wrote:
> ditto, could be a DCHECK if you really want to check for this
Same as above.

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/spee...
File content/browser/speech/speech_recognizer_impl.h (right):

http://codereview.chromium.org/9663066/diff/14001/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.h:63: enum FSMState {
On 2012/03/21 13:29:48, Satish wrote:
> could the FSM changes be moved to a separate CL as that is conceptually
separate
> from the rest of renames and cleanup? Also would be easier to review

Done.

http://codereview.chromium.org/9663066/diff/14001/content/public/common/speec...
File content/public/common/speech_recognition_result.h (right):

http://codereview.chromium.org/9663066/diff/14001/content/public/common/speec...
content/public/common/speech_recognition_result.h:60: int details;
On 2012/03/21 13:29:48, Satish wrote:
> I see the above enum is being passed via this member. Can we change the int to
> the enum instead?
The problem is that details is a specialization of (and depends on) the error
code, but we cannot know it at compile time. Of course, currently we use details
only for AudioError, but we might soon use it to specialize also other error
codes.

[Satish, 2012-03-22 12:03:27]

Getting close..

http://codereview.chromium.org/9663066/diff/30003/content/browser/speech/spee...
File content/browser/speech/speech_recognition_engine.h (right):

http://codereview.chromium.org/9663066/diff/30003/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:42: virtual void
OnSpeechRecognitionEngineError(
also add a comment mentioning whether recognition automatically gets stopped on
error or if the caller has to call EndRecognition specifically after receiving
an error call

http://codereview.chromium.org/9663066/diff/30003/content/browser/speech/spee...
File content/browser/speech/speech_recognizer_impl.cc (left):

http://codereview.chromium.org/9663066/diff/30003/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.cc:173:
listener_->OnSoundEnd(caller_id_);
I don't see OnSoundEnd called from this class anymore. Should this deleted line
be added back?

http://codereview.chromium.org/9663066/diff/30003/content/browser/speech/spee...
File content/browser/speech/speech_recognizer_impl.cc (right):

http://codereview.chromium.org/9663066/diff/30003/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.cc:62: // TODO(primiano)
transitional, see description in speech_recognizer.h.
this doesn't seem useful, could remove

http://codereview.chromium.org/9663066/diff/30003/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.cc:84: new
speech::GoogleOneShotRemoteEngine(context_getter);
is this code to create google_sr_engine necessary? I don't see it used anywhere
in this function and you are creating this again in HandleOnData

http://codereview.chromium.org/9663066/diff/30003/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.cc:116: recognition_engine_(NULL),
this isn't required as it is a scoped_ptr

http://codereview.chromium.org/9663066/diff/30003/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.cc:125: DCHECK(listener_ != NULL);
is this DCHECK required? There are other pointer params you don't check for

http://codereview.chromium.org/9663066/diff/30003/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.cc:158:
GoogleOneShotRemoteEngine::kAudioPacketIntervalMs / 1000;
this doesn't seem right, having sampling rate as part of this file but packet
interval as part of the one shot reco engine. Can we move that to here as well?

http://codereview.chromium.org/9663066/diff/30003/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.cc:179:
recognition_engine_.reset();
is it required to call EndRecognition here or does .reset cleanly shutdown
things?

http://codereview.chromium.org/9663066/diff/30003/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.cc:254:
GoogleOneShotRemoteEngineConfig google_sr_config;
suggest moving all this to a separate helper function and call that from here to
initialize recognition_engine_

[Primiano Tucci (use gerrit), 2012-03-22 12:39:29]

http://codereview.chromium.org/9663066/diff/30003/content/browser/speech/goog...
File content/browser/speech/google_one_shot_remote_engine.cc (right):

http://codereview.chromium.org/9663066/diff/30003/content/browser/speech/goog...
content/browser/speech/google_one_shot_remote_engine.cc:83: error->code =
content::SPEECH_RECOGNITION_ERROR_NONE;
This was mistakenly added after the first steps of the review. This code should
not clear the error (which is set to NETWORK_ERROR before the call) because in
cause of parse failures (thus a return false) we should keep it. In case of
success, instead, the error code is not used at all (it is just a local variable
in OnURLFetchComplete).

http://codereview.chromium.org/9663066/diff/30003/content/browser/speech/spee...
File content/browser/speech/speech_recognition_engine.h (right):

http://codereview.chromium.org/9663066/diff/30003/content/browser/speech/spee...
content/browser/speech/speech_recognition_engine.h:42: virtual void
OnSpeechRecognitionEngineError(
On 2012/03/22 12:03:28, Satish wrote:
> also add a comment mentioning whether recognition automatically gets stopped
on
> error or if the caller has to call EndRecognition specifically after receiving
> an error call
It was described a few lines above. BTW I added also a comment on the
EndRecognition method.

http://codereview.chromium.org/9663066/diff/30003/content/browser/speech/spee...
File content/browser/speech/speech_recognizer_impl.cc (left):

http://codereview.chromium.org/9663066/diff/30003/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.cc:173:
listener_->OnSoundEnd(caller_id_);
On 2012/03/22 12:03:28, Satish wrote:
> I don't see OnSoundEnd called from this class anymore. Should this deleted
line
> be added back?

Done.

http://codereview.chromium.org/9663066/diff/30003/content/browser/speech/spee...
File content/browser/speech/speech_recognizer_impl.cc (right):

http://codereview.chromium.org/9663066/diff/30003/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.cc:62: // TODO(primiano)
transitional, see description in speech_recognizer.h.
On 2012/03/22 12:03:28, Satish wrote:
> this doesn't seem useful, could remove

Done.

http://codereview.chromium.org/9663066/diff/30003/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.cc:84: new
speech::GoogleOneShotRemoteEngine(context_getter);
On 2012/03/22 12:03:28, Satish wrote:
> is this code to create google_sr_engine necessary? I don't see it used
anywhere
> in this function and you are creating this again in HandleOnData
Oops. It was a "remainder" of the old code. Removed.

http://codereview.chromium.org/9663066/diff/30003/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.cc:116: recognition_engine_(NULL),
On 2012/03/22 12:03:28, Satish wrote:
> this isn't required as it is a scoped_ptr

Done.

http://codereview.chromium.org/9663066/diff/30003/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.cc:125: DCHECK(listener_ != NULL);
On 2012/03/22 12:03:28, Satish wrote:
> is this DCHECK required? There are other pointer params you don't check for
The only othher pointer I see is context_getter, that might be NULL (it is,
indeed, during tests)

http://codereview.chromium.org/9663066/diff/30003/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.cc:158:
GoogleOneShotRemoteEngine::kAudioPacketIntervalMs / 1000;
On 2012/03/22 12:03:28, Satish wrote:
> this doesn't seem right, having sampling rate as part of this file but packet
> interval as part of the one shot reco engine. Can we move that to here as
well?
Hmm this is transitional code, I know it is not clean.
The clean way is inquiring the recognition_engine through
GetDesiredAudioChunkDuration(). However, in this intermediate CL we instantiate
the engine on-demand (is not injected as it should be), thus it is NULL at this
point. Therefore I need this (very temporary) dirty patch, just in this CL.

http://codereview.chromium.org/9663066/diff/30003/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.cc:179:
recognition_engine_.reset();
On 2012/03/22 12:03:28, Satish wrote:
> is it required to call EndRecognition here or does .reset cleanly shutdown
> things?
I didn't make a lot of changes in this CL as this code is temporary. BTW,
currently the RecognitionEnd just destroys the url_request in the engine, and
calling a dtor has the same effect. So in this (temporary) situation, that
indeed violates the spech_recognition_engine contract, we get the same effect.

http://codereview.chromium.org/9663066/diff/30003/content/browser/speech/spee...
content/browser/speech/speech_recognizer_impl.cc:254:
GoogleOneShotRemoteEngineConfig google_sr_config;
On 2012/03/22 12:03:28, Satish wrote:
> suggest moving all this to a separate helper function and call that from here
to
> initialize recognition_engine_
Same as above, this code is temporary and the next CL will completely change it.

[Satish, 2012-03-22 14:37:21]

LGTM

Please also wait for Hans to take a look before submit

http://codereview.chromium.org/9663066/diff/34004/content/browser/speech/goog...
File content/browser/speech/google_one_shot_remote_engine.cc (right):

http://codereview.chromium.org/9663066/diff/34004/content/browser/speech/goog...
content/browser/speech/google_one_shot_remote_engine.cc:112: // For now we
support only single shot recognition, so we are giving only a
add newline above full length comments like these

[hans, 2012-03-22 16:25:59]

On 2012/03/22 14:37:21, Satish wrote:
> LGTM
> 
> Please also wait for Hans to take a look before submit
> 
>
http://codereview.chromium.org/9663066/diff/34004/content/browser/speech/goog...
> File content/browser/speech/google_one_shot_remote_engine.cc (right):
> 
>
http://codereview.chromium.org/9663066/diff/34004/content/browser/speech/goog...
> content/browser/speech/google_one_shot_remote_engine.cc:112: // For now we
> support only single shot recognition, so we are giving only a
> add newline above full length comments like these

LGTM, but the cl description needs to be updated.

[Primiano Tucci (use gerrit), 2012-03-22 17:19:04]

http://codereview.chromium.org/9663066/diff/34004/content/browser/speech/goog...
File content/browser/speech/google_one_shot_remote_engine.cc (right):

http://codereview.chromium.org/9663066/diff/34004/content/browser/speech/goog...
content/browser/speech/google_one_shot_remote_engine.cc:112: // For now we
support only single shot recognition, so we are giving only a
On 2012/03/22 14:37:22, Satish wrote:
> add newline above full length comments like these

Done.

[commit-bot: I haz the power, 2012-03-22 17:19:25]

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

[commit-bot: I haz the power, 2012-03-22 17:19:38]

Presubmit check for 9663066-22005 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
    content/public/browser

Presubmit checks took 2.2s to calculate.

[jam, 2012-03-22 17:31:08]

http://codereview.chromium.org/9663066/diff/22005/content/public/common/speec...
File content/public/common/speech_recognition_result.h (right):

http://codereview.chromium.org/9663066/diff/22005/content/public/common/speec...
content/public/common/speech_recognition_result.h:58: struct CONTENT_EXPORT
SpeechRecognitionError {
please see http://www.chromium.org/developers/content-module/content-api for
conventions for the Content API

in particular, the enum values should be named
SPEECH_AUDIO_ERROR_DETAILS_UNKNOWN.. and the struct below needs to be in its own
file. if you can avoid a cc file without clang errors, that'd be better

[Primiano Tucci (use gerrit), 2012-03-23 10:52:19]

http://codereview.chromium.org/9663066/diff/22008/content/public/common/speec...
File content/public/common/speech_recognition_error.h (right):

http://codereview.chromium.org/9663066/diff/22008/content/public/common/speec...
content/public/common/speech_recognition_error.h:30: enum
SpeechAudioErrorDetails {
I know that the presence of SpeechAudioErrorDetails in this file might seem
superfluous. However, in the next CLs it will be required by files under
/chrome/browser/.

[Satish, 2012-03-23 10:57:45]

http://codereview.chromium.org/9663066/diff/22008/content/public/common/speec...
File content/public/common/speech_recognition_error.h (right):

http://codereview.chromium.org/9663066/diff/22008/content/public/common/speec...
content/public/common/speech_recognition_error.h:30: enum
SpeechAudioErrorDetails {
On 2012/03/23 10:52:19, Primiano Tucci wrote:
> I know that the presence of SpeechAudioErrorDetails in this file might seem
> superfluous. However, in the next CLs it will be required by files under
> /chrome/browser/.

Can this be added in the next CLs where it is actually used?

[Primiano Tucci (use gerrit), 2012-03-23 11:09:54]

> Can this be added in the next CLs where it is actually used?
It already used currently. However, in this CL is used only partly, and only
inside content.

[jam, 2012-03-23 15:53:29]

lgtm

http://codereview.chromium.org/9663066/diff/24018/content/public/common/speec...
File content/public/common/speech_recognition_error.h (right):

http://codereview.chromium.org/9663066/diff/24018/content/public/common/speec...
content/public/common/speech_recognition_error.h:40:
SpeechRecognitionError(SpeechRecognitionErrorCode code_val)
nit: code_value (and also below). see google style guide

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

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

[commit-bot: I haz the power, 2012-03-23 16:57:25]

Try job failure for 9663066-25046 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-03-23 17:09:07]

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

[commit-bot: I haz the power, 2012-03-23 18:13:34]

Try job failure for 9663066-24047 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-03-26 08:47:21]

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

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

Try job failure for 9663066-41001 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-03-26 09:55:23]

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

[commit-bot: I haz the power, 2012-03-26 11:56:03]

Change committed as 128896