Collapse Should{Enable,Disable}Defer() into ShouldDefer() + simplify some other code.

Collapse Should{Enable,Disable}Defer() into ShouldDefer() + simplify some other code.

BUG=none
TEST=none


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

[scherkus (not reviewing), 2012-07-11 18:11:50]

https://chromiumcodereview.appspot.com/10694138/diff/4001/webkit/media/buffer...
File webkit/media/buffered_resource_loader.cc (left):

https://chromiumcodereview.appspot.com/10694138/diff/4001/webkit/media/buffer...
webkit/media/buffered_resource_loader.cc:659: return !read_cb_.is_null() &&
last_offset_ > buffer_.forward_bytes();
Need some scrutiny here!!!

The last half of this condition isn't really covered any more but I'm trying to
figure out when it would be needed

inside Read() if we can immediately satisfy a read we do so, meaning by time we
UpdateDeferBehavior() read_cb_ is null.

if we hit the WillFulfillRead() codepath inside Read() then we adjust
buffers+capacity+DCHECK() that we'll un-defer

https://chromiumcodereview.appspot.com/10694138/diff/4001/webkit/media/buffer...
File webkit/media/buffered_resource_loader.cc (right):

https://chromiumcodereview.appspot.com/10694138/diff/4001/webkit/media/buffer...
webkit/media/buffered_resource_loader.cc:608: if (!active_loader_.get())
I don't know how useful this method is or whether this should be a DCHECK or
similar (why are we attempting to update the defer behaviour when we don't have
a loader?!)

[Ami GONE FROM CHROMIUM, 2012-07-11 18:36:09]

LGTM

http://codereview.chromium.org/10694138/diff/4001/webkit/media/buffered_resou...
File webkit/media/buffered_resource_loader.cc (left):

http://codereview.chromium.org/10694138/diff/4001/webkit/media/buffered_resou...
webkit/media/buffered_resource_loader.cc:659: return !read_cb_.is_null() &&
last_offset_ > buffer_.forward_bytes();
On 2012/07/11 18:11:51, scherkus wrote:
> Need some scrutiny here!!!
> 
> The last half of this condition isn't really covered any more but I'm trying
to
> figure out when it would be needed
> 
> inside Read() if we can immediately satisfy a read we do so, meaning by time
we
> UpdateDeferBehavior() read_cb_ is null.
> 
> if we hit the WillFulfillRead() codepath inside Read() then we adjust
> buffers+capacity+DCHECK() that we'll un-defer

ISTM the thing to do is simply not call UpdateDeferBehavior in cases where we
know it can't have changed.

http://codereview.chromium.org/10694138/diff/4001/webkit/media/buffered_resou...
File webkit/media/buffered_resource_loader.cc (right):

http://codereview.chromium.org/10694138/diff/4001/webkit/media/buffered_resou...
webkit/media/buffered_resource_loader.cc:624: // Never defer at all.
This (and the comments for the other two cases) doesn't seem to be adding value.

http://codereview.chromium.org/10694138/diff/4001/webkit/media/buffered_resou...
webkit/media/buffered_resource_loader.cc:636: return false;
NOTREACHED?

[scherkus (not reviewing), 2012-07-12 00:21:03]

PTAL... if you dare

https://chromiumcodereview.appspot.com/10694138/diff/4001/webkit/media/buffer...
File webkit/media/buffered_resource_loader.cc (right):

https://chromiumcodereview.appspot.com/10694138/diff/4001/webkit/media/buffer...
webkit/media/buffered_resource_loader.cc:624: // Never defer at all.
On 2012/07/11 18:36:09, Ami Fischman wrote:
> This (and the comments for the other two cases) doesn't seem to be adding
value.

what is this -- webkit!? :P

https://chromiumcodereview.appspot.com/10694138/diff/4001/webkit/media/buffer...
webkit/media/buffered_resource_loader.cc:636: return false;
On 2012/07/11 18:36:09, Ami Fischman wrote:
> NOTREACHED?

Done.

[Ami GONE FROM CHROMIUM, 2012-07-12 00:24:37]

https://chromiumcodereview.appspot.com/10694138/diff/4003/webkit/media/buffer...
File webkit/media/buffered_resource_loader.cc (right):

https://chromiumcodereview.appspot.com/10694138/diff/4003/webkit/media/buffer...
webkit/media/buffered_resource_loader.cc:628: DCHECK(read_cb_.is_null() ||
last_offset_ > buffer_.forward_bytes())
Did you truth-tabling overtake your test-writing??

https://chromiumcodereview.appspot.com/10694138/diff/4003/webkit/media/buffer...
File webkit/media/buffered_resource_loader.h (right):

https://chromiumcodereview.appspot.com/10694138/diff/4003/webkit/media/buffer...
webkit/media/buffered_resource_loader.h:34: // BufferedResourceLoader is single
threaded and must be accessed on the
Promote threadedness comment to NotThreadSafe w/ checking in the various public
entry points, or overkill?

[scherkus (not reviewing), 2012-07-12 21:02:17]

http://codereview.chromium.org/10694138/diff/4003/webkit/media/buffered_resou...
File webkit/media/buffered_resource_loader.cc (right):

http://codereview.chromium.org/10694138/diff/4003/webkit/media/buffered_resou...
webkit/media/buffered_resource_loader.cc:628: DCHECK(read_cb_.is_null() ||
last_offset_ > buffer_.forward_bytes())
On 2012/07/12 00:24:37, Ami Fischman wrote:
> Did you truth-tabling overtake your test-writing??

As discussed offline...

This is covered by BufferedResourceLoaderTest.ReadThenDeferStrategy. If we
somehow forget to fulfill a read when we're capable of doing so inside
didReceiveData() this DCHECK() fires (you can test  by commenting out that block
/ appending "&& false" so the condition never fires, etc...)

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

http://codereview.chromium.org/10694138/diff/4003/webkit/media/buffered_resou...
webkit/media/buffered_resource_loader.h:34: // BufferedResourceLoader is single
threaded and must be accessed on the
On 2012/07/12 00:24:37, Ami Fischman wrote:
> Promote threadedness comment to NotThreadSafe w/ checking in the various
public
> entry points, or overkill?

IMO overkill at this point in time

[commit-bot: I haz the power, 2012-07-13 23:39:07]

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

[commit-bot: I haz the power, 2012-07-14 01:13:00]

Change committed as 146700