Improve retry support for media network loading.

Improve retry support for media network loading.

Retry 30 times in 30 seconds instead of 3 times in 750ms.
The retries are spaced so that the first retry is 250ms, second is 300ms, then 350ms, etc.

BUG=592703
Committed: https://crrev.com/018c484fd85b3a64ef1037191ca513b389fa21ff
Cr-Commit-Position: refs/heads/master@{#380723}

[hubbe, 2016-03-10 00:44:10]

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

[DaleCurtis, 2016-03-10 01:23:32]

https://codereview.chromium.org/1777153002/diff/1/media/blink/buffered_data_s...
File media/blink/buffered_data_source_unittest.cc (right):

https://codereview.chromium.org/1777153002/diff/1/media/blink/buffered_data_s...
media/blink/buffered_data_source_unittest.cc:636: for (int i = 0; i < 30; i++) {
Expose kLoaderRetries on the header file you can use here instead? Or better yet
since these test likely now take 30 seconds, add a SetRetryCountForTesting()
method which limits this to say 3 or 4 retries and ensures the delay grows
between each retry.

https://codereview.chromium.org/1777153002/diff/1/media/blink/buffered_data_s...
media/blink/buffered_data_source_unittest.cc:656: for (int i = 0; i < 30; i++) {
Ditto.

https://codereview.chromium.org/1777153002/diff/1/media/blink/resource_multib...
File media/blink/resource_multibuffer_data_provider.cc (right):

https://codereview.chromium.org/1777153002/diff/1/media/blink/resource_multib...
media/blink/resource_multibuffer_data_provider.cc:421: kLoaderFailedRetryDelayMs
+ kAdditionalDelayPerRetryMs * retries_));
IIRC we reuse the BufferedDataSource unittest for this class too right?

[hubbe, 2016-03-11 03:18:53]

https://codereview.chromium.org/1777153002/diff/1/media/blink/buffered_data_s...
File media/blink/buffered_data_source_unittest.cc (right):

https://codereview.chromium.org/1777153002/diff/1/media/blink/buffered_data_s...
media/blink/buffered_data_source_unittest.cc:636: for (int i = 0; i < 30; i++) {
On 2016/03/10 01:23:32, DaleCurtis wrote:
> Expose kLoaderRetries on the header file you can use here instead? Or better
yet
> since these test likely now take 30 seconds, add a SetRetryCountForTesting()
> method which limits this to say 3 or 4 retries and ensures the delay grows
> between each retry.

Exposed kLoaderRetries.
These tests don't wait for the CB to execute, so they finish very quickly.

https://codereview.chromium.org/1777153002/diff/1/media/blink/buffered_data_s...
media/blink/buffered_data_source_unittest.cc:656: for (int i = 0; i < 30; i++) {
On 2016/03/10 01:23:32, DaleCurtis wrote:
> Ditto.

Done.

https://codereview.chromium.org/1777153002/diff/1/media/blink/resource_multib...
File media/blink/resource_multibuffer_data_provider.cc (right):

https://codereview.chromium.org/1777153002/diff/1/media/blink/resource_multib...
media/blink/resource_multibuffer_data_provider.cc:421: kLoaderFailedRetryDelayMs
+ kAdditionalDelayPerRetryMs * retries_));
On 2016/03/10 01:23:32, DaleCurtis wrote:
> IIRC we reuse the BufferedDataSource unittest for this class too right?

No, we do not.
Also, the tests we have weren't really testing what they were supposed to test,
but that is fixed now.

[DaleCurtis, 2016-03-11 16:36:32]

lgtm

https://codereview.chromium.org/1777153002/diff/20001/media/blink/buffered_da...
File media/blink/buffered_data_source.h (right):

https://codereview.chromium.org/1777153002/diff/20001/media/blink/buffered_da...
media/blink/buffered_data_source.h:125: // Number of times we retry before
erroring out.
Static const on an interface is odd. Enum is preferred these days.  On mobile,
but can send discussion later. You can add the other retry const here too then
and avoid duplicate defs.

https://codereview.chromium.org/1777153002/diff/20001/media/blink/buffered_da...
File media/blink/buffered_data_source_unittest.cc (right):

https://codereview.chromium.org/1777153002/diff/20001/media/blink/buffered_da...
media/blink/buffered_data_source_unittest.cc:636: for (int i = 0; i < 30; i++) {
kLoaderRetries?

https://codereview.chromium.org/1777153002/diff/20001/media/blink/buffered_da...
media/blink/buffered_data_source_unittest.cc:656: for (int i = 0; i < 30; i++) {
Ditto

[hubbe, 2016-03-11 18:15:56]

https://codereview.chromium.org/1777153002/diff/20001/media/blink/buffered_da...
File media/blink/buffered_data_source.h (right):

https://codereview.chromium.org/1777153002/diff/20001/media/blink/buffered_da...
media/blink/buffered_data_source.h:125: // Number of times we retry before
erroring out.
On 2016/03/11 16:36:32, DaleCurtis wrote:
> Static const on an interface is odd. Enum is preferred these days.  On mobile,
> but can send discussion later. You can add the other retry const here too then
> and avoid duplicate defs.

This is not an interface, but regardless, enums preferred.
(I remember reading the discussion, after you pointed it out..)

Since intend to delete this class later, I don't want to de-dupe the
two constants.

https://codereview.chromium.org/1777153002/diff/20001/media/blink/buffered_da...
File media/blink/buffered_data_source_unittest.cc (right):

https://codereview.chromium.org/1777153002/diff/20001/media/blink/buffered_da...
media/blink/buffered_data_source_unittest.cc:636: for (int i = 0; i < 30; i++) {
On 2016/03/11 16:36:32, DaleCurtis wrote:
> kLoaderRetries?

Done. (oops)

https://codereview.chromium.org/1777153002/diff/20001/media/blink/buffered_da...
media/blink/buffered_data_source_unittest.cc:656: for (int i = 0; i < 30; i++) {
On 2016/03/11 16:36:32, DaleCurtis wrote:
> Ditto

Done.

[DaleCurtis, 2016-03-11 18:47:59]

lgtm, thanks!

[hubbe, 2016-03-11 18:52:16]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2016-03-11 18:52:56]

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

[commit-bot: I haz the power, 2016-03-11 19:38:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-11 19:38:06]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hubbe, 2016-03-11 20:14:34]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2016-03-11 20:15:16]

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

[commit-bot: I haz the power, 2016-03-11 21:03:11]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-11 21:04:20]

Description was changed from

==========
Improve retry support for media network loading.

Retry 30 times in 30 seconds instead of 3 times in 750ms.
The retries are spaced so that the first retry is 250ms, second is 300ms, then
350ms, etc.

BUG=592703
==========

to

==========
Improve retry support for media network loading.

Retry 30 times in 30 seconds instead of 3 times in 750ms.
The retries are spaced so that the first retry is 250ms, second is 300ms, then
350ms, etc.

BUG=592703

Committed: https://crrev.com/018c484fd85b3a64ef1037191ca513b389fa21ff
Cr-Commit-Position: refs/heads/master@{#380723}
==========

[commit-bot: I haz the power, 2016-03-11 21:04:21]

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