Fold RestartLoadingTask() into ReadCallback() as the method is called on the render thread.

Fold RestartLoadingTask() into ReadCallback() as the method is called on the render thread.

This is the cache miss equivalent of r118386. We were unnecessarily posting a task to restart the HTTP request on a cache miss. On pages with heavy activity this can delay the load until the task can get executed.

BUG=none
TEST=none


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

[scherkus (not reviewing), 2012-07-06 17:25:49]

Depends on https://chromiumcodereview.appspot.com/10735016/

I haven't done the benchmarking w/ a busy page (will be easier on my desktop!)
but we should never-ever risk delaying an HTTP request.

[Ami GONE FROM CHROMIUM, 2012-07-10 02:48:25]

https://chromiumcodereview.appspot.com/10700125/diff/1/webkit/media/buffered_...
File webkit/media/buffered_data_source.cc (right):

https://chromiumcodereview.appspot.com/10700125/diff/1/webkit/media/buffered_...
webkit/media/buffered_data_source.cc:416: // Recreate a loader starting from
where we last left off until the
Why is it no longer necessary to test stopped_on_render_loop_?

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

https://chromiumcodereview.appspot.com/10700125/diff/1/webkit/media/buffered_...
File webkit/media/buffered_data_source.cc (right):

https://chromiumcodereview.appspot.com/10700125/diff/1/webkit/media/buffered_...
webkit/media/buffered_data_source.cc:416: // Recreate a loader starting from
where we last left off until the
On 2012/07/10 02:48:25, Ami Fischman wrote:
> Why is it no longer necessary to test stopped_on_render_loop_?

Previously the only way that could happen is if CleanupTask() was called before
RestartLoadingTask() was executed.

The only way that could have happened was if WebMediaPlayerImpl::Destroy() was
triggered due to ~WMPI() or the render thread going away.

Since we're on the render thread when we notice a cache miss it's impossible for
us to be stopped so we kick off the read anyway. If CleanupTask() were to
execute immediately after we yield back to the render thread we'd simply stop
the load again via loader_->Stop();

[Ami GONE FROM CHROMIUM, 2012-07-10 03:27:21]

lgtm

[ivan_cobs.com.sg, 2012-07-10 03:47:33]

Pls remove the following email adds from your list NOW

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

-----Original Message-----
From: scherkus@chromium.org [mailto:scherkus@chromium.org] 
Sent: Tuesday, 10 July, 2012 11:15 AM
To: fischman@chromium.org
Cc: chromium-reviews@chromium.org; feature-media-reviews@chromium.org;
darin-cc@chromium.org
Subject: [chromium-reviews] Re: Fold RestartLoadingTask() into
ReadCallback() as the method is called on the render thread. (issue
10700125)


https://chromiumcodereview.appspot.com/10700125/diff/1/webkit/media/buffered
_data_source.cc
File webkit/media/buffered_data_source.cc (right):

https://chromiumcodereview.appspot.com/10700125/diff/1/webkit/media/buffered
_data_source.cc#newcode416
webkit/media/buffered_data_source.cc:416: // Recreate a loader starting from
where we last left off until the On 2012/07/10 02:48:25, Ami Fischman wrote:
> Why is it no longer necessary to test stopped_on_render_loop_?

Previously the only way that could happen is if CleanupTask() was called
before RestartLoadingTask() was executed.

The only way that could have happened was if
WebMediaPlayerImpl::Destroy() was triggered due to ~WMPI() or the render
thread going away.

Since we're on the render thread when we notice a cache miss it's impossible
for us to be stopped so we kick off the read anyway. If
CleanupTask() were to execute immediately after we yield back to the render
thread we'd simply stop the load again via loader_->Stop();

https://chromiumcodereview.appspot.com/10700125/

[scherkus (not reviewing), 2012-07-12 01:54:07]

I did some benchmarking -- on a simulated busy page using rAF() it does indeed
shave off some time (~40ms), which would be more pronounced on a high latency
connection. Yay.

[commit-bot: I haz the power, 2012-07-12 01:54:19]

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

[commit-bot: I haz the power, 2012-07-12 03:20:37]

Change committed as 146281