Rewrite BufferedDataSource tests to use real BufferedResourceLoader objects.

Rewrite BufferedDataSource tests to use real BufferedResourceLoader objects.

The new tests expose the very real memory leak described in bug 100914. The fix for the leak is non trivial and will be addressed in a follow up patch.

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

[scherkus (not reviewing), 2011-11-22 21:12:45]

fischman/vrk: PTAL

the diff might be kind of useless but I swear the new tests are easier to
understand!!!

[Ami GONE FROM CHROMIUM, 2011-11-22 22:06:23]

Nice!

http://codereview.chromium.org/8649002/diff/17/tools/valgrind/memcheck/suppre...
File tools/valgrind/memcheck/suppressions.txt (right):

http://codereview.chromium.org/8649002/diff/17/tools/valgrind/memcheck/suppre...
tools/valgrind/memcheck/suppressions.txt:4700:
fun:_ZN12webkit_media18BufferedDataSource20CreateResourceLoaderEll
Here and below it seems strange to me that that a frame *upwards* is specified
(the variant between a/b/c).  Is there a reason to keep the suppressions
separated instead of just ending the stack after this frame?

http://codereview.chromium.org/8649002/diff/17/webkit/media/buffered_data_sou...
File webkit/media/buffered_data_source_unittest.cc (right):

http://codereview.chromium.org/8649002/diff/17/webkit/media/buffered_data_sou...
webkit/media/buffered_data_source_unittest.cc:83: :
responses_(GURL("http://localhost/foo.webm"), 5000000),
Can this 5M (and the 4M elsewhere in this file) be phrased in terms of something
symbolic?
Or at least collect the magic numbers in one place in *this* file, with
cross-commentary pointing at the relevant numbers elsewhere (in
BufferedDataSource/BufferedResourceLoader, presumably?).

http://codereview.chromium.org/8649002/diff/17/webkit/media/buffered_data_sou...
webkit/media/buffered_data_source_unittest.cc:175: return
static_cast<NiceMock<MockWebURLLoader>*>(
Does this really need to return the Mock subtype (and thus require the cast)?

http://codereview.chromium.org/8649002/diff/17/webkit/media/buffered_data_sou...
webkit/media/buffered_data_source_unittest.cc:191: ResponseGenerator responses_;
s/responses_/response_generator_/

http://codereview.chromium.org/8649002/diff/17/webkit/media/buffered_data_sou...
webkit/media/buffered_data_source_unittest.cc:225: // It'll try again.
(here and below)
Is this business of blindly retrying a bug worthy of filing or at least a TODO?

http://codereview.chromium.org/8649002/diff/17/webkit/media/buffered_data_sou...
webkit/media/buffered_data_source_unittest.cc:237: // It'll try again.
I think we have a crbug to realize range->200 shouldn't trigger a new
(non-range) request, instead just use the 200 data.  TODO to it?

http://codereview.chromium.org/8649002/diff/17/webkit/media/buffered_data_sou...
webkit/media/buffered_data_source_unittest.cc:259:
Respond(responses_.Generate206(0, ResponseGenerator::kNoContentRange));
Shouldn't this be failing back to a non-range request (so get a 200)?

http://codereview.chromium.org/8649002/diff/17/webkit/media/buffered_data_sou...
webkit/media/buffered_data_source_unittest.cc:329: // It'll error after this.
I wish expecting an error in all these tests didn't manifest as "don't call
Stop()".

http://codereview.chromium.org/8649002/diff/17/webkit/media/response_generato...
File webkit/media/response_generator.cc (right):

http://codereview.chromium.org/8649002/diff/17/webkit/media/response_generato...
webkit/media/response_generator.cc:50: int64 last_byte_offset = content_length_
- first_byte_offset - 1;
What the what?
A 10-byte file, requesting starting from byte 3, will return:
3-6/10
instead of 
3-9/10
?

http://codereview.chromium.org/8649002/diff/17/webkit/media/response_generator.h
File webkit/media/response_generator.h (right):

http://codereview.chromium.org/8649002/diff/17/webkit/media/response_generato...
webkit/media/response_generator.h:12: // Generates WebURLErrors and
WebURLResponses suitable for testing purposes.
Include "test" in the file & class names?

http://codereview.chromium.org/8649002/diff/17/webkit/media/response_generato...
webkit/media/response_generator.h:15: enum Flags {
How about some commentary?

TBH, this whole class feels small enough that it should be in-lined in the test
file, unless you expect to be using this elsewhere shortly.

[scherkus (not reviewing), 2011-11-23 04:22:41]

http://codereview.chromium.org/8649002/diff/17/tools/valgrind/memcheck/suppre...
File tools/valgrind/memcheck/suppressions.txt (right):

http://codereview.chromium.org/8649002/diff/17/tools/valgrind/memcheck/suppre...
tools/valgrind/memcheck/suppressions.txt:4700:
fun:_ZN12webkit_media18BufferedDataSource20CreateResourceLoaderEll
On 2011/11/22 22:06:23, Ami Fischman wrote:
> Here and below it seems strange to me that that a frame *upwards* is specified
> (the variant between a/b/c).  Is there a reason to keep the suppressions
> separated instead of just ending the stack after this frame?

Done.

http://codereview.chromium.org/8649002/diff/17/webkit/media/buffered_data_sou...
File webkit/media/buffered_data_source_unittest.cc (right):

http://codereview.chromium.org/8649002/diff/17/webkit/media/buffered_data_sou...
webkit/media/buffered_data_source_unittest.cc:83: :
responses_(GURL("http://localhost/foo.webm"), 5000000),
On 2011/11/22 22:06:23, Ami Fischman wrote:
> Can this 5M (and the 4M elsewhere in this file) be phrased in terms of
something
> symbolic?
> Or at least collect the magic numbers in one place in *this* file, with
> cross-commentary pointing at the relevant numbers elsewhere (in
> BufferedDataSource/BufferedResourceLoader, presumably?).

the 5M/4M are test specific but I'll go ahead and add some constants

http://codereview.chromium.org/8649002/diff/17/webkit/media/buffered_data_sou...
webkit/media/buffered_data_source_unittest.cc:175: return
static_cast<NiceMock<MockWebURLLoader>*>(
On 2011/11/22 22:06:23, Ami Fischman wrote:
> Does this really need to return the Mock subtype (and thus require the cast)?

Nope!

It used to but we no longer require the mock object after creation

http://codereview.chromium.org/8649002/diff/17/webkit/media/buffered_data_sou...
webkit/media/buffered_data_source_unittest.cc:191: ResponseGenerator responses_;
On 2011/11/22 22:06:23, Ami Fischman wrote:
> s/responses_/response_generator_/

Done.

http://codereview.chromium.org/8649002/diff/17/webkit/media/buffered_data_sou...
webkit/media/buffered_data_source_unittest.cc:225: // It'll try again.
On 2011/11/22 22:06:23, Ami Fischman wrote:
> (here and below)
> Is this business of blindly retrying a bug worthy of filing or at least a
TODO?

bug worthy

http://codereview.chromium.org/8649002/diff/17/webkit/media/buffered_data_sou...
webkit/media/buffered_data_source_unittest.cc:237: // It'll try again.
On 2011/11/22 22:06:23, Ami Fischman wrote:
> I think we have a crbug to realize range->200 shouldn't trigger a new
> (non-range) request, instead just use the 200 data.  TODO to it?

Done.

http://codereview.chromium.org/8649002/diff/17/webkit/media/buffered_data_sou...
webkit/media/buffered_data_source_unittest.cc:259:
Respond(responses_.Generate206(0, ResponseGenerator::kNoContentRange));
On 2011/11/22 22:06:23, Ami Fischman wrote:
> Shouldn't this be failing back to a non-range request (so get a 200)?

this is a 206 response that's missing content range or has otherwise unparsable
content range headers -- it's simply a bad response from the server

http://codereview.chromium.org/8649002/diff/17/webkit/media/buffered_data_sou...
webkit/media/buffered_data_source_unittest.cc:329: // It'll error after this.
On 2011/11/22 22:06:23, Ami Fischman wrote:
> I wish expecting an error in all these tests didn't manifest as "don't call
> Stop()".

The reason I don't call Stop() is because we don't distinguish between a read
callback error that was caused by the network versus an error that was caused by
aborting everything via Stop()

I think I can fix this by adding in more gmock if that's preferable

http://codereview.chromium.org/8649002/diff/17/webkit/media/response_generato...
File webkit/media/response_generator.cc (right):

http://codereview.chromium.org/8649002/diff/17/webkit/media/response_generato...
webkit/media/response_generator.cc:50: int64 last_byte_offset = content_length_
- first_byte_offset - 1;
On 2011/11/22 22:06:23, Ami Fischman wrote:
> What the what?
> A 10-byte file, requesting starting from byte 3, will return:
> 3-6/10
> instead of 
> 3-9/10
> ?
whoops!

http://codereview.chromium.org/8649002/diff/17/webkit/media/response_generator.h
File webkit/media/response_generator.h (right):

http://codereview.chromium.org/8649002/diff/17/webkit/media/response_generato...
webkit/media/response_generator.h:12: // Generates WebURLErrors and
WebURLResponses suitable for testing purposes.
On 2011/11/22 22:06:23, Ami Fischman wrote:
> Include "test" in the file & class names?

Done.

http://codereview.chromium.org/8649002/diff/17/webkit/media/response_generato...
webkit/media/response_generator.h:15: enum Flags {
On 2011/11/22 22:06:23, Ami Fischman wrote:
> How about some commentary?
> 
> TBH, this whole class feels small enough that it should be in-lined in the
test
> file, unless you expect to be using this elsewhere shortly.

BufferedResourceLoaderTest has lots of crusty response generation code I'd like
to pull out

[Ami GONE FROM CHROMIUM, 2011-11-24 01:20:38]

LGTM

http://codereview.chromium.org/8649002/diff/17/webkit/media/buffered_data_sou...
File webkit/media/buffered_data_source_unittest.cc (right):

http://codereview.chromium.org/8649002/diff/17/webkit/media/buffered_data_sou...
webkit/media/buffered_data_source_unittest.cc:329: // It'll error after this.
On 2011/11/23 04:22:41, scherkus wrote:
> On 2011/11/22 22:06:23, Ami Fischman wrote:
> > I wish expecting an error in all these tests didn't manifest as "don't call
> > Stop()".
> 
> The reason I don't call Stop() is because we don't distinguish between a read
> callback error that was caused by the network versus an error that was caused
by
> aborting everything via Stop()
> 
> I think I can fix this by adding in more gmock if that's preferable

Your choice (maybe code it and see what it looks like?).

http://codereview.chromium.org/8649002/diff/8001/webkit/media/test_response_g...
File webkit/media/test_response_generator.cc (right):

http://codereview.chromium.org/8649002/diff/8001/webkit/media/test_response_g...
webkit/media/test_response_generator.cc:49: Flags flags) {
indent

[scherkus (not reviewing), 2011-11-28 18:30:47]

turns out checking loading has indeed stopped prior to calling Stop() is good
enough to verify that we won't receive a network error due to aborting the load
via Stop()

http://codereview.chromium.org/8649002/diff/8001/webkit/media/test_response_g...
File webkit/media/test_response_generator.cc (right):

http://codereview.chromium.org/8649002/diff/8001/webkit/media/test_response_g...
webkit/media/test_response_generator.cc:49: Flags flags) {
On 2011/11/24 01:20:38, Ami Fischman wrote:
> indent

Done.

[vrk (LEFT CHROMIUM), 2011-11-29 04:08:23]

LGTM with nits

The tests look WAY better; thanks scherkus!!

http://codereview.chromium.org/8649002/diff/22002/webkit/media/buffered_data_...
File webkit/media/buffered_data_source_unittest.cc (right):

http://codereview.chromium.org/8649002/diff/22002/webkit/media/buffered_data_...
webkit/media/buffered_data_source_unittest.cc:166: EXPECT_CALL(*this,
ReadCallback(media::DataSource::kReadError));
(This comment is based on the slight confusion I had in Range_TooManyRetries.)

Hmm, it was a little surprising to me that EXPECT_CALL here and in ReadAt was
not fulfilled by the end of the method call (but rather the expectation is set
for some later method call to fulfill). Comment here and in ReadAt?

Or, it may be clearer to remove the expectation from the method altogether
(which would also remove ReadAtWithError entirely) and explicitly expect the
read success or fail in each test, right before the expected failure.

http://codereview.chromium.org/8649002/diff/22002/webkit/media/buffered_data_...
webkit/media/buffered_data_source_unittest.cc:260: ExpectCreateResourceLoader();
nit: move ExpectCreateResourceLoader() into beginning of Initialize()?

http://codereview.chromium.org/8649002/diff/22002/webkit/media/buffered_data_...
webkit/media/buffered_data_source_unittest.cc:360: FinishLoading();
So to be clear: The expected error that's set in ReadAtWithError gets fufilled
at the end of this method?

http://codereview.chromium.org/8649002/diff/22002/webkit/media/buffered_data_...
webkit/media/buffered_data_source_unittest.cc:443: // Verify loader changed but
still has same bitrate.
nit: s/bitrate/playback rate

http://codereview.chromium.org/8649002/diff/22002/webkit/media/buffered_data_...
webkit/media/buffered_data_source_unittest.cc:455: // When the read completes
we'll update our network status.
nit: use kDataSize?

http://codereview.chromium.org/8649002/diff/22002/webkit/media/buffered_data_...
webkit/media/buffered_data_source_unittest.cc:461: EXPECT_CALL(host_,
SetBufferedBytes(1024));
nit: use kDataSize?

[scherkus (not reviewing), 2011-11-29 18:49:29]

thanks for the review vrk/fischman!

http://codereview.chromium.org/8649002/diff/22002/webkit/media/buffered_data_...
File webkit/media/buffered_data_source_unittest.cc (right):

http://codereview.chromium.org/8649002/diff/22002/webkit/media/buffered_data_...
webkit/media/buffered_data_source_unittest.cc:166: EXPECT_CALL(*this,
ReadCallback(media::DataSource::kReadError));
On 2011/11/29 04:08:23, Victoria Kirst wrote:
> (This comment is based on the slight confusion I had in Range_TooManyRetries.)
> 
> Hmm, it was a little surprising to me that EXPECT_CALL here and in ReadAt was
> not fulfilled by the end of the method call (but rather the expectation is set
> for some later method call to fulfill). Comment here and in ReadAt?
> 
> Or, it may be clearer to remove the expectation from the method altogether
> (which would also remove ReadAtWithError entirely) and explicitly expect the
> read success or fail in each test, right before the expected failure. 

Good point!

I've removed ReadAtWithError() and now have ReadAt() which is a convenience
function for issuing a read. The expectations have been moved closer to where we
actually expect them to get called.

http://codereview.chromium.org/8649002/diff/22002/webkit/media/buffered_data_...
webkit/media/buffered_data_source_unittest.cc:260: ExpectCreateResourceLoader();
On 2011/11/29 04:08:23, Victoria Kirst wrote:
> nit: move ExpectCreateResourceLoader() into beginning of Initialize()?

Done.

http://codereview.chromium.org/8649002/diff/22002/webkit/media/buffered_data_...
webkit/media/buffered_data_source_unittest.cc:443: // Verify loader changed but
still has same bitrate.
On 2011/11/29 04:08:23, Victoria Kirst wrote:
> nit: s/bitrate/playback rate

Done.

http://codereview.chromium.org/8649002/diff/22002/webkit/media/buffered_data_...
webkit/media/buffered_data_source_unittest.cc:455: // When the read completes
we'll update our network status.
On 2011/11/29 04:08:23, Victoria Kirst wrote:
> nit: use kDataSize?

Done.

http://codereview.chromium.org/8649002/diff/22002/webkit/media/buffered_data_...
webkit/media/buffered_data_source_unittest.cc:461: EXPECT_CALL(host_,
SetBufferedBytes(1024));
On 2011/11/29 04:08:23, Victoria Kirst wrote:
> nit: use kDataSize?

Done.

[scherkus (not reviewing), 2011-11-29 21:07:20]

+tony for webkit/tools OWNERS

[tony, 2011-11-29 21:33:53]

LGTM