Buffered time ranges for <video>

Buffered time ranges for <video>

API change and related changes to support multiple buffered time ranges.
This change doesn't change any behavior.

BUG=21305
TEST=Open a video or audio file, you see the full bar of buffered ranges.

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

[Alpha Left Google, 2009-09-09 00:26:33]

This is a two-sided change, upstream patch:
https://bugs.webkit.org/show_bug.cgi?id=29041

[scherkus (not reviewing), 2009-09-09 00:46:25]

Looks like a candidate for BUG=21305 :)

[Alpha Left Google, 2009-09-09 01:19:06]

Added BUG=, any concerns?

[scherkus (not reviewing), 2009-09-09 01:44:53]

sorry still reviewing

On Tue, Sep 8, 2009 at 6:19 PM, <hclam@chromium.org> wrote:

>
> Added BUG=, any concerns?
>
>
> http://codereview.chromium.org/199037
>

[scherkus (not reviewing), 2009-09-09 06:07:06]

darin needs to confirm the WebVector vs. std::vector usage, other than that
change is looking OK

http://codereview.chromium.org/199037/diff/2001/2002
File webkit/api/public/WebMediaPlayer.h (right):

http://codereview.chromium.org/199037/diff/2001/2002#newcode51
Line 51: typedef std::vector<WebTimeRange> WebTimeRanges;
hmm darin should be able to confirm but I believe you need to use WebVector.h
and avoid std types

http://codereview.chromium.org/199037/diff/2001/2003
File webkit/api/src/WebMediaPlayerClientImpl.cpp (right):

http://codereview.chromium.org/199037/diff/2001/2003#newcode262
Line 262: // FIXME: Save the time ranges in a member variable and updates it
when needed.
updates -> update

http://codereview.chromium.org/199037/diff/2001/2003#newcode263
Line 263: WTF::RefPtr<TimeRanges> ranges = TimeRanges::create();
can we file a bug to tackle this in the future?  we could get slammed if people
are update their UI too much

http://codereview.chromium.org/199037/diff/2001/2005
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/199037/diff/2001/2005#newcode469
Line 469: buffered_.push_back(
hmm... good thing WebKit doesn't try to re-use this object, otherwise we'd have
an issue if the pipeline was ever initialized twice

I was going to suggest calling clear() here, but it doesn't quite make sense
since this whole class isn't designed to be re-used

mind adding an assert in load() to check if the pipeline has already been
loaded?

http://codereview.chromium.org/199037/diff/2001/2006
File webkit/glue/webmediaplayer_impl.h (right):

http://codereview.chromium.org/199037/diff/2001/2006#newcode176
Line 176: virtual const WebKit::WebTimeRanges& buffered() const { return
buffered_; }
nit: can we move this back into the .cc file with our message loop DCHECK?

[Alpha Left Google, 2009-09-09 22:32:01]

http://codereview.chromium.org/199037/diff/2001/2002
File webkit/api/public/WebMediaPlayer.h (right):

http://codereview.chromium.org/199037/diff/2001/2002#newcode51
Line 51: typedef std::vector<WebTimeRange> WebTimeRanges;
On 2009/09/09 06:07:06, scherkus wrote:
> hmm darin should be able to confirm but I believe you need to use WebVector.h
> and avoid std types

But WebVector is supposed to be static.. I can do a std::vector => WebVector
everytime.. but that would cause extra copy which isn't needed..
Then we'll have so many copies:

std::vector => WebKit::WebVector => WebCore::TimeRanges..

http://codereview.chromium.org/199037/diff/2001/2003
File webkit/api/src/WebMediaPlayerClientImpl.cpp (right):

http://codereview.chromium.org/199037/diff/2001/2003#newcode262
Line 262: // FIXME: Save the time ranges in a member variable and updates it
when needed.
On 2009/09/09 06:07:06, scherkus wrote:
> updates -> update

Done.

http://codereview.chromium.org/199037/diff/2001/2003#newcode263
Line 263: WTF::RefPtr<TimeRanges> ranges = TimeRanges::create();
On 2009/09/09 06:07:06, scherkus wrote:
> can we file a bug to tackle this in the future?  we could get slammed if
people
> are update their UI too much

Yup, when this gets in I'll file a bug.

http://codereview.chromium.org/199037/diff/2001/2005
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/199037/diff/2001/2005#newcode469
Line 469: buffered_.push_back(
On 2009/09/09 06:07:06, scherkus wrote:
> hmm... good thing WebKit doesn't try to re-use this object, otherwise we'd
have
> an issue if the pipeline was ever initialized twice
> 
> I was going to suggest calling clear() here, but it doesn't quite make sense
> since this whole class isn't designed to be re-used
> 
> mind adding an assert in load() to check if the pipeline has already been
> loaded?

I don't think is necessary.. since this class is not designed to be re-used..
Doing so would introduce an additional member variable just for the assert
purpose.

http://codereview.chromium.org/199037/diff/2001/2006
File webkit/glue/webmediaplayer_impl.h (right):

http://codereview.chromium.org/199037/diff/2001/2006#newcode176
Line 176: virtual const WebKit::WebTimeRanges& buffered() const { return
buffered_; }
On 2009/09/09 06:07:06, scherkus wrote:
> nit: can we move this back into the .cc file with our message loop DCHECK?

Done.

[scherkus (not reviewing), 2009-09-09 23:03:53]

cool!  lgtm

http://codereview.chromium.org/199037/diff/2001/2002
File webkit/api/public/WebMediaPlayer.h (right):

http://codereview.chromium.org/199037/diff/2001/2002#newcode51
Line 51: typedef std::vector<WebTimeRange> WebTimeRanges;
On 2009/09/09 22:32:01, Alpha wrote:
> On 2009/09/09 06:07:06, scherkus wrote:
> > hmm darin should be able to confirm but I believe you need to use
WebVector.h
> > and avoid std types
> 
> But WebVector is supposed to be static.. I can do a std::vector => WebVector
> everytime.. but that would cause extra copy which isn't needed..
> Then we'll have so many copies:
> 
> std::vector => WebKit::WebVector => WebCore::TimeRanges..
> 

Ok!  Looks like other files do use some std stuff (not to mention /base,
/googleurl) so looks good

[darin (slow to review), 2009-10-02 17:07:30]

Please use WebVector instead.  I know it might mean an extra copy, but we can
optimize that later.

(I have been thinking of just making WebVector be implemented as std::vector for
Chromium, but WebVector needs to work for consumers such as Android who do not
have a STL implementation.)