make multibuffer work with layout tests

make multibuffer work with layout tests

This Cl does four things:

1) Makes sure that the loading state callback is handled in a backwards-compatible way.
   (BufferedDataSource)
2) Fixes a problem where we would try to read past the end of the file if the file was
   already completely cached. (MultiBufferReader)
3) Fixes one layout test that relies on a file getting read from the server slowly.
4) Avoids going back to the beginning of the block when server chooses to return less
   data than we asked for. (ResourceMultiBufferDataProvider)

+ adds a few more DVLOG statements which I found very useful when debugging.

Committed: https://crrev.com/d82bed5a05b67e16a284c9f56d579c2ac55cff70
Cr-Commit-Position: refs/heads/master@{#365371}

[hubbe, 2015-12-08 20:33:48]

hubbe@chromium.org changed reviewers:
+ liberato@chromium.org

[liberato (no reviews please), 2015-12-08 21:59:33]

if i'm completely misunderstanding, i'll be in the office tomorrow if it's
easier to chat in person.

https://codereview.chromium.org/1509663003/diff/1/media/blink/multibuffer_dat...
File media/blink/multibuffer_data_source.cc (right):

https://codereview.chromium.org/1509663003/diff/1/media/blink/multibuffer_dat...
media/blink/multibuffer_data_source.cc:513: loading_ = is_loading ||
force_loading;
if the reader reports !loading, how does loading_ get cleared here if we force
it to be set here?

in other words, what happens if we just call |downloading_cb_|(true) without
setting |loading_|?

https://codereview.chromium.org/1509663003/diff/1/media/blink/multibuffer_dat...
media/blink/multibuffer_data_source.cc:515: if (!loading_ && cancel_on_defer_) {
this can happen on force_loading that wouldn't have happened before, since
loading_ defaults to false, and the if at 512 wouldn't get us here.  is that
intentional?

https://codereview.chromium.org/1509663003/diff/1/media/blink/multibuffer_dat...
File media/blink/multibuffer_data_source.h (right):

https://codereview.chromium.org/1509663003/diff/1/media/blink/multibuffer_dat...
media/blink/multibuffer_data_source.h:142: void UpdateLoadingState(bool
force_loading);
please add docs.

[hubbe, 2015-12-08 22:25:59]

If I need to explain things in person, how is the next reader going to
understand it?

https://codereview.chromium.org/1509663003/diff/1/media/blink/multibuffer_dat...
File media/blink/multibuffer_data_source.cc (right):

https://codereview.chromium.org/1509663003/diff/1/media/blink/multibuffer_dat...
media/blink/multibuffer_data_source.cc:513: loading_ = is_loading ||
force_loading;
On 2015/12/08 21:59:33, liberato wrote:
> if the reader reports !loading, how does loading_ get cleared here if we force
> it to be set here?
> 
> in other words, what happens if we just call |downloading_cb_|(true) without
> setting |loading_|?

Updated comment for |loading_|.
loading_ will be set to the true value next time UpdateLoadingState() is called.

https://codereview.chromium.org/1509663003/diff/1/media/blink/multibuffer_dat...
media/blink/multibuffer_data_source.cc:515: if (!loading_ && cancel_on_defer_) {
On 2015/12/08 21:59:33, liberato wrote:
> this can happen on force_loading that wouldn't have happened before, since
> loading_ defaults to false, and the if at 512 wouldn't get us here.  is that
> intentional?

if force_loading is true, then loading_ will be true and this cannot happen.

https://codereview.chromium.org/1509663003/diff/1/media/blink/multibuffer_dat...
File media/blink/multibuffer_data_source.h (right):

https://codereview.chromium.org/1509663003/diff/1/media/blink/multibuffer_dat...
media/blink/multibuffer_data_source.h:142: void UpdateLoadingState(bool
force_loading);
On 2015/12/08 21:59:33, liberato wrote:
> please add docs.

Done.

[liberato (no reviews please), 2015-12-10 20:39:19]

lgtm

-fl

[hubbe, 2015-12-14 19:24:26]

hubbe@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[DaleCurtis, 2015-12-14 19:32:15]

lgtm

[hubbe, 2015-12-15 19:44:27]

The CQ bit was checked by hubbe@chromium.org

[hubbe, 2015-12-15 19:44:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org, liberato@chromium.org
Link to the patchset: https://codereview.chromium.org/1509663003/#ps40001
(title: "merged & formatted")

[commit-bot: I haz the power, 2015-12-15 19:46:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1509663003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1509663003/40001

[commit-bot: I haz the power, 2015-12-15 23:07:23]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-12-15 23:08:27]

Description was changed from

==========
make multibuffer work with layout tests

This Cl does four things:

1) Makes sure that the loading state callback is handled in a
backwards-compatible way.
   (BufferedDataSource)
2) Fixes a problem where we would try to read past the end of the file if the
file was
   already completely cached. (MultiBufferReader)
3) Fixes one layout test that relies on a file getting read from the server
slowly.
4) Avoids going back to the beginning of the block when server chooses to return
less
   data than we asked for. (ResourceMultiBufferDataProvider)

+ adds a few more DVLOG statements which I found very useful when debugging.
==========

to

==========
make multibuffer work with layout tests

This Cl does four things:

1) Makes sure that the loading state callback is handled in a
backwards-compatible way.
   (BufferedDataSource)
2) Fixes a problem where we would try to read past the end of the file if the
file was
   already completely cached. (MultiBufferReader)
3) Fixes one layout test that relies on a file getting read from the server
slowly.
4) Avoids going back to the beginning of the block when server chooses to return
less
   data than we asked for. (ResourceMultiBufferDataProvider)

+ adds a few more DVLOG statements which I found very useful when debugging.

Committed: https://crrev.com/d82bed5a05b67e16a284c9f56d579c2ac55cff70
Cr-Commit-Position: refs/heads/master@{#365371}
==========

[commit-bot: I haz the power, 2015-12-15 23:08:27]

Patchset 3 (id:??) landed as
https://crrev.com/d82bed5a05b67e16a284c9f56d579c2ac55cff70
Cr-Commit-Position: refs/heads/master@{#365371}