Update the Speech Api to support array(s) of result items

Update the Speech Api to support array(s) of result items
instead of a single item at a time.

BUG=143124
TEST=Covered by content_unittests

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

[tommi (sloooow) - chröme, 2012-11-28 13:22:39]

This is based on a pending change in WebKit so I won't land this until WebKit
has been rolled.  Please ping me if you need the relevant patch.

[hans, 2012-11-28 13:33:35]

Great, I'll take a look at this now.

Just a quick note: I think the BUG= field should be pointing to crbug.com/143124

[hans, 2012-11-28 14:15:34]

Looks great!

LGTM with some minor nits.

https://codereview.chromium.org/11421103/diff/1003/content/browser/speech/goo...
File content/browser/speech/google_streaming_remote_engine_unittest.cc (right):

https://codereview.chromium.org/11421103/diff/1003/content/browser/speech/goo...
content/browser/speech/google_streaming_remote_engine_unittest.cc:466: if
((*it_a).is_provisional != (*it_b).is_provisional ||
nit: instead if (*it_a).is_provisional, I'd have gone for it_a->is_provisional

It might even be worth doing
const SpeechRecognitionResult& res_a = *it_a;

to simplify the code below a bit.

https://codereview.chromium.org/11421103/diff/1003/content/browser/speech/inp...
File content/browser/speech/input_tag_speech_dispatcher_host.cc (right):

https://codereview.chromium.org/11421103/diff/1003/content/browser/speech/inp...
content/browser/speech/input_tag_speech_dispatcher_host.cc:123: VLOG(1) <<
"InputTagSpeechDispatcherHost::OnRecognitionResult enter";
ultra nit: while here, we might as well update this VLOG (and the one at the end
of the function).

Oh, and maybe update them to DVLOG()'s as you did in the other file.

https://codereview.chromium.org/11421103/diff/1003/content/browser/speech/spe...
File content/browser/speech/speech_recognizer.cc (right):

https://codereview.chromium.org/11421103/diff/1003/content/browser/speech/spe...
content/browser/speech/speech_recognizer.cc:568: const SpeechRecognitionResult&
result = (*i);
ultra nit: parenthesis around *i seems a bit overkill to me

https://codereview.chromium.org/11421103/diff/1003/content/common/speech_reco...
File content/common/speech_recognition_messages.h (right):

https://codereview.chromium.org/11421103/diff/1003/content/common/speech_reco...
content/common/speech_recognition_messages.h:84:
content::SpeechRecognitionResults /* result */)
nit: maybe update the /* result */ part too? applies below as well.

[Charlie Reis, 2012-11-29 02:21:42]

I'm not familiar with any of this code, so I can't do much of a review, and at
best could only give a rubber stamp approval.  Are any of the other content/
owners better suited for this?

[hans, 2012-11-29 08:46:53]

On 2012/11/29 02:21:42, creis wrote:
> I'm not familiar with any of this code, so I can't do much of a review, and at
> best could only give a rubber stamp approval.  Are any of the other content/
> owners better suited for this?

I think a rubber stamp is sufficient here.

The changes that are not in content/browser/speech/ (in which I and tommi are
both owners) are just mechanical, except for
content/renderer/speech_recognition_dispatcher.cc which looks good to me from a
speech perspective.

[tommi (sloooow) - chröme, 2012-11-29 09:30:20]

all done.

creis: We just need a rubber stamp approval from content owners so unless you
don't feel comfortable with reviewing this at all, then I'd like to ask you to
please take a look.
In essence, this change is about changing a single parameter from being a
SpeechRecognitionResult (one item) to SpeechRecognitionResults (a vector of such
items).  Since this is an implementation of a WebKit API, it touches a lot of
files, but the change in itself isn't super complicated.

https://codereview.chromium.org/11421103/diff/1003/content/browser/speech/goo...
File content/browser/speech/google_streaming_remote_engine_unittest.cc (right):

https://codereview.chromium.org/11421103/diff/1003/content/browser/speech/goo...
content/browser/speech/google_streaming_remote_engine_unittest.cc:466: if
((*it_a).is_provisional != (*it_b).is_provisional ||
On 2012/11/28 14:15:35, hans wrote:
> nit: instead if (*it_a).is_provisional, I'd have gone for it_a->is_provisional
> 
> It might even be worth doing
> const SpeechRecognitionResult& res_a = *it_a;
> 
> to simplify the code below a bit.

Done.  I just changed (*foo). to foo-> since I figured that |it_a->| would be as
readable as |res_a.|

https://codereview.chromium.org/11421103/diff/1003/content/browser/speech/inp...
File content/browser/speech/input_tag_speech_dispatcher_host.cc (right):

https://codereview.chromium.org/11421103/diff/1003/content/browser/speech/inp...
content/browser/speech/input_tag_speech_dispatcher_host.cc:123: VLOG(1) <<
"InputTagSpeechDispatcherHost::OnRecognitionResult enter";
On 2012/11/28 14:15:35, hans wrote:
> ultra nit: while here, we might as well update this VLOG (and the one at the
end
> of the function).
> 
> Oh, and maybe update them to DVLOG()'s as you did in the other file.

Done.

https://codereview.chromium.org/11421103/diff/1003/content/browser/speech/spe...
File content/browser/speech/speech_recognizer.cc (right):

https://codereview.chromium.org/11421103/diff/1003/content/browser/speech/spe...
content/browser/speech/speech_recognizer.cc:568: const SpeechRecognitionResult&
result = (*i);
On 2012/11/28 14:15:35, hans wrote:
> ultra nit: parenthesis around *i seems a bit overkill to me

Done.

https://codereview.chromium.org/11421103/diff/1003/content/common/speech_reco...
File content/common/speech_recognition_messages.h (right):

https://codereview.chromium.org/11421103/diff/1003/content/common/speech_reco...
content/common/speech_recognition_messages.h:84:
content::SpeechRecognitionResults /* result */)
On 2012/11/28 14:15:35, hans wrote:
> nit: maybe update the /* result */ part too? applies below as well.

Done.

[Charlie Reis, 2012-11-29 17:52:20]

Ok.  content/ LGTM.

[tommi (sloooow) - chröme, 2012-11-30 21:13:18]

+jschuh for messages.h file.

[jschuh, 2012-11-30 21:26:07]

Looks like just opaque types and no custom serialization. Seems safe. lgtm.

[commit-bot: I haz the power, 2012-12-01 12:09:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommi@chromium.org/11421103/9027

[commit-bot: I haz the power, 2012-12-01 12:37:59]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-12-01 14:21:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommi@chromium.org/11421103/4036

[commit-bot: I haz the power, 2012-12-01 18:10:15]

Change committed as 170668