Retry requests that has timed out in BufferedDataSource

Retry requests that has timed out in BufferedDataSource

Retries request if the read request is not fulfilled in 5 seconds and at most 3 times. This will handle the case when
a user pause a movie and starts it in a later time when the
connection to the server has already timed out.

TEST=BufferedDataSourceTest.*
BUG=17479
done

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

[Alpha Left Google, 2009-07-27 21:29:43]

+ajwong

ajwong: do you mind taking a look too?

[scherkus (not reviewing), 2009-07-27 21:31:32]

yeah sorry for taking a while.. need to get this regression fixed :(

On Mon, Jul 27, 2009 at 2:29 PM, <hclam@chromium.org> wrote:

>
> +ajwong
>
> ajwong: do you mind taking a look too?
>
>
> http://codereview.chromium.org/159382
>

[scherkus (not reviewing), 2009-07-27 22:53:04]

http://codereview.chromium.org/159382/diff/12/13
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/159382/diff/12/13#newcode436
Line 436: DCHECK(stopped_);
watch out: object is created with stopped_ == false, meaning if someone created
+ deleted this object without doing anything on it, we'd hit a DCHECK

perhaps leave stopped_ as true and set it to false in Initialize()?

http://codereview.chromium.org/159382/diff/12/13#newcode485
Line 485: AutoLock auto_lock(lock_);
quick question: why don't we want to set stopped_ = true inside StopTask?

http://codereview.chromium.org/159382/diff/12/13#newcode589
Line 589: // It is only if there is an active read request that we care.
wording: "We only care if there is an active read request."

http://codereview.chromium.org/159382/diff/12/13#newcode599
Line 599: // the whole pipeline may get destroyed...
Hmmm yeah perhaps with kyle's error reporting code it'll be ok

right now we only call Stop() on every filter and if we properly report an error
we should be good

http://codereview.chromium.org/159382/diff/12/14
File webkit/glue/media/buffered_data_source.h (right):

http://codereview.chromium.org/159382/diff/12/14#newcode12
Line 12: #include "base/time.h"
nit: timer.h includes time.h, you can remove

http://codereview.chromium.org/159382/diff/12/14#newcode191
Line 191: virtual int GetTimeoutMilliseconds();
nit: why not base::TimeDelta?

http://codereview.chromium.org/159382/diff/12/14#newcode268
Line 268: int read_tried_;
nit: maybe "read_attempts_" ?

http://codereview.chromium.org/159382/diff/12/14#newcode297
Line 297: // loop. By having a repeating timer, we can reduce the lifelime of
the
lifelime -> lifetime

when I first read this I thought that RepeatingTimer would use a different
thread, however it is still implemented using MessageLoop::current() +
PostDelayedTask() which makes this comment sound a little misleading.

Maybe say something like "RepeatingTimer makes sure the time doesn't fire as
soon as we call Stop()" or something

http://codereview.chromium.org/159382/diff/12/15
File webkit/glue/media/buffered_data_source_unittest.cc (right):

http://codereview.chromium.org/159382/diff/12/15#newcode579
Line 579: void ReadDataSourceTimesOut(int64 position, int size) {
I'm a bit confused why this isn't inlined inside the test

http://codereview.chromium.org/159382/diff/12/15#newcode619
Line 619: EXPECT_TRUE(loader_.get() == NULL);
nit: could use EXPECT_FALSE without == NULL

[Alpha Left Google, 2009-07-28 20:55:44]

http://codereview.chromium.org/159382/diff/12/13
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/159382/diff/12/13#newcode436
Line 436: DCHECK(stopped_);
On 2009/07/27 22:53:06, scherkus wrote:
> watch out: object is created with stopped_ == false, meaning if someone
created
> + deleted this object without doing anything on it, we'd hit a DCHECK
> 
> perhaps leave stopped_ as true and set it to false in Initialize()?

Hum... The check is mostly for debugging purpose.. If i set it in Initialize() I
need additional locks.. I'd just remove this check.

http://codereview.chromium.org/159382/diff/12/13#newcode485
Line 485: AutoLock auto_lock(lock_);
On 2009/07/27 22:53:06, scherkus wrote:
> quick question: why don't we want to set stopped_ = true inside StopTask?

Because this will create a race condition:
Stop() on pipeline thread
... demuxer is killed
ReadTask() on render thread
... calls to demuxer or host() *crash*
StopTask() on render thread

http://codereview.chromium.org/159382/diff/12/13#newcode589
Line 589: // It is only if there is an active read request that we care.
On 2009/07/27 22:53:06, scherkus wrote:
> wording: "We only care if there is an active read request."

http://codereview.chromium.org/159382/diff/12/13#newcode589
Line 589: // It is only if there is an active read request that we care.
On 2009/07/27 22:53:06, scherkus wrote:
> wording: "We only care if there is an active read request."

Done.

http://codereview.chromium.org/159382/diff/12/13#newcode599
Line 599: // the whole pipeline may get destroyed...
On 2009/07/27 22:53:06, scherkus wrote:
> Hmmm yeah perhaps with kyle's error reporting code it'll be ok
> 
> right now we only call Stop() on every filter and if we properly report an
error
> we should be good

I'll leave this as a TODO because I need some tweaking here.

http://codereview.chromium.org/159382/diff/12/14
File webkit/glue/media/buffered_data_source.h (right):

http://codereview.chromium.org/159382/diff/12/14#newcode12
Line 12: #include "base/time.h"
On 2009/07/27 22:53:06, scherkus wrote:
> nit: timer.h includes time.h, you can remove

Done.

http://codereview.chromium.org/159382/diff/12/14#newcode268
Line 268: int read_tried_;
On 2009/07/27 22:53:06, scherkus wrote:
> nit: maybe "read_attempts_" ?

Done.

http://codereview.chromium.org/159382/diff/12/14#newcode297
Line 297: // loop. By having a repeating timer, we can reduce the lifelime of
the
On 2009/07/27 22:53:06, scherkus wrote:
> lifelime -> lifetime
> 
> when I first read this I thought that RepeatingTimer would use a different
> thread, however it is still implemented using MessageLoop::current() +
> PostDelayedTask() which makes this comment sound a little misleading.
> 
> Maybe say something like "RepeatingTimer makes sure the time doesn't fire as
> soon as we call Stop()" or something

The purpose of this timer is really to reduce the extra reference, even if I
implement this as PostDelayedTask it doesn't get fired once we call Stop().

[scherkus (not reviewing), 2009-07-29 18:42:20]

few comments

http://codereview.chromium.org/159382/diff/22/24
File webkit/glue/media/buffered_data_source.h (right):

http://codereview.chromium.org/159382/diff/22/24#newcode295
Line 295: // of doing PostDelayedTask reduce the extra reference held by the
message
PostDelayedTask -> PostDelayedTask()

http://codereview.chromium.org/159382/diff/22/24#newcode296
Line 296: // loop. The RepeatingTimer does PostDelayedTask internally, by using
it
PostDelayedTask -> PostDelayedTask()

http://codereview.chromium.org/159382/diff/22/25
File webkit/glue/media/buffered_data_source_unittest.cc (right):

http://codereview.chromium.org/159382/diff/22/25#newcode354
Line 354: return base::TimeDelta::FromMilliseconds(100);
could it be zero seconds?

will this test be flaky?

http://codereview.chromium.org/159382/diff/22/25#newcode579
Line 579: void ReadDataSourceTimesOut(int64 position, int size) {
this is only used once... maybe move it to inside the test body itself?

sorry if I already recommended this... didn't check comment history before
typing this up :)

http://codereview.chromium.org/159382/diff/22/25#newcode614
Line 614: message_loop_->Run();
comment saying this will block until watchdog fires and loop shuts down

[Alpha Left Google, 2009-07-29 19:00:43]

http://codereview.chromium.org/159382/diff/22/24
File webkit/glue/media/buffered_data_source.h (right):

http://codereview.chromium.org/159382/diff/22/24#newcode295
Line 295: // of doing PostDelayedTask reduce the extra reference held by the
message
On 2009/07/29 18:42:20, scherkus wrote:
> PostDelayedTask -> PostDelayedTask()

Done.

http://codereview.chromium.org/159382/diff/22/24#newcode296
Line 296: // loop. The RepeatingTimer does PostDelayedTask internally, by using
it
On 2009/07/29 18:42:20, scherkus wrote:
> PostDelayedTask -> PostDelayedTask()

Done.

http://codereview.chromium.org/159382/diff/22/25
File webkit/glue/media/buffered_data_source_unittest.cc (right):

http://codereview.chromium.org/159382/diff/22/25#newcode354
Line 354: return base::TimeDelta::FromMilliseconds(100);
On 2009/07/29 18:42:20, scherkus wrote:
> could it be zero seconds?
> 
> will this test be flaky?

Haven't tried this.. but 100 ms is not too bad, it's better to have non-flaky
test.

http://codereview.chromium.org/159382/diff/22/25#newcode579
Line 579: void ReadDataSourceTimesOut(int64 position, int size) {
On 2009/07/29 18:42:20, scherkus wrote:
> this is only used once... maybe move it to inside the test body itself?
> 
> sorry if I already recommended this... didn't check comment history before
> typing this up :)

I have compilation errors if I do so.. :( I'd leave it as is..

http://codereview.chromium.org/159382/diff/22/25#newcode614
Line 614: message_loop_->Run();
On 2009/07/29 18:42:20, scherkus wrote:
> comment saying this will block until watchdog fires and loop shuts down

Done.

[scherkus (not reviewing), 2009-07-29 19:02:41]

LGTM