Split BufferedResourceLoader's network callback into separate loading state and progress callbacks.

Split BufferedResourceLoader's network callback into separate loading state and progress callbacks.

BufferedDataSource now receives buffering updates separate from loading state updates and no longer has to query the loader for what changed.

BUG=none
TEST=none


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

[scherkus (not reviewing), 2012-07-06 16:28:35]

http://codereview.chromium.org/10692106/diff/3008/webkit/media/buffered_data_...
File webkit/media/buffered_data_source.cc (right):

http://codereview.chromium.org/10692106/diff/3008/webkit/media/buffered_data_...
webkit/media/buffered_data_source.cc:26: // loading/progress related callbacks.
see http://codereview.chromium.org/10692106/ -- I ended up deleting this

[Ami GONE FROM CHROMIUM, 2012-07-09 00:25:46]

LGTM++

http://codereview.chromium.org/10692106/diff/3008/webkit/media/buffered_data_...
File webkit/media/buffered_data_source_unittest.cc (left):

http://codereview.chromium.org/10692106/diff/3008/webkit/media/buffered_data_...
webkit/media/buffered_data_source_unittest.cc:439: EXPECT_CALL(host_,
SetNetworkActivity(false));
So this is not expected to fire anymore?

http://codereview.chromium.org/10692106/diff/3008/webkit/media/buffered_resou...
File webkit/media/buffered_resource_loader.h (left):

http://codereview.chromium.org/10692106/diff/3008/webkit/media/buffered_resou...
webkit/media/buffered_resource_loader.h:118: int64 GetBufferedPosition();
hawt!

http://codereview.chromium.org/10692106/diff/3008/webkit/media/buffered_resou...
File webkit/media/buffered_resource_loader.h (right):

http://codereview.chromium.org/10692106/diff/3008/webkit/media/buffered_resou...
webkit/media/buffered_resource_loader.h:95: // |loading_cb| is executed when the
loading state has changed.
Flip order of these two lines to match the orders below.

http://codereview.chromium.org/10692106/diff/3008/webkit/media/buffered_resou...
webkit/media/buffered_resource_loader.h:288: LoadingCB loading_cb_;
LoadingStateChangedCB?
LoadingCB sounds to me like something that'll fire exactly once (when loading
kicks off).  But LSCCB is a mouthful...
Up to you, just wanted to flag opportunity for improved naming.

http://codereview.chromium.org/10692106/diff/3008/webkit/media/buffered_resou...
webkit/media/buffered_resource_loader.h:291: // zero-indexed file offset of the
leading buffered byte.
s/leading/furthest/?

http://codereview.chromium.org/10692106/diff/3008/webkit/media/buffered_resou...
File webkit/media/buffered_resource_loader_unittest.cc (left):

http://codereview.chromium.org/10692106/diff/3008/webkit/media/buffered_resou...
webkit/media/buffered_resource_loader_unittest.cc:271:
EXPECT_EQ(loader_->active_loader_->deferred(), expectedVal);
Does allow de-friend'ing some tests from BRL?

[Ami GONE FROM CHROMIUM, 2012-07-09 03:25:10]

http://codereview.chromium.org/10692106/diff/3008/webkit/media/buffered_data_...
File webkit/media/buffered_data_source.cc (right):

http://codereview.chromium.org/10692106/diff/3008/webkit/media/buffered_data_...
webkit/media/buffered_data_source.cc:26: // loading/progress related callbacks.
On 2012/07/06 16:28:35, scherkus wrote:
> see http://codereview.chromium.org/10692106/ -- I ended up deleting this

FTR, you mean https://chromiumcodereview.appspot.com/10735016/

[scherkus (not reviewing), 2012-07-09 18:53:30]

http://codereview.chromium.org/10692106/diff/3008/webkit/media/buffered_data_...
File webkit/media/buffered_data_source_unittest.cc (left):

http://codereview.chromium.org/10692106/diff/3008/webkit/media/buffered_data_...
webkit/media/buffered_data_source_unittest.cc:439: EXPECT_CALL(host_,
SetNetworkActivity(false));
On 2012/07/09 00:25:46, Ami Fischman wrote:
> So this is not expected to fire anymore?

The call to Stop() below simulates page navigation by calling didFail() on the
loader

Prior to this CL it would trigger a network event leading to these expectations.
Now that it triggers a load state change of kLoadingFailed we no longer report
anything outside of BDS (see TODO inside BDS::HttpLoadingCallback)

http://codereview.chromium.org/10692106/diff/3008/webkit/media/buffered_resou...
File webkit/media/buffered_resource_loader.h (right):

http://codereview.chromium.org/10692106/diff/3008/webkit/media/buffered_resou...
webkit/media/buffered_resource_loader.h:95: // |loading_cb| is executed when the
loading state has changed.
On 2012/07/09 00:25:46, Ami Fischman wrote:
> Flip order of these two lines to match the orders below.

Done.

http://codereview.chromium.org/10692106/diff/3008/webkit/media/buffered_resou...
webkit/media/buffered_resource_loader.h:288: LoadingCB loading_cb_;
On 2012/07/09 00:25:46, Ami Fischman wrote:
> LoadingStateChangedCB?
> LoadingCB sounds to me like something that'll fire exactly once (when loading
> kicks off).  But LSCCB is a mouthful...
> Up to you, just wanted to flag opportunity for improved naming.

It's a mouthful but I agree that LoadingCB is a bit deceptive

http://codereview.chromium.org/10692106/diff/3008/webkit/media/buffered_resou...
webkit/media/buffered_resource_loader.h:291: // zero-indexed file offset of the
leading buffered byte.
On 2012/07/09 00:25:46, Ami Fischman wrote:
> s/leading/furthest/?

Done.

http://codereview.chromium.org/10692106/diff/3008/webkit/media/buffered_resou...
File webkit/media/buffered_resource_loader_unittest.cc (left):

http://codereview.chromium.org/10692106/diff/3008/webkit/media/buffered_resou...
webkit/media/buffered_resource_loader_unittest.cc:271:
EXPECT_EQ(loader_->active_loader_->deferred(), expectedVal);
On 2012/07/09 00:25:46, Ami Fischman wrote:
> Does allow de-friend'ing some tests from BRL?

Not yet -- BRL is my experiment in friending all of BufferedResourceLoaderTest
(for example we're still accessing buffer_, offset_, first_offset_, etc...)

[scherkus (not reviewing), 2012-07-09 20:35:47]

PTAL -- my source was old and rebasing led to a conflict with your DownloadingCB
change

http://codereview.chromium.org/10692106/diff/14009/webkit/media/buffered_data...
File webkit/media/buffered_data_source_unittest.cc (right):

http://codereview.chromium.org/10692106/diff/14009/webkit/media/buffered_data...
webkit/media/buffered_data_source_unittest.cc:456:
EXPECT_TRUE(data_source_->downloading());
ATTN!

[Ami GONE FROM CHROMIUM, 2012-07-10 02:40:14]

http://codereview.chromium.org/10692106/diff/14009/webkit/media/buffered_data...
File webkit/media/buffered_data_source_unittest.cc (right):

http://codereview.chromium.org/10692106/diff/14009/webkit/media/buffered_data...
webkit/media/buffered_data_source_unittest.cc:456:
EXPECT_TRUE(data_source_->downloading());
This change seems wrong.  Why are we still downloading() after a Stop()/didFail?

http://codereview.chromium.org/10692106/diff/14009/webkit/media/buffered_data...
webkit/media/buffered_data_source_unittest.cc:456:
EXPECT_TRUE(data_source_->downloading());
On 2012/07/09 20:35:48, scherkus wrote:
> ATTN!

Shouldn't BDS be firing a ReadCB(kReadError) in case of BRL::didFail()?

[Ami GONE FROM CHROMIUM, 2012-07-10 02:43:53]

http://codereview.chromium.org/10692106/diff/14009/webkit/media/buffered_data...
File webkit/media/buffered_data_source.cc (right):

http://codereview.chromium.org/10692106/diff/14009/webkit/media/buffered_data...
webkit/media/buffered_data_source.cc:593:
downloading_cb_.Run(is_downloading_data);
This is now run under lock, where it didn't use to be. 
Problem?

[scherkus (not reviewing), 2012-07-10 03:22:19]

http://codereview.chromium.org/10692106/diff/14009/webkit/media/buffered_data...
File webkit/media/buffered_data_source_unittest.cc (right):

http://codereview.chromium.org/10692106/diff/14009/webkit/media/buffered_data...
webkit/media/buffered_data_source_unittest.cc:456:
EXPECT_TRUE(data_source_->downloading());
On 2012/07/10 02:40:14, Ami Fischman wrote:
> On 2012/07/09 20:35:48, scherkus wrote:
> > ATTN!
> 
> Shouldn't BDS be firing a ReadCB(kReadError) in case of BRL::didFail()?

It does and we cover that case somewhat indirectly in the
SetBitrate/SetPlaybackRate tests above. This test doesn't have a pending read
when the test ends (see line 448 where we expect our read to be fulfilled)

http://codereview.chromium.org/10692106/diff/14009/webkit/media/buffered_data...
webkit/media/buffered_data_source_unittest.cc:456:
EXPECT_TRUE(data_source_->downloading());
On 2012/07/10 02:40:14, Ami Fischman wrote:
> This change seems wrong.  Why are we still downloading() after a
Stop()/didFail?

We're not really "downloading" per se -- it's our cached value of downloading_
that we're inspecting here.

Should the API contract when calling data_source_->Stop() be that
downloading_cb_.Run(false) is executed?

Being the people that called data_source_->Stop() in the first place I'd assume
that we know we're not downloading anymore.

[Ami GONE FROM CHROMIUM, 2012-07-10 03:26:28]

LGTM

Right - I think it's fine to not notify anyone about anything while no Read is
in progress.

[ivan_cobs.com.sg, 2012-07-10 03:46:39]

Pls remove the following email adds from your list NOW

ivan@cobs.com.sg
lynn@cobs.com.sg

-----Original Message-----
From: fischman@chromium.org [mailto:fischman@chromium.org] 
Sent: Tuesday, 10 July, 2012 10:40 AM
To: scherkus@chromium.org
Cc: chromium-reviews@chromium.org; feature-media-reviews@chromium.org;
darin-cc@chromium.org
Subject: [chromium-reviews] Re: Split BufferedResourceLoader's network
callback into separate loading state and progress callbacks. (issue
10692106)


http://codereview.chromium.org/10692106/diff/14009/webkit/media/buffered_dat
a_source_unittest.cc
File webkit/media/buffered_data_source_unittest.cc (right):

http://codereview.chromium.org/10692106/diff/14009/webkit/media/buffered_dat
a_source_unittest.cc#newcode456
webkit/media/buffered_data_source_unittest.cc:456:
EXPECT_TRUE(data_source_->downloading());
This change seems wrong.  Why are we still downloading() after a
Stop()/didFail?

http://codereview.chromium.org/10692106/diff/14009/webkit/media/buffered_dat
a_source_unittest.cc#newcode456
webkit/media/buffered_data_source_unittest.cc:456:
EXPECT_TRUE(data_source_->downloading());
On 2012/07/09 20:35:48, scherkus wrote:
> ATTN!

Shouldn't BDS be firing a ReadCB(kReadError) in case of BRL::didFail()?

http://codereview.chromium.org/10692106/

[scherkus (not reviewing), 2012-07-10 04:10:52]

http://codereview.chromium.org/10692106/diff/14009/webkit/media/buffered_data...
File webkit/media/buffered_data_source.cc (right):

http://codereview.chromium.org/10692106/diff/14009/webkit/media/buffered_data...
webkit/media/buffered_data_source.cc:593:
downloading_cb_.Run(is_downloading_data);
On 2012/07/10 02:43:53, Ami Fischman wrote:
> This is now run under lock, where it didn't use to be. 
> Problem?

I don't *think* it should be an issue, but now that downloading_cb != host()
there's no reason for this block of code to be under the lock at all! (the lock
+ stop_signal_received_ is to avoid calling host() which is
Demuxer/Pipeline/etc)

[commit-bot: I haz the power, 2012-07-10 04:11:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scherkus@chromium.org/10692106/15004

[commit-bot: I haz the power, 2012-07-10 05:17:26]

Change committed as 145841