Cleanup media::SeekableBuffer and unit tests...

Cleanup media::SeekableBuffer and unit tests
TEST=SeekableBufferTest.*
Updating media::SeekableBuffer and unit tests to meet
code style standards.


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

[Alpha Left Google, 2009-05-26 18:03:32]

http://codereview.chromium.org/114042/diff/3008/2013
File media/base/seekable_buffer.cc (right):

http://codereview.chromium.org/114042/diff/3008/2013#newcode193
Line 193: 
This space is just to get rietveld to accept this file, will remove this once
changes are made to this file.

http://codereview.chromium.org/114042/diff/3008/2012
File media/base/seekable_buffer.h (right):

http://codereview.chromium.org/114042/diff/3008/2012#newcode94
Line 94: // A structure that contains a block of data.
Definition of struct is moved on top.

http://codereview.chromium.org/114042/diff/3008/2012#newcode118
Line 118: typedef std::list<Buffer*> BufferQueue;
Should the the typedef for the STL container be defined just above the first
use?

http://codereview.chromium.org/114042/diff/3008/2012#newcode130
Line 130: DISALLOW_COPY_AND_ASSIGN(SeekableBuffer);
added disallow_copy_and_assign

[Jon Perkins, 2009-05-27 19:25:14]

As I said before, the code's fairly clean.  Most of my concerns seem to be tied
to comments and grammar.  :)

http://codereview.chromium.org/114042/diff/3008/2013
File media/base/seekable_buffer.cc (right):

http://codereview.chromium.org/114042/diff/3008/2013#newcode85
Line 85: // only be invalid before any data is appended, this case should be
handled
Run-on sentence.

http://codereview.chromium.org/114042/diff/3008/2013#newcode89
Line 89: // We try to at most |size| bytes in the backward direction, we also
have
Please fix the wording here.  Try to what?

http://codereview.chromium.org/114042/diff/3008/2013#newcode89
Line 89: // We try to at most |size| bytes in the backward direction, we also
have
Run-on sentence.

http://codereview.chromium.org/114042/diff/3008/2013#newcode108
Line 108: // The current buffer pointed by current iterator has been consumed,
Run-on sentence.  You have several more of these; please go through all your
comments and consider using periods instead of commas.

http://codereview.chromium.org/114042/diff/3008/2013#newcode134
Line 134: delete *i;
Odd comment, but why not "delete buffer"?  Otherwise, why even have the local
buffer variable?

http://codereview.chromium.org/114042/diff/3008/2012
File media/base/seekable_buffer.h (right):

http://codereview.chromium.org/114042/diff/3008/2012#newcode46
Line 46: // Construct an instance with forward capacity of |forward_capacity|
Function comments should be phrased using descriptive (Constructs an instance)
rather than imperative (Construct an instance).  It looks like you're generally
using imperative; please fix all your comments.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

http://codereview.chromium.org/114042/diff/3008/2012#newcode47
Line 47: // and backward capacity of |backward_capacity|. The values are in
bytes.
Saying "forward capacity of forward capacity and backward capacity of backward
capacity" seems a little redundant.

http://codereview.chromium.org/114042/diff/3008/2012#newcode54
Line 54: // The current read position will advances by the amount of bytes read.
If
s/advances/advance

http://codereview.chromium.org/114042/diff/3008/2012#newcode55
Line 55: // reading caused backward bytes to exceed backward_capacity(), an
eviction
s/backward bytes/backward_bytes(), to fit what you're doing with
backward_capacity()

http://codereview.chromium.org/114042/diff/3008/2012#newcode56
Line 56: // of backward buffer will be done internally.
s/of/of the

http://codereview.chromium.org/114042/diff/3008/2012#newcode61
Line 61: // Append operations are always successful, a return value of false
only means
This sentence runs on.  You're using commas, but not using and/or/but.  Please
fix the grammar.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Punctu...

http://codereview.chromium.org/114042/diff/3008/2012#newcode114
Line 114: bool SeekForward(size_t size);
Please comment all functions, even if it's a brief one-liner.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

http://codereview.chromium.org/114042/diff/3008/2012#newcode118
Line 118: typedef std::list<Buffer*> BufferQueue;
On 2009/05/26 18:03:33, Alpha wrote:
> Should the the typedef for the STL container be defined just above the first
> use?

Per declaration order, typedefs (not just structs/classes, but also literal
typedefs) come first (with enums).
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

http://codereview.chromium.org/114042/diff/3008/2011
File media/base/seekable_buffer_unittest.cc (right):

http://codereview.chromium.org/114042/diff/3008/2011#newcode11
Line 11: namespace {
You might consider moving this unit test inside the media namespace, so that you
don't need to do media::

http://codereview.chromium.org/114042/diff/3008/2011#newcode17
Line 17: size_t seed = static_cast<int32>(base::Time::Now().ToInternalValue());
Internally, we have FLAGS_test_random_seed so that even random tests can be
repeated.  This is useful for trying to find failure causes.  I doubt Chrome has
a similar mechanism, but you might consider finding a way to choose a random
seed which can be reproduced if an issue is found.

http://codereview.chromium.org/114042/diff/3008/2011#newcode30
Line 30: buffer_.reset(new media::SeekableBuffer(kBufferSize, kBufferSize));
For what it's worth, gtest treats the constructor and destructor in the same way
as it does SetUp and TearDown.  If you used the constructor instead of SetUp,
you may be able to avoid scoped_ptr/scoped_array.

http://codereview.chromium.org/114042/diff/3008/2011#newcode37
Line 37: static const size_t kDataSize = 409600;
Fix declaration order
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

http://codereview.chromium.org/114042/diff/3008/2011#newcode83
Line 83: buffer_->Append(kWriteSize, data_.get() + write_position);
Is write_position still necessary?  It seems to me that j == write_position. 
Similarly, j == forward_bytes (in this loop, at least).

The same comment could be made of BufferFull; please remove redundant local
variables.

http://codereview.chromium.org/114042/diff/3008/2011#newcode162
Line 162: // Write 10 more times, the buffer is full.
I'm not clear what you're trying to say here.

http://codereview.chromium.org/114042/diff/3008/2011#newcode188
Line 188: // Expect we have no bytes left.
s/Expect/Expect that

http://codereview.chromium.org/114042/diff/3008/2011#newcode199
Line 199: const size_t kWriteSize = 512;
You set kWriteSize = 512 in multiple places.  It should probably be on the test
class.

http://codereview.chromium.org/114042/diff/3008/2011#newcode233
Line 233: if (!write_size)
Rather than using ! on an int, I'd suggest <= 0

Also, because GetRandomInt shouldn't be <= 0, you might want to explicitly
compare kDataSize - write_position to be clearer about what you're trying to
check.

You could do that before the GetRandomInt or std::min call.  It could even be
part of the for loop.

[Alpha Left Google, 2009-05-27 23:30:02]

http://codereview.chromium.org/114042/diff/3008/2013
File media/base/seekable_buffer.cc (right):

http://codereview.chromium.org/114042/diff/3008/2013#newcode85
Line 85: // only be invalid before any data is appended, this case should be
handled
On 2009/05/27 19:25:14, Jon Perkins wrote:
> Run-on sentence.

Done.

http://codereview.chromium.org/114042/diff/3008/2013#newcode89
Line 89: // We try to at most |size| bytes in the backward direction, we also
have
On 2009/05/27 19:25:14, Jon Perkins wrote:
> Run-on sentence.

Done.

http://codereview.chromium.org/114042/diff/3008/2013#newcode108
Line 108: // The current buffer pointed by current iterator has been consumed,
On 2009/05/27 19:25:14, Jon Perkins wrote:
> Run-on sentence.  You have several more of these; please go through all your
> comments and consider using periods instead of commas.

Done.

http://codereview.chromium.org/114042/diff/3008/2013#newcode134
Line 134: delete *i;
On 2009/05/27 19:25:14, Jon Perkins wrote:
> Odd comment, but why not "delete buffer"?  Otherwise, why even have the local
> buffer variable?

Done.

http://codereview.chromium.org/114042/diff/3008/2013#newcode134
Line 134: delete *i;
On 2009/05/27 19:25:14, Jon Perkins wrote:
> Odd comment, but why not "delete buffer"?  Otherwise, why even have the local
> buffer variable?

Done.

http://codereview.chromium.org/114042/diff/3008/2012
File media/base/seekable_buffer.h (right):

http://codereview.chromium.org/114042/diff/3008/2012#newcode46
Line 46: // Construct an instance with forward capacity of |forward_capacity|
On 2009/05/27 19:25:14, Jon Perkins wrote:
> Function comments should be phrased using descriptive (Constructs an instance)
> rather than imperative (Construct an instance).  It looks like you're
generally
> using imperative; please fix all your comments.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

Done.

http://codereview.chromium.org/114042/diff/3008/2012#newcode47
Line 47: // and backward capacity of |backward_capacity|. The values are in
bytes.
On 2009/05/27 19:25:14, Jon Perkins wrote:
> Saying "forward capacity of forward capacity and backward capacity of backward
> capacity" seems a little redundant.

Done.

http://codereview.chromium.org/114042/diff/3008/2012#newcode54
Line 54: // The current read position will advances by the amount of bytes read.
If
On 2009/05/27 19:25:14, Jon Perkins wrote:
> s/advances/advance

Done.

http://codereview.chromium.org/114042/diff/3008/2012#newcode55
Line 55: // reading caused backward bytes to exceed backward_capacity(), an
eviction
On 2009/05/27 19:25:14, Jon Perkins wrote:
> s/backward bytes/backward_bytes(), to fit what you're doing with
> backward_capacity()

Done.

http://codereview.chromium.org/114042/diff/3008/2012#newcode56
Line 56: // of backward buffer will be done internally.
On 2009/05/27 19:25:14, Jon Perkins wrote:
> s/of/of the

Done.

http://codereview.chromium.org/114042/diff/3008/2012#newcode61
Line 61: // Append operations are always successful, a return value of false
only means
On 2009/05/27 19:25:14, Jon Perkins wrote:
> This sentence runs on.  You're using commas, but not using and/or/but.  Please
> fix the grammar.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Punctu...

Done.

http://codereview.chromium.org/114042/diff/3008/2012#newcode94
Line 94: // A structure that contains a block of data.
On 2009/05/26 18:03:33, Alpha wrote:
> Definition of struct is moved on top.

Done.

http://codereview.chromium.org/114042/diff/3008/2012#newcode114
Line 114: bool SeekForward(size_t size);
On 2009/05/27 19:25:14, Jon Perkins wrote:
> Please comment all functions, even if it's a brief one-liner.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

Done.

http://codereview.chromium.org/114042/diff/3008/2012#newcode118
Line 118: typedef std::list<Buffer*> BufferQueue;
On 2009/05/27 19:25:14, Jon Perkins wrote:
> On 2009/05/26 18:03:33, Alpha wrote:
> > Should the the typedef for the STL container be defined just above the first
> > use?
> 
> Per declaration order, typedefs (not just structs/classes, but also literal
> typedefs) come first (with enums).
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

Done.

http://codereview.chromium.org/114042/diff/3008/2012#newcode130
Line 130: DISALLOW_COPY_AND_ASSIGN(SeekableBuffer);
On 2009/05/26 18:03:33, Alpha wrote:
> added disallow_copy_and_assign

Done.

http://codereview.chromium.org/114042/diff/3008/2011
File media/base/seekable_buffer_unittest.cc (right):

http://codereview.chromium.org/114042/diff/3008/2011#newcode11
Line 11: namespace {
On 2009/05/27 19:25:14, Jon Perkins wrote:
> You might consider moving this unit test inside the media namespace, so that
you
> don't need to do media::

The other unit tests in the same project tend to be in anonymous namespace. I
was suggested in previous review to put it in anonymous namespace, should keep
it coherent across the project.

http://codereview.chromium.org/114042/diff/3008/2011#newcode17
Line 17: size_t seed = static_cast<int32>(base::Time::Now().ToInternalValue());
On 2009/05/27 19:25:14, Jon Perkins wrote:
> Internally, we have FLAGS_test_random_seed so that even random tests can be
> repeated.  This is useful for trying to find failure causes.  I doubt Chrome
has
> a similar mechanism, but you might consider finding a way to choose a random
> seed which can be reproduced if an issue is found.

Chromium doesn't have FLAGS_test_random_seed defined.. I should add it in
another CL.

http://codereview.chromium.org/114042/diff/3008/2011#newcode30
Line 30: buffer_.reset(new media::SeekableBuffer(kBufferSize, kBufferSize));
On 2009/05/27 19:25:14, Jon Perkins wrote:
> For what it's worth, gtest treats the constructor and destructor in the same
way
> as it does SetUp and TearDown.  If you used the constructor instead of SetUp,
> you may be able to avoid scoped_ptr/scoped_array.

Done.

http://codereview.chromium.org/114042/diff/3008/2011#newcode37
Line 37: static const size_t kDataSize = 409600;
On 2009/05/27 19:25:14, Jon Perkins wrote:
> Fix declaration order
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

Done.

http://codereview.chromium.org/114042/diff/3008/2011#newcode83
Line 83: buffer_->Append(kWriteSize, data_.get() + write_position);
On 2009/05/27 19:25:14, Jon Perkins wrote:
> Is write_position still necessary?  It seems to me that j == write_position. 
> Similarly, j == forward_bytes (in this loop, at least).
> 

write_position is there so we try different window of data, as the operations
are the same for each iteration in the bigger for(int i = 0; i < 10; ++i), I
guess it makes sense to get rid of write_position..

> The same comment could be made of BufferFull; please remove redundant local
> variables.

http://codereview.chromium.org/114042/diff/3008/2011#newcode162
Line 162: // Write 10 more times, the buffer is full.
On 2009/05/27 19:25:14, Jon Perkins wrote:
> I'm not clear what you're trying to say here.

Done.

http://codereview.chromium.org/114042/diff/3008/2011#newcode188
Line 188: // Expect we have no bytes left.
On 2009/05/27 19:25:14, Jon Perkins wrote:
> s/Expect/Expect that

Done.

http://codereview.chromium.org/114042/diff/3008/2011#newcode199
Line 199: const size_t kWriteSize = 512;
On 2009/05/27 19:25:14, Jon Perkins wrote:
> You set kWriteSize = 512 in multiple places.  It should probably be on the
test
> class.

Done.

http://codereview.chromium.org/114042/diff/3008/2011#newcode233
Line 233: if (!write_size)
On 2009/05/27 19:25:14, Jon Perkins wrote:
> Rather than using ! on an int, I'd suggest <= 0
> 
> Also, because GetRandomInt shouldn't be <= 0, you might want to explicitly
> compare kDataSize - write_position to be clearer about what you're trying to
> check.
> 
> You could do that before the GetRandomInt or std::min call.  It could even be
> part of the for loop.

Done.

[Jon Perkins, 2009-05-28 01:02:48]

LGTM

Thanks for the changes.  Most of the run-on sentences you fixed were good, but
I've marked a couple that were probably fine.

http://codereview.chromium.org/114042/diff/3013/3015
File media/base/seekable_buffer.cc (right):

http://codereview.chromium.org/114042/diff/3013/3015#newcode35
Line 35: // full we always append data to the buffer.
s/full we/full, we
This was actually a reasonable use of a comma.

http://codereview.chromium.org/114042/diff/3013/3015#newcode158
Line 158: // |data| is NULL if we are seeking forward so there's no need to
copy.
s/forward so/forward, so

(this one was also better before, I think)

http://codereview.chromium.org/114042/diff/3013/3016
File media/base/seekable_buffer.h (right):

http://codereview.chromium.org/114042/diff/3013/3016#newcode119
Line 119: // If return value is true, the operation is completed successfully.
s/is completed/completed

http://codereview.chromium.org/114042/diff/3013/3016#newcode119
Line 119: // If return value is true, the operation is completed successfully.
This should be "If the return value..." here and below.

http://codereview.chromium.org/114042/diff/3013/3016#newcode126
Line 126: // If return value is true, the operation is completed successfully.
s/is completed/completed

http://codereview.chromium.org/114042/diff/2024/2025
File media/base/seekable_buffer_unittest.cc (right):

http://codereview.chromium.org/114042/diff/2024/2025#newcode83
Line 83: size_t forward_bytes = buffer_.forward_bytes();
If you like, kBufferSize should work here

[Alpha Left Google, 2009-05-28 01:31:16]

Thanks for the review! It's all cleaned up now.