Speech refactoring: Turned AudioChunk into a refcounted class (CL1.4)

Speech refactoring: Turned AudioChunk into a refcounted class (CL1.4)

BUG=116954
TEST=Run content_unittests.


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

[Primiano Tucci (use gerrit), 2012-03-27 15:24:46]

Note: the previous outstanding CL 9835049 has been renamed to CL1.5, in order to
make room for this intermediate CL.

[hans, 2012-03-27 15:56:31]

https://chromiumcodereview.appspot.com/9861019/diff/2001/content/browser/spee...
File content/browser/speech/audio_buffer.cc (right):

https://chromiumcodereview.appspot.com/9861019/diff/2001/content/browser/spee...
content/browser/speech/audio_buffer.cc:57: chunks_.push_back(chunk);
could we just do this on a single line?

chunks_.push_back(new AudioChunk(data, length, bytes_per_sample_));

https://chromiumcodereview.appspot.com/9861019/diff/2001/content/browser/spee...
content/browser/speech/audio_buffer.cc:70: ChunksContainer::iterator it;
hmm, why isn't it a const_iterator anymore?

https://chromiumcodereview.appspot.com/9861019/diff/2001/content/browser/spee...
File content/browser/speech/audio_buffer.h (right):

https://chromiumcodereview.appspot.com/9861019/diff/2001/content/browser/spee...
content/browser/speech/audio_buffer.h:10: #include <deque>
nit: header includes should be sorted alphabetically

https://chromiumcodereview.appspot.com/9861019/diff/2001/content/browser/spee...
content/browser/speech/audio_buffer.h:20: public
base::RefCountedThreadSafe<AudioChunk> {
just to double check: it needs to be thread safe because we pass this between
the IO and Audio threads?

https://chromiumcodereview.appspot.com/9861019/diff/2001/content/browser/spee...
File content/browser/speech/google_one_shot_remote_engine_unittest.cc (right):

https://chromiumcodereview.appspot.com/9861019/diff/2001/content/browser/spee...
content/browser/speech/google_one_shot_remote_engine_unittest.cc:57:
client.TakeAudioChunk(*dummy_audio_chunk);
hmm, i'm trying to think whether passing the AudioChunk by constref to
TakeAudioChunk still makes sense..

[Primiano Tucci (use gerrit), 2012-03-27 16:51:24]

http://codereview.chromium.org/9861019/diff/2001/content/browser/speech/audio...
File content/browser/speech/audio_buffer.cc (right):

http://codereview.chromium.org/9861019/diff/2001/content/browser/speech/audio...
content/browser/speech/audio_buffer.cc:57: chunks_.push_back(chunk);
On 2012/03/27 15:56:31, hans wrote:
> could we just do this on a single line?
> 
> chunks_.push_back(new AudioChunk(data, length, bytes_per_sample_));
Uh, right. It was a remainder of the old code :)

http://codereview.chromium.org/9861019/diff/2001/content/browser/speech/audio...
content/browser/speech/audio_buffer.cc:70: ChunksContainer::iterator it;
On 2012/03/27 15:56:31, hans wrote:
> hmm, why isn't it a const_iterator anymore?

Ah right. I tried to use a std::queue at beginning, and then forgot to revert
the iterator to const_.

http://codereview.chromium.org/9861019/diff/2001/content/browser/speech/audio...
File content/browser/speech/audio_buffer.h (right):

http://codereview.chromium.org/9861019/diff/2001/content/browser/speech/audio...
content/browser/speech/audio_buffer.h:10: #include <deque>
On 2012/03/27 15:56:31, hans wrote:
> nit: header includes should be sorted alphabetically
Oops.

http://codereview.chromium.org/9861019/diff/2001/content/browser/speech/audio...
content/browser/speech/audio_buffer.h:20: public
base::RefCountedThreadSafe<AudioChunk> {
On 2012/03/27 15:56:31, hans wrote:
> just to double check: it needs to be thread safe because we pass this between
> the IO and Audio threads?
Right, it might happen that it is released and acquired simultaneously by,
respectively, IO and Audio threads in speech_recognizer_impl.cc

http://codereview.chromium.org/9861019/diff/2001/content/browser/speech/googl...
File content/browser/speech/google_one_shot_remote_engine_unittest.cc (right):

http://codereview.chromium.org/9861019/diff/2001/content/browser/speech/googl...
content/browser/speech/google_one_shot_remote_engine_unittest.cc:57:
client.TakeAudioChunk(*dummy_audio_chunk);
On 2012/03/27 15:56:31, hans wrote:
> hmm, i'm trying to think whether passing the AudioChunk by constref to
> TakeAudioChunk still makes sense..
I kept the const ref for all those methods that don't use/keep the chunk after
returning from the call. After all, if they just use it synchronously, they
don't need to know any detail about the memory ownership of that audio chunk.

[hans, 2012-03-27 17:13:04]

lgtm

[commit-bot: I haz the power, 2012-03-27 17:21:08]

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

[commit-bot: I haz the power, 2012-03-28 00:32:10]

Change committed as 129316