BufferedDataSource to support server without range request support...

BufferedDataSource to support server without range request support
This patch will enable BufferedDataSource to support servers with
no range request support. It will start a probe request of 1 byte
size besides the regular request. If the server does not support
range request, we will turn on the is_streamed flag of FFmpeg and
will not do any seeking.

BUG=17628
TEST=test_shell_tests --gtest_filter=BufferedDataSource.*


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

[scherkus (not reviewing), 2009-07-24 03:43:44]

initial pass, more to come

http://codereview.chromium.org/160076/diff/22/1033
File media/base/filter_host.h (right):

http://codereview.chromium.org/160076/diff/22/1033#newcode56
Line 56: virtual void SetSeekable(bool seekable) = 0;
add a comment

http://codereview.chromium.org/160076/diff/22/1032
File media/base/filters.h (right):

http://codereview.chromium.org/160076/diff/22/1032#newcode158
Line 158: // Returns true if this data source supports random seeking.
update comment

http://codereview.chromium.org/160076/diff/22/1031
File media/base/mock_filter_host.h (right):

http://codereview.chromium.org/160076/diff/22/1031#newcode39
Line 39: MOCK_METHOD1(SetSeekable, void(bool seekable));
see other comment re: Seekable vs. Streamed

http://codereview.chromium.org/160076/diff/22/1030
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/160076/diff/22/1030#newcode53
Line 53: virtual bool IsSeekable() const;
I think it'd be clearer if we maintained the same interface throughout, so name
this IsStreamed()

otherwise it becomes easy to slip in bugs because IsStreamed() == !IsSeekable()

http://codereview.chromium.org/160076/diff/22/1030#newcode119
Line 119: bool seekable_;
comment, even if it's just "Set by filters."

http://codereview.chromium.org/160076/diff/22/1035
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/160076/diff/22/1035#newcode569
Line 569: // If the probe request is still active, stops it too.
stops -> stop

http://codereview.chromium.org/160076/diff/22/1035#newcode571
Line 571: probe_loader_->Stop();
should you also release probe_loader_?

http://codereview.chromium.org/160076/diff/22/1035#newcode652
Line 652: probe_loader_->Stop();
should you also release probe_loader_?

http://codereview.chromium.org/160076/diff/22/1035#newcode690
Line 690: if (error != net::OK) {
comment what we're looking for here

if it's a 206 response (good) is error OK?

wouldn't a 200 response also be OK, or will net return something else

http://codereview.chromium.org/160076/diff/22/1035#newcode696
Line 696: probe_loader_->Stop();
should you also release probe_loader_?

http://codereview.chromium.org/160076/diff/22/1035#newcode698
Line 698: if (initial_loader_started_)
see I find these started_ variables a little misleading because you're stopping
the loader :)

maybe "probe_completed_" or what about a counter for start callbacks?

then you could call a function like LoaderStarted() which could increment the
counter and call DoneInitialization()

hmm I don't know.. it's up to you what you find clearer

http://codereview.chromium.org/160076/diff/22/1035#newcode711
Line 711: loader_->Stop();
hmm I don't know enough about loaders, but after stopping you don't have to set
to NULL?

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

http://codereview.chromium.org/160076/diff/22/1036#newcode66
Line 66: virtual int64 instance_size() { return instance_size_; }
what's instance size?

http://codereview.chromium.org/160076/diff/22/1036#newcode224
Line 224: // Callback method for |loader_|. This method is called when respopnse
for
respopnse -> response

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

http://codereview.chromium.org/160076/diff/22/1039#newcode453
Line 453: EXPECT_CALL(host_, SetSeekable(false));
macro without {}

http://codereview.chromium.org/160076/diff/22/1039#newcode482
Line 482: EXPECT_FALSE(data_source_->IsStreamed());
macro without {}

http://codereview.chromium.org/160076/diff/22/1039#newcode491
Line 491: EXPECT_TRUE(data_source_->IsStreamed());
macro without {}

http://codereview.chromium.org/160076/diff/22/1040
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/160076/diff/22/1040#newcode320
Line 320: if (pipeline_->IsSeekable())
add todo saying we're not telling the truth :)

also I believe webkit added a flag for streaming, which might be worth looking
into

[Alpha Left Google, 2009-07-24 21:11:09]

http://codereview.chromium.org/160076/diff/22/1033
File media/base/filter_host.h (right):

http://codereview.chromium.org/160076/diff/22/1033#newcode56
Line 56: virtual void SetSeekable(bool seekable) = 0;
On 2009/07/24 03:43:44, scherkus wrote:
> add a comment

Done.

http://codereview.chromium.org/160076/diff/22/1032
File media/base/filters.h (right):

http://codereview.chromium.org/160076/diff/22/1032#newcode158
Line 158: // Returns true if this data source supports random seeking.
On 2009/07/24 03:43:44, scherkus wrote:
> update comment

Done.

http://codereview.chromium.org/160076/diff/22/1031
File media/base/mock_filter_host.h (right):

http://codereview.chromium.org/160076/diff/22/1031#newcode39
Line 39: MOCK_METHOD1(SetSeekable, void(bool seekable));
On 2009/07/24 03:43:44, scherkus wrote:
> see other comment re: Seekable vs. Streamed

Done.

http://codereview.chromium.org/160076/diff/22/1030
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/160076/diff/22/1030#newcode53
Line 53: virtual bool IsSeekable() const;
On 2009/07/24 03:43:44, scherkus wrote:
> I think it'd be clearer if we maintained the same interface throughout, so
name
> this IsStreamed()
> 
> otherwise it becomes easy to slip in bugs because IsStreamed() ==
!IsSeekable()

Done.

http://codereview.chromium.org/160076/diff/22/1035
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/160076/diff/22/1035#newcode569
Line 569: // If the probe request is still active, stops it too.
On 2009/07/24 03:43:44, scherkus wrote:
> stops -> stop

Done.

http://codereview.chromium.org/160076/diff/22/1035#newcode571
Line 571: probe_loader_->Stop();
On 2009/07/24 03:43:44, scherkus wrote:
> should you also release probe_loader_?

It may get stopped at other places, it's good to have it there not active, so I
don't need to care about if (loader_.get()) everywhere before stopping them.

http://codereview.chromium.org/160076/diff/22/1035#newcode652
Line 652: probe_loader_->Stop();
On 2009/07/24 03:43:44, scherkus wrote:
> should you also release probe_loader_?

It's the same reason as before, I want to keep it around, especially I will have
use of it later.

http://codereview.chromium.org/160076/diff/22/1035#newcode690
Line 690: if (error != net::OK) {
On 2009/07/24 03:43:44, scherkus wrote:
> comment what we're looking for here
> 
> if it's a 206 response (good) is error OK?
> 
> wouldn't a 200 response also be OK, or will net return something else

net::OK is my own definition of OK regardless of 200 or 206 response from the
server. I added comments about what it means in the loader now.

http://codereview.chromium.org/160076/diff/22/1035#newcode696
Line 696: probe_loader_->Stop();
On 2009/07/24 03:43:44, scherkus wrote:
> should you also release probe_loader_?

I'll keep it around. I'll have use of its data later (to prefetch the index).

http://codereview.chromium.org/160076/diff/22/1035#newcode698
Line 698: if (initial_loader_started_)
On 2009/07/24 03:43:44, scherkus wrote:
> see I find these started_ variables a little misleading because you're
stopping
> the loader :)
> 
> maybe "probe_completed_" or what about a counter for start callbacks?
> 
> then you could call a function like LoaderStarted() which could increment the
> counter and call DoneInitialization()
> 
> hmm I don't know.. it's up to you what you find clearer

It's good to separate them into two different things as I'll have other use of
the probe. Changed them to response_received_.

http://codereview.chromium.org/160076/diff/22/1035#newcode711
Line 711: loader_->Stop();
On 2009/07/24 03:43:44, scherkus wrote:
> hmm I don't know enough about loaders, but after stopping you don't have to
set
> to NULL?

It's fine, they will get cleaned up once this object goes away which will be
soon as we are reporting an error for the read request.

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

http://codereview.chromium.org/160076/diff/22/1036#newcode66
Line 66: virtual int64 instance_size() { return instance_size_; }
On 2009/07/24 03:43:44, scherkus wrote:
> what's instance size?

Done.

http://codereview.chromium.org/160076/diff/22/1036#newcode224
Line 224: // Callback method for |loader_|. This method is called when respopnse
for
On 2009/07/24 03:43:44, scherkus wrote:
> respopnse -> response

Done.

http://codereview.chromium.org/160076/diff/22/1040
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/160076/diff/22/1040#newcode320
Line 320: if (pipeline_->IsSeekable())
On 2009/07/24 03:43:44, scherkus wrote:
> add todo saying we're not telling the truth :)
> 
> also I believe webkit added a flag for streaming, which might be worth looking
> into

Done.

[fbarchard, 2009-07-24 21:48:51]

seems okay to me.  Will this change allow ftp to work as well?

[hclam, 2009-07-24 21:58:23]

I disallowed FTP for now. If it is interesting I can enable FTP in another
patch.
Alpha

2009/7/24 <fbarchard@chromium.org>
>
> seems okay to me.  Will this change allow ftp to work as well?
>
>
> http://codereview.chromium.org/160076
>

[fbarchard1, 2009-07-24 22:01:59]

ya I know and that makes sense.  But I'm just asking, is this enough that we
can now work with protocols like ftp that dont allow seeks?

On Fri, Jul 24, 2009 at 2:57 PM, Alpha (Hin-Chung) Lam <hclam@google.com>wrote:

> I disallowed FTP for now. If it is interesting I can enable FTP in another
> patch.
> Alpha
>
> 2009/7/24 <fbarchard@chromium.org>
>>
>> seems okay to me.  Will this change allow ftp to work as well?
>>
>>
>> http://codereview.chromium.org/160076
>>
>
>

[hclam, 2009-07-24 22:08:17]

Correct, but this mode is very restricted:
1. No seeking
2. Connection timeout or closed then there's no way to recover

This is essentially the same as live streaming. And we can't tolerate a
single network error... :-/

Alpha

2009/7/24 Frank Barchard <fbarchard@google.com>

> ya I know and that makes sense.  But I'm just asking, is this enough that
> we can now work with protocols like ftp that dont allow seeks?
>
>
> On Fri, Jul 24, 2009 at 2:57 PM, Alpha (Hin-Chung) Lam
<hclam@google.com>wrote:
>
>> I disallowed FTP for now. If it is interesting I can enable FTP in another
>> patch.
>> Alpha
>>
>> 2009/7/24 <fbarchard@chromium.org>
>>>
>>> seems okay to me.  Will this change allow ftp to work as well?
>>>
>>>
>>> http://codereview.chromium.org/160076
>>>
>>
>>
>

[scherkus (not reviewing), 2009-07-24 22:16:29]

I wouldn't worry about FTP.
Bigger fish to fry :)

On Fri, Jul 24, 2009 at 3:07 PM, Alpha (Hin-Chung) Lam <hclam@google.com>wrote:

> Correct, but this mode is very restricted:
> 1. No seeking
> 2. Connection timeout or closed then there's no way to recover
>
> This is essentially the same as live streaming. And we can't tolerate a
> single network error... :-/
>
> Alpha
>
> 2009/7/24 Frank Barchard <fbarchard@google.com>
>
> ya I know and that makes sense.  But I'm just asking, is this enough that
>> we can now work with protocols like ftp that dont allow seeks?
>>
>>
>> On Fri, Jul 24, 2009 at 2:57 PM, Alpha (Hin-Chung) Lam
<hclam@google.com>wrote:
>>
>>> I disallowed FTP for now. If it is interesting I can enable FTP in
>>> another patch.
>>> Alpha
>>>
>>> 2009/7/24 <fbarchard@chromium.org>
>>>>
>>>> seems okay to me.  Will this change allow ftp to work as well?
>>>>
>>>>
>>>> http://codereview.chromium.org/160076
>>>>
>>>
>>>
>>
>

[fbarchard1, 2009-07-24 22:55:31]

So this passes thru to ffmpeg isstreamed eh.Thats got 2 bugs
if you play an mp4 with header at the beginning, and then you play an mp4
with header at the end, the second movie will use the header of the first
ogg will return duration is zero.
and it doesnt prevent seeking... is just discourages it.  if the header is
at the end, it will seek there.
if the user seeks, a seek will happen.

On Fri, Jul 24, 2009 at 3:15 PM, Andrew Scherkus <scherkus@chromium.org>wrote:

> I wouldn't worry about FTP.
> Bigger fish to fry :)
>
>
> On Fri, Jul 24, 2009 at 3:07 PM, Alpha (Hin-Chung) Lam
<hclam@google.com>wrote:
>
>> Correct, but this mode is very restricted:
>> 1. No seeking
>> 2. Connection timeout or closed then there's no way to recover
>>
>> This is essentially the same as live streaming. And we can't tolerate a
>> single network error... :-/
>>
>> Alpha
>>
>> 2009/7/24 Frank Barchard <fbarchard@google.com>
>>
>> ya I know and that makes sense.  But I'm just asking, is this enough that
>>> we can now work with protocols like ftp that dont allow seeks?
>>>
>>>
>>> On Fri, Jul 24, 2009 at 2:57 PM, Alpha (Hin-Chung) Lam <hclam@google.com
>>> > wrote:
>>>
>>>> I disallowed FTP for now. If it is interesting I can enable FTP in
>>>> another patch.
>>>> Alpha
>>>>
>>>> 2009/7/24 <fbarchard@chromium.org>
>>>>>
>>>>> seems okay to me.  Will this change allow ftp to work as well?
>>>>>
>>>>>
>>>>> http://codereview.chromium.org/160076
>>>>>
>>>>
>>>>
>>>
>>
>

[awong, 2009-07-27 21:44:42]

couple of comments regarding naming.  seems sane.  I need to give the buffer
code one more look over.

http://codereview.chromium.org/160076/diff/1086/1094
File media/base/filter_host.h (right):

http://codereview.chromium.org/160076/diff/1086/1094#newcode57
Line 57: virtual void SetStreamed(bool seekable) = 0;
SetStreaming(bool streaming)

http://codereview.chromium.org/160076/diff/1086/1093
File media/base/filters.h (right):

http://codereview.chromium.org/160076/diff/1086/1093#newcode160
Line 160: virtual bool IsStreamed() = 0;
IsStreamed -> IsStreaming

http://codereview.chromium.org/160076/diff/1086/1091
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/160076/diff/1086/1091#newcode224
Line 224: // Sets by the filters to indicate whehter we are performing
streaming.
we're not preforming streaming right?  The source is a streaming source right?

"whether the data source is a streaming source"

http://codereview.chromium.org/160076/diff/1086/1103
File webkit/api/src/WebMediaPlayerClientImpl.cpp (right):

http://codereview.chromium.org/160076/diff/1086/1103#newcode387
Line 387: return
static_cast<MediaPlayer::MovieLoadType>(m_webMediaPlayer->movieLoadType());
80-chars

http://codereview.chromium.org/160076/diff/1086/1097
File webkit/glue/media/buffered_data_source.h (right):

http://codereview.chromium.org/160076/diff/1086/1097#newcode83
Line 83: virtual int64 instance_size() { return instance_size_; }
Why isn't this original_size()?

http://codereview.chromium.org/160076/diff/1086/1097#newcode241
Line 241: // Callback method for |loader_|. This method is called when response
for
the second sentence doesn't parse.  What do you mean "when response for initial
request has started"?

http://codereview.chromium.org/160076/diff/1086/1097#newcode284
Line 284: // Filter callbacks.
Not a useful comment.  Expand please.

http://codereview.chromium.org/160076/diff/1086/1099
File webkit/glue/media/simple_data_source.h (right):

http://codereview.chromium.org/160076/diff/1086/1099#newcode47
Line 47: virtual bool IsStreamed();
I'm confused why this is changed from IsSeekable to IsSteramed...

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

http://codereview.chromium.org/160076/diff/1086/1093
File media/base/filters.h (right):

http://codereview.chromium.org/160076/diff/1086/1093#newcode160
Line 160: virtual bool IsStreamed() = 0;
On 2009/07/27 21:44:42, awong wrote:
> IsStreamed -> IsStreaming

Done.

http://codereview.chromium.org/160076/diff/1086/1091
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/160076/diff/1086/1091#newcode224
Line 224: // Sets by the filters to indicate whehter we are performing
streaming.
On 2009/07/27 21:44:42, awong wrote:
> we're not preforming streaming right?  The source is a streaming source right?
> 
> "whether the data source is a streaming source"

Done.

http://codereview.chromium.org/160076/diff/1086/1103
File webkit/api/src/WebMediaPlayerClientImpl.cpp (right):

http://codereview.chromium.org/160076/diff/1086/1103#newcode387
Line 387: return
static_cast<MediaPlayer::MovieLoadType>(m_webMediaPlayer->movieLoadType());
On 2009/07/27 21:44:42, awong wrote:
> 80-chars

Done.

http://codereview.chromium.org/160076/diff/1086/1097
File webkit/glue/media/buffered_data_source.h (right):

http://codereview.chromium.org/160076/diff/1086/1097#newcode83
Line 83: virtual int64 instance_size() { return instance_size_; }
On 2009/07/27 21:44:42, awong wrote:
> Why isn't this original_size()?

This is to mirror the naming of the HTTP spec for range request.

http://codereview.chromium.org/160076/diff/1086/1097#newcode241
Line 241: // Callback method for |loader_|. This method is called when response
for
On 2009/07/27 21:44:42, awong wrote:
> the second sentence doesn't parse.  What do you mean "when response for
initial
> request has started"?

Done.

http://codereview.chromium.org/160076/diff/1086/1097#newcode284
Line 284: // Filter callbacks.
On 2009/07/27 21:44:42, awong wrote:
> Not a useful comment.  Expand please.

Done.

http://codereview.chromium.org/160076/diff/1086/1099
File webkit/glue/media/simple_data_source.h (right):

http://codereview.chromium.org/160076/diff/1086/1099#newcode47
Line 47: virtual bool IsStreamed();
On 2009/07/27 21:44:42, awong wrote:
> I'm confused why this is changed from IsSeekable to IsSteramed...

It would a little strange because we have IsSeekable for datasource, IsStreamed
for pipeline and IsStreamed for the ffmpeg glue.. so I name them all to
IsStreamed.

[fbarchard, 2009-07-28 20:55:57]

There are bugs in mp4 and ogg with isstreamed and it would be good to do more
testing.  Could you show me, how to test this, so I can revisit with ffmpeg
patches?
Perhaps add todo somewhere on future work, which is
1. fixed bugs in mp4/ogg
2. enable streaming all the time, if bugs are fixed
3. enable ftp

http://codereview.chromium.org/160076/diff/1086/1093
File media/base/filters.h (right):

http://codereview.chromium.org/160076/diff/1086/1093#newcode160
Line 160: virtual bool IsStreamed() = 0;
is_streamed is the ffmpeg name for this. shouldnt we be consistent?  Now we have
3 names for the same thing
is_seekable, IsStreaming and is_streamed.

http://codereview.chromium.org/160076/diff/1086/1090
File media/base/mock_filters.h (right):

http://codereview.chromium.org/160076/diff/1086/1090#newcode105
Line 105: MOCK_METHOD0(IsStreamed, bool());
oh ok... this is good.

http://codereview.chromium.org/160076/diff/1086/1097
File webkit/glue/media/buffered_data_source.h (right):

http://codereview.chromium.org/160076/diff/1086/1097#newcode273
Line 273: bool streaming_;
is_streamed_ would be more consistent

http://codereview.chromium.org/160076/diff/1086/1097#newcode296
Line 296: // This flag is set to true if the probe request has started.
this comment and the name of the variable seem at odds.

[Alpha Left Google, 2009-07-28 21:11:21]

On 2009/07/28 20:55:57, fbarchard wrote:
> There are bugs in mp4 and ogg with isstreamed and it would be good to do more
> testing.  Could you show me, how to test this, so I can revisit with ffmpeg
> patches?
> Perhaps add todo somewhere on future work, which is
> 1. fixed bugs in mp4/ogg
> 2. enable streaming all the time, if bugs are fixed
> 3. enable ftp
> 

Sure I can add those TODOs.

> http://codereview.chromium.org/160076/diff/1086/1093
> File media/base/filters.h (right):
> 
> http://codereview.chromium.org/160076/diff/1086/1093#newcode160
> Line 160: virtual bool IsStreamed() = 0;
> is_streamed is the ffmpeg name for this. shouldnt we be consistent?  Now we
have
> 3 names for the same thing
> is_seekable, IsStreaming and is_streamed.
> 
> http://codereview.chromium.org/160076/diff/1086/1090
> File media/base/mock_filters.h (right):
> 
> http://codereview.chromium.org/160076/diff/1086/1090#newcode105
> Line 105: MOCK_METHOD0(IsStreamed, bool());
> oh ok... this is good.
> 
> http://codereview.chromium.org/160076/diff/1086/1097
> File webkit/glue/media/buffered_data_source.h (right):
> 
> http://codereview.chromium.org/160076/diff/1086/1097#newcode273
> Line 273: bool streaming_;
> is_streamed_ would be more consistent
> 
> http://codereview.chromium.org/160076/diff/1086/1097#newcode296
> Line 296: // This flag is set to true if the probe request has started.
> this comment and the name of the variable seem at odds.

[scherkus (not reviewing), 2009-07-29 18:37:27]

http://codereview.chromium.org/160076/diff/183/1135
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/160076/diff/183/1135#newcode83
Line 83: virtual bool IsStreaming() const;
this method isn't part of the pipeline interface

could you add it to pipeline.h in the same order?

http://codereview.chromium.org/160076/diff/183/1151
File media/filters/ffmpeg_glue_unittest.cc (right):

http://codereview.chromium.org/160076/diff/183/1151#newcode49
Line 49: EXPECT_CALL(*protocol, IsStreaming()).WillOnce(Return(true));
do we want to return true?

wont this affect our seeking tests?  (just curious)

http://codereview.chromium.org/160076/diff/183/1147
File webkit/api/src/WebMediaPlayerClientImpl.cpp (right):

http://codereview.chromium.org/160076/diff/183/1147#newcode373
Line 373: COMPILE_ASSERT(
tricky :)

http://codereview.chromium.org/160076/diff/183/1140
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/160076/diff/183/1140#newcode230
Line 230: error = net::ERR_REQUEST_RANGE_NOT_SATISFIABLE;
could you add a one-line comment explaining what error should be if this if
statement is false?  (even just saying "Otherwise net::OK" would help)

http://codereview.chromium.org/160076/diff/183/1144
File webkit/glue/media/buffered_data_source_unittest.cc (right):

http://codereview.chromium.org/160076/diff/183/1144#newcode445
Line 445: EXPECT_CALL(host_, SetStreaming(true));
nice -- I love being able to expect these calls!

http://codereview.chromium.org/160076/diff/183/1144#newcode483
Line 483: EXPECT_FALSE(data_source_->IsStreaming());
warning: macro without using {}

http://codereview.chromium.org/160076/diff/183/1144#newcode492
Line 492: EXPECT_TRUE(data_source_->IsStreaming());
warning: macro without using {}

http://codereview.chromium.org/160076/diff/183/1146
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/160076/diff/183/1146#newcode278
Line 278: } else {
no need for else

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

http://codereview.chromium.org/160076/diff/183/1135
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/160076/diff/183/1135#newcode83
Line 83: virtual bool IsStreaming() const;
On 2009/07/29 18:37:27, scherkus wrote:
> this method isn't part of the pipeline interface
> 
> could you add it to pipeline.h in the same order?

Done.

http://codereview.chromium.org/160076/diff/183/1151
File media/filters/ffmpeg_glue_unittest.cc (right):

http://codereview.chromium.org/160076/diff/183/1151#newcode49
Line 49: EXPECT_CALL(*protocol, IsStreaming()).WillOnce(Return(true));
On 2009/07/29 18:37:27, scherkus wrote:
> do we want to return true?
> 
> wont this affect our seeking tests?  (just curious)

This is just a function name change, to make it consistently IsStreaming, this
still returns true.

http://codereview.chromium.org/160076/diff/183/1140
File webkit/glue/media/buffered_data_source.cc (right):

http://codereview.chromium.org/160076/diff/183/1140#newcode230
Line 230: error = net::ERR_REQUEST_RANGE_NOT_SATISFIABLE;
On 2009/07/29 18:37:27, scherkus wrote:
> could you add a one-line comment explaining what error should be if this if
> statement is false?  (even just saying "Otherwise net::OK" would help)

Done.

http://codereview.chromium.org/160076/diff/183/1144
File webkit/glue/media/buffered_data_source_unittest.cc (right):

http://codereview.chromium.org/160076/diff/183/1144#newcode483
Line 483: EXPECT_FALSE(data_source_->IsStreaming());
On 2009/07/29 18:37:27, scherkus wrote:
> warning: macro without using {}

Done.

http://codereview.chromium.org/160076/diff/183/1144#newcode492
Line 492: EXPECT_TRUE(data_source_->IsStreaming());
On 2009/07/29 18:37:27, scherkus wrote:
> warning: macro without using {}

Done.

http://codereview.chromium.org/160076/diff/183/1146
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/160076/diff/183/1146#newcode278
Line 278: } else {
On 2009/07/29 18:37:27, scherkus wrote:
> no need for else

Done.

[scherkus (not reviewing), 2009-07-29 20:24:44]

LGTM from my side