Report stalled event correctly for <video>

Report stalled event correctly for <video>

BUG=20127
BUG=13568
TEST=Opens a web page, stalled event should be fire only if network
     is actuall stalled.

What this patch does:
1. Report Loading / Idle according to whether we are actively
   receiving data in the BuffereResourceLoader. This is done by
   signaling the network events to BufferedDataSource and then
   to the media playback pipeline.

2. Report byteLoaded() as the last byte position buffered. This
   will enable an actual ticking progress for the progress event.
   With this value actually ticking, stalled event is suppressed.

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

[Alpha Left Google, 2009-10-07 06:01:04]

This is really a not-so-nice fix for the stalled event. In the end I still need
to report the last byte buffered as "byteLoaded" which is not correct...

So the progress event received by javascript will be something like:

0
1024
10000 (end of file)
2048
3172
4096
...

This is because for those files with index at the end, we will read those bytes
first.. This could be fixed.. but I think this is good enough to make a fix.

[scherkus (not reviewing), 2009-10-15 00:15:53]

http://codereview.chromium.org/269002/diff/1001/2005
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/269002/diff/1001/2005#newcode145
Line 145: if (!buffer_.get())
hmm the next line destroys the internal buffer

does Stop() get called multiple times?

http://codereview.chromium.org/269002/diff/1001/2005#newcode800
Line 800: streaming_ = instance_size == kPositionNotSpecified ||
!partial_response;
would recommend brackets around instance_size == kPositionNotSpecified for
clarity

http://codereview.chromium.org/269002/diff/1001/2005#newcode836
Line 836: host()->SetLoaded(loaded_);
so really loaded_ here is still false

http://codereview.chromium.org/269002/diff/1001/2005#newcode947
Line 947: if (loaded_)
if this is the only reason for having loaded, you could use
!IsHttpProtocol(url_) instead and do without the extra variable

loaded_ seems misleading because it really means "we're using a non-http
resource"

what you're doing here is suppressing network activity events for non-http
resources.. I think a comment would be good.

http://codereview.chromium.org/269002/diff/1001/2006
File webkit/glue/media/buffered_data_source.h (right):

http://codereview.chromium.org/269002/diff/1001/2006#newcode65
Line 65: Callback0::Type* event_callback);
can we typedef this?

http://codereview.chromium.org/269002/diff/1001/2006#newcode83
Line 83: // Returns the position of the first byte buffered.
I would mention when it returns zero.

http://codereview.chromium.org/269002/diff/1001/2006#newcode87
Line 87: virtual int64 GetBufferedLastBytePosition();
I would mention when it returns zero.

http://codereview.chromium.org/269002/diff/1001/2006#newcode267
Line 267: void RestartTask();
could we get a more descriptive name?

[Alpha Left Google, 2009-10-15 01:35:36]

http://codereview.chromium.org/269002/diff/1001/2005
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/269002/diff/1001/2005#newcode145
Line 145: if (!buffer_.get())
On 2009/10/15 00:15:54, scherkus wrote:
> hmm the next line destroys the internal buffer
> 
> does Stop() get called multiple times?

It doesn't, but just in case it does.. I'm being a bit nervous about this...

http://codereview.chromium.org/269002/diff/1001/2005#newcode800
Line 800: streaming_ = instance_size == kPositionNotSpecified ||
!partial_response;
On 2009/10/15 00:15:54, scherkus wrote:
> would recommend brackets around instance_size == kPositionNotSpecified for
> clarity

Done.

http://codereview.chromium.org/269002/diff/1001/2005#newcode836
Line 836: host()->SetLoaded(loaded_);
On 2009/10/15 00:15:54, scherkus wrote:
> so really loaded_ here is still false

Done.

http://codereview.chromium.org/269002/diff/1001/2005#newcode947
Line 947: if (loaded_)
On 2009/10/15 00:15:54, scherkus wrote:
> if this is the only reason for having loaded, you could use
> !IsHttpProtocol(url_) instead and do without the extra variable
> 
> loaded_ seems misleading because it really means "we're using a non-http
> resource"
> 
> what you're doing here is suppressing network activity events for non-http
> resources.. I think a comment would be good.

Will add a comment, checking the url is bad as this code is getting called very
often...

http://codereview.chromium.org/269002/diff/1001/2006
File webkit/glue/media/buffered_data_source.h (right):

http://codereview.chromium.org/269002/diff/1001/2006#newcode267
Line 267: void RestartTask();
On 2009/10/15 00:15:54, scherkus wrote:
> could we get a more descriptive name?

now called RestartLoadingTask

[Alpha Left Google, 2009-10-15 23:38:19]

ping

[scherkus (not reviewing), 2009-10-15 23:51:38]

one nit but LGTM

what's unfortunate is I think most of this will change after progress events are
removed

http://codereview.chromium.org/269002/diff/7001/7006
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/269002/diff/7001/7006#newcode102
Line 102: Callback0::Type* event_callback) {
use NetworkEventCallback instead