Applying changes to the existing speech input code to support the extension API.

Applying changes to the existing speech input code to support the extension API.

This includes extended error management by handling the status response code and the DidStartReceivingSpeech/DidStopReceivingSpeech events.


BUG=97388
TEST=none

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

[Satish, 2011-10-04 20:36:33]

Can you give a full description of the changes in the CL description? Also

http://codereview.chromium.org/8137005/diff/3001/content/browser/speech/speec...
File content/browser/speech/speech_input_manager.h (right):

http://codereview.chromium.org/8137005/diff/3001/content/browser/speech/speec...
content/browser/speech/speech_input_manager.h:77: virtual void
DidSpeechInputStart(int caller_id);
please use the new OVERRIDE annotation for all overridden virtual functions in
the files you touch in this CL. See
https://sites.google.com/a/chromium.org/dev/developers/coding-style?pli=1#Met...
and
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...

http://codereview.chromium.org/8137005/diff/3001/content/browser/speech/speec...
File content/browser/speech/speech_recognition_request.cc (right):

http://codereview.chromium.org/8137005/diff/3001/content/browser/speech/speec...
content/browser/speech/speech_recognition_request.cc:65:
response_object->GetWithoutPathExpansion(kStatusString, &status_value);
since the 'path' here is the first parameter and it doesn't have any dots, can't
we use the simpler "->Get()" method here? We probably don't need the above check
as well and could just check the return value of ->Get() like we do for
kHypothesesString below.

I also wonder if we could use ->GetAsInteger directly here and check the return
value of that to simplify all of this. Could you check if that fails when the
value is not an integer?

http://codereview.chromium.org/8137005/diff/3001/content/browser/speech/speec...
content/browser/speech/speech_recognition_request.cc:70: <<
status_value->GetType();
return false here?

http://codereview.chromium.org/8137005/diff/3001/content/browser/speech/speec...
content/browser/speech/speech_recognition_request.cc:139: result->status =
speech_input::kStatusAborted;
is kStatusAborted the right error code? seems like it should be something like
'protocol error'

http://codereview.chromium.org/8137005/diff/3001/content/browser/speech/speec...
File content/browser/speech/speech_recognizer.cc (right):

http://codereview.chromium.org/8137005/diff/3001/content/browser/speech/speec...
content/browser/speech/speech_recognizer.cc:225: bool speech_started =
endpointer_.DidStartReceivingSpeech();
suggest renaming to speech_was_heard to indicate this is the state before the
new audio packet is processed

http://codereview.chromium.org/8137005/diff/3001/content/browser/speech/speec...
content/browser/speech/speech_recognizer.cc:263: if
(!endpointer_.DidStartReceivingSpeech() &&
perhaps assign the value of DidStartReceivingSpeech to a new bool here and use
that in this if statement and below?

http://codereview.chromium.org/8137005/diff/3001/content/browser/speech/speec...
content/browser/speech/speech_recognizer.cc:293:
delegate_->DidSpeechInputStop(caller_id_);
instead of invoking this here do it inside StopRecording() so if someone else
calls that directly this callback will still get invoked

http://codereview.chromium.org/8137005/diff/3001/content/browser/speech/speec...
content/browser/speech/speech_recognizer.cc:340: StopRecording();
why is this call required here? wouldn't recognition have stopped well before
the result was received?

http://codereview.chromium.org/8137005/diff/3001/content/browser/speech/speec...
File content/browser/speech/speech_recognizer.h (right):

http://codereview.chromium.org/8137005/diff/3001/content/browser/speech/speec...
content/browser/speech/speech_recognizer.h:33: enum ErrorCode {
Can this enum be removed and we just use SpeechInputResultStatus instead?

http://codereview.chromium.org/8137005/diff/3001/content/browser/speech/speec...
content/browser/speech/speech_recognizer.h:40: RECOGNIZER_ERROR_INTERNAL,
since this is related to the protocol/parse error, i'd suggest naming it as
protocol_error, or use ERROR_NETWORK instead

http://codereview.chromium.org/8137005/diff/3001/content/browser/speech/speec...
content/browser/speech/speech_recognizer.h:66: virtual void
DidSpeechInputStart(int caller_id) = 0;
this would be clearer if renamed as DidStartReceivingSpeech and similarly the
one below

http://codereview.chromium.org/8137005/diff/3001/content/browser/speech/speec...
content/browser/speech/speech_recognizer.h:72: virtual void OnRecording(int
caller_id) = 0;
any reason why we need this when we have DidStartReceivingAudio? Is it really
important to inform about this and not wait for the first packet of audio (which
could be a more accurate representation of audio recording start) ?

http://codereview.chromium.org/8137005/diff/3001/content/common/speech_input_...
File content/common/speech_input_result.h (right):

http://codereview.chromium.org/8137005/diff/3001/content/common/speech_input_...
content/common/speech_input_result.h:28: SpeechInputHypothesisArray;
would be clearer to rename SpeechInputResultItem to SpeechInputHypothesis so
that the vector just adds an Array at the end of the name

http://codereview.chromium.org/8137005/diff/3001/content/common/speech_input_...
content/common/speech_input_result.h:40: struct SpeechInputResult {
can you add a constructor and initialize .status?

http://codereview.chromium.org/8137005/diff/3001/content/renderer/speech_inpu...
File content/renderer/speech_input_dispatcher.cc (right):

http://codereview.chromium.org/8137005/diff/3001/content/renderer/speech_inpu...
content/renderer/speech_input_dispatcher.cc:91: for (size_t i = 0; i <
result.hypotheses.size(); ++i)
suggest using braces for multiline for blocks such as this

[Leandro Graciá Gil, 2011-10-05 22:09:00]

http://codereview.chromium.org/8137005/diff/3001/content/browser/speech/speec...
File content/browser/speech/speech_input_manager.h (right):

http://codereview.chromium.org/8137005/diff/3001/content/browser/speech/speec...
content/browser/speech/speech_input_manager.h:77: virtual void
DidSpeechInputStart(int caller_id);
On 2011/10/04 20:36:33, Satish wrote:
> please use the new OVERRIDE annotation for all overridden virtual functions in
> the files you touch in this CL. See
>
https://sites.google.com/a/chromium.org/dev/developers/coding-style?pli=1#Met...
> and
>
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...

Done.

http://codereview.chromium.org/8137005/diff/3001/content/browser/speech/speec...
File content/browser/speech/speech_recognition_request.cc (right):

http://codereview.chromium.org/8137005/diff/3001/content/browser/speech/speec...
content/browser/speech/speech_recognition_request.cc:65:
response_object->GetWithoutPathExpansion(kStatusString, &status_value);
I think you're getting it the other way around. GetWithoutPathExpansion is
internally simpler as it doesn't check for any dots as Get does. Check
base/values.cc.

GetAsInteger might be used directly, and it will use Get internally and return
false in case of failure. I'll be using it.

On 2011/10/04 20:36:33, Satish wrote:
> since the 'path' here is the first parameter and it doesn't have any dots,
can't
> we use the simpler "->Get()" method here? We probably don't need the above
check
> as well and could just check the return value of ->Get() like we do for
> kHypothesesString below.
> 
> I also wonder if we could use ->GetAsInteger directly here and check the
return
> value of that to simplify all of this. Could you check if that fails when the
> value is not an integer?

http://codereview.chromium.org/8137005/diff/3001/content/browser/speech/speec...
content/browser/speech/speech_recognition_request.cc:70: <<
status_value->GetType();
On 2011/10/04 20:36:33, Satish wrote:
> return false here?

Done.

http://codereview.chromium.org/8137005/diff/3001/content/browser/speech/speec...
content/browser/speech/speech_recognition_request.cc:139: result->status =
speech_input::kStatusAborted;
Changed to network error since it seems the closest option for the status value
(which is following this:
http://www.w3.org/2005/Incubator/htmlspeech/2010/10/google-api-draft.html#spe...
)

On 2011/10/04 20:36:33, Satish wrote:
> is kStatusAborted the right error code? seems like it should be something like
> 'protocol error'

http://codereview.chromium.org/8137005/diff/3001/content/browser/speech/speec...
File content/browser/speech/speech_recognizer.cc (right):

http://codereview.chromium.org/8137005/diff/3001/content/browser/speech/speec...
content/browser/speech/speech_recognizer.cc:225: bool speech_started =
endpointer_.DidStartReceivingSpeech();
On 2011/10/04 20:36:33, Satish wrote:
> suggest renaming to speech_was_heard to indicate this is the state before the
> new audio packet is processed

Done.

http://codereview.chromium.org/8137005/diff/3001/content/browser/speech/speec...
content/browser/speech/speech_recognizer.cc:263: if
(!endpointer_.DidStartReceivingSpeech() &&
On 2011/10/04 20:36:33, Satish wrote:
> perhaps assign the value of DidStartReceivingSpeech to a new bool here and use
> that in this if statement and below?

Done.

http://codereview.chromium.org/8137005/diff/3001/content/browser/speech/speec...
content/browser/speech/speech_recognizer.cc:293:
delegate_->DidSpeechInputStop(caller_id_);
On 2011/10/04 20:36:33, Satish wrote:
> instead of invoking this here do it inside StopRecording() so if someone else
> calls that directly this callback will still get invoked

Done.

http://codereview.chromium.org/8137005/diff/3001/content/browser/speech/speec...
content/browser/speech/speech_recognizer.cc:340: StopRecording();
On 2011/10/04 20:36:33, Satish wrote:
> why is this call required here? wouldn't recognition have stopped well before
> the result was received?

Done.

http://codereview.chromium.org/8137005/diff/3001/content/browser/speech/speec...
File content/browser/speech/speech_recognizer.h (right):

http://codereview.chromium.org/8137005/diff/3001/content/browser/speech/speec...
content/browser/speech/speech_recognizer.h:40: RECOGNIZER_ERROR_INTERNAL,
I'll go for network error as it's an un expected value for the server response.

On 2011/10/04 20:36:33, Satish wrote:
> since this is related to the protocol/parse error, i'd suggest naming it as
> protocol_error, or use ERROR_NETWORK instead

http://codereview.chromium.org/8137005/diff/3001/content/browser/speech/speec...
content/browser/speech/speech_recognizer.h:66: virtual void
DidSpeechInputStart(int caller_id) = 0;
On 2011/10/04 20:36:33, Satish wrote:
> this would be clearer if renamed as DidStartReceivingSpeech and similarly the
> one below

Done.

http://codereview.chromium.org/8137005/diff/3001/content/browser/speech/speec...
content/browser/speech/speech_recognizer.h:72: virtual void OnRecording(int
caller_id) = 0;
Fixed. Now using only DidStartReceivingAudio after checking that it's properly
called even when the mic is muted.

On 2011/10/04 20:36:33, Satish wrote:
> any reason why we need this when we have DidStartReceivingAudio? Is it really
> important to inform about this and not wait for the first packet of audio
(which
> could be a more accurate representation of audio recording start) ?

http://codereview.chromium.org/8137005/diff/3001/content/common/speech_input_...
File content/common/speech_input_result.h (right):

http://codereview.chromium.org/8137005/diff/3001/content/common/speech_input_...
content/common/speech_input_result.h:28: SpeechInputHypothesisArray;
On 2011/10/04 20:36:33, Satish wrote:
> would be clearer to rename SpeechInputResultItem to SpeechInputHypothesis so
> that the vector just adds an Array at the end of the name

Done.

http://codereview.chromium.org/8137005/diff/3001/content/common/speech_input_...
content/common/speech_input_result.h:40: struct SpeechInputResult {
On 2011/10/04 20:36:33, Satish wrote:
> can you add a constructor and initialize .status?

Done.

http://codereview.chromium.org/8137005/diff/3001/content/renderer/speech_inpu...
File content/renderer/speech_input_dispatcher.cc (right):

http://codereview.chromium.org/8137005/diff/3001/content/renderer/speech_inpu...
content/renderer/speech_input_dispatcher.cc:91: for (size_t i = 0; i <
result.hypotheses.size(); ++i)
On 2011/10/04 20:36:33, Satish wrote:
> suggest using braces for multiline for blocks such as this

Done.

[Satish, 2011-10-06 09:09:06]

Can you do a try run as well?

http://codereview.chromium.org/8137005/diff/7001/chrome/browser/speech/chrome...
File chrome/browser/speech/chrome_speech_input_manager.h (right):

http://codereview.chromium.org/8137005/diff/7001/chrome/browser/speech/chrome...
chrome/browser/speech/chrome_speech_input_manager.h:35: virtual void
ShowWarmUp(int caller_id);
use OVERRIDE for these functions as well

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
File content/browser/speech/speech_input_browsertest.cc (right):

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
content/browser/speech/speech_input_browsertest.cc:121:
results.stable.push_back(SpeechInputResultItem(
did you mean '.hypotheses' here? I don't see a 'stable' property in
SpeechInputResult

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
File content/browser/speech/speech_input_manager.h (right):

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
content/browser/speech/speech_input_manager.h:71: virtual void
DidStartReceivingAudio(int caller_id);
OVERRIDE

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
File content/browser/speech/speech_recognition_request.cc (right):

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
content/browser/speech/speech_recognition_request.cc:58: if
(!response_object->HasKey(kStatusString)) {
Is this check required or is GetInteger returning false sufficient here?

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
content/browser/speech/speech_recognition_request.cc:66: VLOG(1) <<
"ParseServerResponse: get " << kStatusString
could print something like
  kStatusString << " is not a valid integer value."

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
content/browser/speech/speech_recognition_request.cc:134: result->error =
speech_input::kErrorNetwork;
See the next comment, if you go with that you can remove this line.

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
content/browser/speech/speech_recognition_request.cc:222: if (!error)
We could remove the 'bool error' parameter in the delegate method
SetRecognitionResult() and just use result.error

e.g.
SpeechInputResult result;
if (!source->status().is_success() ||
    source->response_code() != 200 ||
    ParseServerResponse(...) {
  result.error = kErrorNetwork;
}
...
delegate_->SetRecognitionResult(result);

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
File content/browser/speech/speech_recognizer.cc (right):

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
content/browser/speech/speech_recognizer.cc:284: // TODO(satish): Once we have
streaming POST, start sending the data received
This todo looks obsolete as we are uploading audio in chunks now (see
UploadAudioChunk() call above). Can you remove this as well?

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
File content/browser/speech/speech_recognizer.h (right):

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
content/browser/speech/speech_recognizer.h:55: // Informs that the end pointer
has started detecting input speech.
"input speech" -> "speech"

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
content/browser/speech/speech_recognizer.h:58: // Informs that the end pointer
has stopped detecting input speech.
ditto

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
content/browser/speech/speech_recognizer.h:118: const SpeechInputResult&
result);
could align with the previous param?

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
File content/browser/speech/speech_recognizer_unittest.cc (right):

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
content/browser/speech/speech_recognizer_unittest.cc:47: const
SpeechInputResult& result) {
use OVERRIDE for all these virtuals

http://codereview.chromium.org/8137005/diff/7001/content/common/speech_input_...
File content/common/speech_input_result.h (right):

http://codereview.chromium.org/8137005/diff/7001/content/common/speech_input_...
content/common/speech_input_result.h:28: SpeechInputHypothesisArray;
merge with previous line

http://codereview.chromium.org/8137005/diff/7001/content/common/speech_input_...
content/common/speech_input_result.h:30: enum SpeechInputError {
can you add a comment to each of these error codes explaining what they mean?

[Leandro Graciá Gil, 2011-10-06 18:26:25]

http://codereview.chromium.org/8137005/diff/7001/chrome/browser/speech/chrome...
File chrome/browser/speech/chrome_speech_input_manager.h (right):

http://codereview.chromium.org/8137005/diff/7001/chrome/browser/speech/chrome...
chrome/browser/speech/chrome_speech_input_manager.h:35: virtual void
ShowWarmUp(int caller_id);
On 2011/10/06 09:09:06, Satish wrote:
> use OVERRIDE for these functions as well

Done.

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
File content/browser/speech/speech_input_browsertest.cc (right):

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
content/browser/speech/speech_input_browsertest.cc:121:
results.stable.push_back(SpeechInputResultItem(
On 2011/10/06 09:09:06, Satish wrote:
> did you mean '.hypotheses' here? I don't see a 'stable' property in
> SpeechInputResult

Done.

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
File content/browser/speech/speech_input_manager.h (right):

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
content/browser/speech/speech_input_manager.h:71: virtual void
DidStartReceivingAudio(int caller_id);
On 2011/10/06 09:09:06, Satish wrote:
> OVERRIDE

Done.

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
File content/browser/speech/speech_recognition_request.cc (right):

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
content/browser/speech/speech_recognition_request.cc:58: if
(!response_object->HasKey(kStatusString)) {
On 2011/10/06 09:09:06, Satish wrote:
> Is this check required or is GetInteger returning false sufficient here?

Done.

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
content/browser/speech/speech_recognition_request.cc:66: VLOG(1) <<
"ParseServerResponse: get " << kStatusString
On 2011/10/06 09:09:06, Satish wrote:
> could print something like
>   kStatusString << " is not a valid integer value."

Done.

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
content/browser/speech/speech_recognition_request.cc:134: result->error =
speech_input::kErrorNetwork;
On 2011/10/06 09:09:06, Satish wrote:
> See the next comment, if you go with that you can remove this line.

Done.

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
content/browser/speech/speech_recognition_request.cc:222: if (!error)
On 2011/10/06 09:09:06, Satish wrote:
> We could remove the 'bool error' parameter in the delegate method
> SetRecognitionResult() and just use result.error
> 
> e.g.
> SpeechInputResult result;
> if (!source->status().is_success() ||
>     source->response_code() != 200 ||
>     ParseServerResponse(...) {
>   result.error = kErrorNetwork;
> }
> ...
> delegate_->SetRecognitionResult(result);

Done.

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
File content/browser/speech/speech_recognizer.cc (right):

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
content/browser/speech/speech_recognizer.cc:284: // TODO(satish): Once we have
streaming POST, start sending the data received
On 2011/10/06 09:09:06, Satish wrote:
> This todo looks obsolete as we are uploading audio in chunks now (see
> UploadAudioChunk() call above). Can you remove this as well?

Done.

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
File content/browser/speech/speech_recognizer.h (right):

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
content/browser/speech/speech_recognizer.h:55: // Informs that the end pointer
has started detecting input speech.
On 2011/10/06 09:09:06, Satish wrote:
> "input speech" -> "speech"

Done.

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
content/browser/speech/speech_recognizer.h:58: // Informs that the end pointer
has stopped detecting input speech.
On 2011/10/06 09:09:06, Satish wrote:
> ditto

Done.

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
content/browser/speech/speech_recognizer.h:118: const SpeechInputResult&
result);
On 2011/10/06 09:09:06, Satish wrote:
> could align with the previous param?

Done.

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
File content/browser/speech/speech_recognizer_unittest.cc (right):

http://codereview.chromium.org/8137005/diff/7001/content/browser/speech/speec...
content/browser/speech/speech_recognizer_unittest.cc:47: const
SpeechInputResult& result) {
On 2011/10/06 09:09:06, Satish wrote:
> use OVERRIDE for all these virtuals

Done.

http://codereview.chromium.org/8137005/diff/7001/content/common/speech_input_...
File content/common/speech_input_result.h (right):

http://codereview.chromium.org/8137005/diff/7001/content/common/speech_input_...
content/common/speech_input_result.h:28: SpeechInputHypothesisArray;
On 2011/10/06 09:09:06, Satish wrote:
> merge with previous line

Done.

http://codereview.chromium.org/8137005/diff/7001/content/common/speech_input_...
content/common/speech_input_result.h:30: enum SpeechInputError {
On 2011/10/06 09:09:06, Satish wrote:
> can you add a comment to each of these error codes explaining what they mean?

Done.

[Satish, 2011-10-06 20:30:12]

LGTM with comments below.

http://codereview.chromium.org/8137005/diff/16001/chrome/browser/speech/chrom...
File chrome/browser/speech/chrome_speech_input_manager.h (right):

http://codereview.chromium.org/8137005/diff/16001/chrome/browser/speech/chrom...
chrome/browser/speech/chrome_speech_input_manager.h:24:
SpeechInputBubble::Button button);
OVERRIDE these as well?

http://codereview.chromium.org/8137005/diff/16001/content/common/speech_input...
File content/common/speech_input_result.h (right):

http://codereview.chromium.org/8137005/diff/16001/content/common/speech_input...
content/common/speech_input_result.h:30: //
http://www.w3.org/2005/Incubator/htmlspeech/2010/10/ (continues below)
I think it is fine to have URLs > 80 chars, please check with existing code.

http://codereview.chromium.org/8137005/diff/16001/content/common/speech_input...
content/common/speech_input_result.h:46: SpeechInputResult() : error(kErrorNone)
{}
mac bot complains about this, needs to go into a .cc file

[Leandro Graciá Gil, 2011-10-06 22:00:28]

Fixing nits and trying the bots once more. Will land the patch if there are no
problems with the bots.

http://codereview.chromium.org/8137005/diff/16001/chrome/browser/speech/chrom...
File chrome/browser/speech/chrome_speech_input_manager.h (right):

http://codereview.chromium.org/8137005/diff/16001/chrome/browser/speech/chrom...
chrome/browser/speech/chrome_speech_input_manager.h:24:
SpeechInputBubble::Button button);
On 2011/10/06 20:30:12, Satish wrote:
> OVERRIDE these as well?

Done.

http://codereview.chromium.org/8137005/diff/16001/content/common/speech_input...
File content/common/speech_input_result.h (right):

http://codereview.chromium.org/8137005/diff/16001/content/common/speech_input...
content/common/speech_input_result.h:30: //
http://www.w3.org/2005/Incubator/htmlspeech/2010/10/ (continues below)
On 2011/10/06 20:30:12, Satish wrote:
> I think it is fine to have URLs > 80 chars, please check with existing code.

Done.

http://codereview.chromium.org/8137005/diff/16001/content/common/speech_input...
content/common/speech_input_result.h:46: SpeechInputResult() : error(kErrorNone)
{}
On 2011/10/06 20:30:12, Satish wrote:
> mac bot complains about this, needs to go into a .cc file

Done.