Changing constructor of BufferedDataSource to accept GURL and CORSMode

Changing constructor of BufferedDataSource to accept GURL and CORSMode

BufferedDataSource should accept the 'url' and the 'CORS mode' parameters
in the constructor instead via the initialize() method. This patch makes
this change and now only the 'initializeCallBack' is passed to initialize()
method. Also now the WebMediaPlayerImpl directly queries the BufferedDataSource
whether the resource is local or not, instead of finding it out itself.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274513

[amogh.bihani, 2014-05-30 09:32:44]

This is in accordance to what we discussed in
https://codereview.chromium.org/302553006/#msg5.
PTAL.

[scherkus (not reviewing), 2014-05-30 20:45:14]

thanks for tackling this!

I've got some suggestions on other cleanup this CL should include

https://codereview.chromium.org/306953005/diff/1/content/renderer/media/buffe...
File content/renderer/media/buffered_data_source.cc (right):

https://codereview.chromium.org/306953005/diff/1/content/renderer/media/buffe...
content/renderer/media/buffered_data_source.cc:151: assume_fully_buffered_ =
true;
this boolean isn't needed -- it's essentially a duplicate check for
!url_.SchemeIsHTTPOrHTTPS()

in which case we should add a public getter on this class:

bool assume_fully_buffered() { return !url_.SchemeIsHTTPOrHTTPS(); }


then in WebMediaPlayerImpl we can also replace is_local_source_ with
data_source_->assume_fully_buffered()

https://codereview.chromium.org/306953005/diff/1/content/renderer/media/buffe...
File content/renderer/media/buffered_data_source.h (right):

https://codereview.chromium.org/306953005/diff/1/content/renderer/media/buffe...
content/renderer/media/buffered_data_source.h:64: // Initialize this object
using |url| and |cors_mode|, executing |init_cb|
please update the comments here and on the constructor to reflect the changes in
API

https://codereview.chromium.org/306953005/diff/1/content/renderer/media/webme...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/306953005/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.cc:1116: void
WebMediaPlayerImpl::DataSourceInitialized(const GURL& gurl, bool success) {
it doesn't look like |gurl| is used -- can you also clean this up and the
callback to BufferedDataSource::Initialize() on line 329?

[amogh.bihani, 2014-06-02 06:11:01]

Thanks for the review. :)
I have made the changes, PTAL.

https://codereview.chromium.org/306953005/diff/1/content/renderer/media/buffe...
File content/renderer/media/buffered_data_source.cc (right):

https://codereview.chromium.org/306953005/diff/1/content/renderer/media/buffe...
content/renderer/media/buffered_data_source.cc:151: assume_fully_buffered_ =
true;
On 2014/05/30 20:45:14, scherkus wrote:
> this boolean isn't needed -- it's essentially a duplicate check for
> !url_.SchemeIsHTTPOrHTTPS()
> 
> in which case we should add a public getter on this class:
> 
> bool assume_fully_buffered() { return !url_.SchemeIsHTTPOrHTTPS(); }
> 
> 
> then in WebMediaPlayerImpl we can also replace is_local_source_ with
> data_source_->assume_fully_buffered()

Done.

https://codereview.chromium.org/306953005/diff/1/content/renderer/media/buffe...
File content/renderer/media/buffered_data_source.h (right):

https://codereview.chromium.org/306953005/diff/1/content/renderer/media/buffe...
content/renderer/media/buffered_data_source.h:64: // Initialize this object
using |url| and |cors_mode|, executing |init_cb|
On 2014/05/30 20:45:14, scherkus wrote:
> please update the comments here and on the constructor to reflect the changes
in
> API

Done.

https://codereview.chromium.org/306953005/diff/1/content/renderer/media/webme...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/306953005/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.cc:1116: void
WebMediaPlayerImpl::DataSourceInitialized(const GURL& gurl, bool success) {
On 2014/05/30 20:45:14, scherkus wrote:
> it doesn't look like |gurl| is used -- can you also clean this up and the
> callback to BufferedDataSource::Initialize() on line 329?

Done.

[scherkus (not reviewing), 2014-06-02 20:11:23]

lgtm w/ nit

https://codereview.chromium.org/306953005/diff/40001/content/renderer/media/b...
File content/renderer/media/buffered_data_source.h (right):

https://codereview.chromium.org/306953005/diff/40001/content/renderer/media/b...
content/renderer/media/buffered_data_source.h:97: inline bool
assume_fully_buffered() { return !url_.SchemeIsHTTPOrHTTPS(); }
nit: don't bother manually specifying "inline" -- the compiler will take of it
if needed

[amogh.bihani, 2014-06-03 09:10:28]

The CQ bit was checked by amogh.bihani@samsung.com

[commit-bot: I haz the power, 2014-06-03 09:11:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/amogh.bihani@samsung.com/306953005/50001

[commit-bot: I haz the power, 2014-06-03 10:49:57]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/12163)

[commit-bot: I haz the power, 2014-06-03 13:43:49]

Change committed as 274513