Adding protobuf headers that will be used by upcoming continuous speech recognition classes (Speech…

Adding protobuf headers that will be used by upcoming continuous speech recognition classes (Speech CL2.2)


BUG=116954
TEST=none


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

[jam, 2012-05-15 16:34:49]

nit: I had meant to put this in content/browser/speech/proto/speech_proto.gypi

[Primiano Tucci (use gerrit), 2012-05-15 16:55:21]

On 2012/05/15 16:34:49, John Abd-El-Malek wrote:
> nit: I had meant to put this in content/browser/speech/proto/speech_proto.gypi

Ah, sorry.
Submitted a new patchset.

[jam, 2012-05-15 21:17:01]

http://codereview.chromium.org/10384153/diff/3004/content/content.gyp
File content/content.gyp (right):

http://codereview.chromium.org/10384153/diff/3004/content/content.gyp#newcode12
content/content.gyp:12: 'browser/speech/proto/speech_proto.gypi',
why do you need to include this in both content.gyp and content_browser.gypi?

[Primiano Tucci (use gerrit), 2012-05-16 08:43:39]

http://codereview.chromium.org/10384153/diff/3004/content/content.gyp
File content/content.gyp (right):

http://codereview.chromium.org/10384153/diff/3004/content/content.gyp#newcode12
content/content.gyp:12: 'browser/speech/proto/speech_proto.gypi',
On 2012/05/15 21:17:01, John Abd-El-Malek wrote:
> why do you need to include this in both content.gyp and content_browser.gypi?
The file is included only in content.gyp. The speech_proto.gypi defines a gyp
target section, and it is my understanding that I cannot move this include
directly into content_browser.gypi because content_browser.gypi is included
within a target itself.
The other line inside content_browser.gypi is just a dependency on speech_proto,
which requires the corresponding target to be previously defined.

[hans, 2012-05-16 10:42:47]

http://codereview.chromium.org/10384153/diff/3004/content/browser/speech/prot...
File content/browser/speech/proto/google_streaming_api.proto (right):

http://codereview.chromium.org/10384153/diff/3004/content/browser/speech/prot...
content/browser/speech/proto/google_streaming_api.proto:9: //option
java_api_version = 2;
should these two commented out lines be here?

http://codereview.chromium.org/10384153/diff/3004/content/browser/speech/prot...
content/browser/speech/proto/google_streaming_api.proto:14: // Messages sent to
client.
not sure how helpful this comment is?

http://codereview.chromium.org/10384153/diff/3004/content/browser/speech/prot...
content/browser/speech/proto/google_streaming_api.proto:17: // All responses
must be an extension of this message.
hmm, but this this seems to be the only response message that gets defined?

http://codereview.chromium.org/10384153/diff/3004/content/browser/speech/prot...
content/browser/speech/proto/google_streaming_api.proto:20: // Required.  The
request_id of the RecognitionRequest that
if it's required, why is it marked optional?

http://codereview.chromium.org/10384153/diff/3004/content/browser/speech/prot...
content/browser/speech/proto/google_streaming_api.proto:25: optional string
provisional = 4;
could we try to comment this file a bit more extensively? maybe look for
inspiration in sync/protocol/sync.proto which is very nicely commented.

http://codereview.chromium.org/10384153/diff/3004/content/browser/speech/prot...
content/browser/speech/proto/google_streaming_api.proto:34: optional string
utterance = 1;
i like a blank line between fields when they have comments, to make it easier to
read and more obvious which field a comment belongs to

http://codereview.chromium.org/10384153/diff/3004/content/content.gyp
File content/content.gyp (right):

http://codereview.chromium.org/10384153/diff/3004/content/content.gyp#newcode12
content/content.gyp:12: 'browser/speech/proto/speech_proto.gypi',
On 2012/05/16 08:43:39, Primiano Tucci wrote:
> On 2012/05/15 21:17:01, John Abd-El-Malek wrote:
> > why do you need to include this in both content.gyp and
content_browser.gypi?
> The file is included only in content.gyp. The speech_proto.gypi defines a gyp
> target section, and it is my understanding that I cannot move this include
> directly into content_browser.gypi because content_browser.gypi is included
> within a target itself.
> The other line inside content_browser.gypi is just a dependency on
speech_proto,
> which requires the corresponding target to be previously defined.

Maybe speech_proto.gypi should actually be speec_proto.gyp? Then we could add a
dependency on it in chrome_browser.gypi. I'm not an expert on this gyp stuff,
though.

[Primiano Tucci (use gerrit), 2012-05-16 13:34:19]

http://codereview.chromium.org/10384153/diff/3004/content/browser/speech/prot...
File content/browser/speech/proto/google_streaming_api.proto (right):

http://codereview.chromium.org/10384153/diff/3004/content/browser/speech/prot...
content/browser/speech/proto/google_streaming_api.proto:9: //option
java_api_version = 2;
On 2012/05/16 10:42:47, hans wrote:
> should these two commented out lines be here?

Done.

http://codereview.chromium.org/10384153/diff/3004/content/browser/speech/prot...
content/browser/speech/proto/google_streaming_api.proto:14: // Messages sent to
client.
On 2012/05/16 10:42:47, hans wrote:
> not sure how helpful this comment is?
Right.

http://codereview.chromium.org/10384153/diff/3004/content/browser/speech/prot...
content/browser/speech/proto/google_streaming_api.proto:14: // Messages sent to
client.
On 2012/05/16 10:42:47, hans wrote:
> not sure how helpful this comment is?
Agree, removed.

http://codereview.chromium.org/10384153/diff/3004/content/browser/speech/prot...
content/browser/speech/proto/google_streaming_api.proto:17: // All responses
must be an extension of this message.
On 2012/05/16 10:42:47, hans wrote:
> hmm, but this this seems to be the only response message that gets defined?
Agree. removed.

http://codereview.chromium.org/10384153/diff/3004/content/browser/speech/prot...
content/browser/speech/proto/google_streaming_api.proto:20: // Required.  The
request_id of the RecognitionRequest that
On 2012/05/16 10:42:47, hans wrote:
> if it's required, why is it marked optional?
Ha. To be honest either its purpose is not completely clear to me. Added a TODO.

http://codereview.chromium.org/10384153/diff/3004/content/browser/speech/prot...
content/browser/speech/proto/google_streaming_api.proto:25: optional string
provisional = 4;
On 2012/05/16 10:42:47, hans wrote:
> could we try to comment this file a bit more extensively? maybe look for
> inspiration in sync/protocol/sync.proto which is very nicely commented.

Done.

http://codereview.chromium.org/10384153/diff/3004/content/browser/speech/prot...
content/browser/speech/proto/google_streaming_api.proto:34: optional string
utterance = 1;
On 2012/05/16 10:42:47, hans wrote:
> i like a blank line between fields when they have comments, to make it easier
to
> read and more obvious which field a comment belongs to

Done.

http://codereview.chromium.org/10384153/diff/3004/content/content.gyp
File content/content.gyp (right):

http://codereview.chromium.org/10384153/diff/3004/content/content.gyp#newcode12
content/content.gyp:12: 'browser/speech/proto/speech_proto.gypi',
On 2012/05/16 10:42:47, hans wrote:
> On 2012/05/16 08:43:39, Primiano Tucci wrote:
> > On 2012/05/15 21:17:01, John Abd-El-Malek wrote:
> > > why do you need to include this in both content.gyp and
> content_browser.gypi?
> > The file is included only in content.gyp. The speech_proto.gypi defines a
gyp
> > target section, and it is my understanding that I cannot move this include
> > directly into content_browser.gypi because content_browser.gypi is included
> > within a target itself.
> > The other line inside content_browser.gypi is just a dependency on
> speech_proto,
> > which requires the corresponding target to be previously defined.
> 
> Maybe speech_proto.gypi should actually be speec_proto.gyp? Then we could add
a
> dependency on it in chrome_browser.gypi. I'm not an expert on this gyp stuff,
> though.
Nice! Seems to work requiring no edits to content.gyp.

[hans, 2012-05-16 13:49:13]

LGTM

http://codereview.chromium.org/10384153/diff/15001/content/browser/speech/pro...
File content/browser/speech/proto/google_streaming_api.proto (right):

http://codereview.chromium.org/10384153/diff/15001/content/browser/speech/pro...
content/browser/speech/proto/google_streaming_api.proto:39: // Used for
receiving and hypothesis for a definitive result.
"used for receiving and" sounds weird

[jam, 2012-05-16 15:10:28]

lgtm

[commit-bot: I haz the power, 2012-05-16 15:21:05]

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

[commit-bot: I haz the power, 2012-05-16 18:01:47]

Change committed as 137454