Add retries for completed background loading attempts

Add retries for completed background loading attempts

We determined through manual testing that retries are needed for an
acceptable page display rate.  This adds back the retry limit, set to 3
for now, and has the unit test changes to adapt to the retry limit.

Since we now do retries, we set the policy for retries to prefer requests with fewer retries, and among requests with the same number
of retries, to prefer earlier requests.  This will try the requests
in round-robin order so that one troublesome website will not stop
the other requests.

We also removed the check temporarily allowing completed retries (under the guise of started retries)

BUG=660991
Committed: https://crrev.com/cdb23a159c0f4cbd8e18f9b15c878cc20a4ba48d
Cr-Commit-Position: refs/heads/master@{#429748}

[Pete Williamson, 2016-11-01 23:15:57]

Description was changed from

==========
We determined through manual testing that retries are needed for an
acceptable page display rate.  This adds back the retry limit, set to 3
for now, and has the unit test changes to adapt to the retry limit.

BUG=660991
==========

to

==========
We determined through manual testing that retries are needed for an
acceptable page display rate.  This adds back the retry limit, set to 3
for now, and has the unit test changes to adapt to the retry limit.

Since we now do retries, we set the policy for retries to prefer requests with
fewer retries, and among requests with the same number
of retries, to prefer earlier requests.  This will try the requests
in round-robin order so that one troublesome website will not stop 
the other requests.

We also removed the check temporarily allowing completed retries (under the
guise of started retries)

BUG=660991
==========

[Pete Williamson, 2016-11-01 23:16:12]

petewil@chromium.org changed reviewers:
+ dimich@chromium.org, dougarnett@chromium.org

[dougarnett, 2016-11-02 19:28:12]

lgtm with minor comment

https://codereview.chromium.org/2472593002/diff/1/components/offline_pages/ba...
File components/offline_pages/background/offliner_policy.h (right):

https://codereview.chromium.org/2472593002/diff/1/components/offline_pages/ba...
components/offline_pages/background/offliner_policy.h:14: // we often need about
4 retries with a 2 minute window, or 3 retries with a 3
retries => tries here?

[dewittj, 2016-11-02 19:49:51]

dewittj@chromium.org changed reviewers:
+ dewittj@chromium.org

[dewittj, 2016-11-02 19:49:52]

drive-by: please follow
https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list...

and make sure a summary is the first line of the CL description.  It looks like
the "subject" is your summary but that won't be reflected in Git.

[dougarnett, 2016-11-03 00:13:32]

Btw, as this is a simple looking change that looks good, I think it merits some
good ad-hoc testing before landing.

[Pete Williamson, 2016-11-03 18:28:12]

Description was changed from

==========
We determined through manual testing that retries are needed for an
acceptable page display rate.  This adds back the retry limit, set to 3
for now, and has the unit test changes to adapt to the retry limit.

Since we now do retries, we set the policy for retries to prefer requests with
fewer retries, and among requests with the same number
of retries, to prefer earlier requests.  This will try the requests
in round-robin order so that one troublesome website will not stop 
the other requests.

We also removed the check temporarily allowing completed retries (under the
guise of started retries)

BUG=660991
==========

to

==========
Add retries for completed background loading attempts

We determined through manual testing that retries are needed for an
acceptable page display rate.  This adds back the retry limit, set to 3
for now, and has the unit test changes to adapt to the retry limit.

Since we now do retries, we set the policy for retries to prefer requests with
fewer retries, and among requests with the same number
of retries, to prefer earlier requests.  This will try the requests
in round-robin order so that one troublesome website will not stop 
the other requests.

We also removed the check temporarily allowing completed retries (under the
guise of started retries)

BUG=660991
==========

[Pete Williamson, 2016-11-03 18:33:50]

On 2016/11/02 19:49:52, dewittj wrote:
> drive-by: please follow
>
https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list...
> 
> and make sure a summary is the first line of the CL description.  It looks
like
> the "subject" is your summary but that won't be reflected in Git.

Done.

[Pete Williamson, 2016-11-03 18:34:08]

Also, all the big pages I tested downloaded fine.

https://codereview.chromium.org/2472593002/diff/1/components/offline_pages/ba...
File components/offline_pages/background/offliner_policy.h (right):

https://codereview.chromium.org/2472593002/diff/1/components/offline_pages/ba...
components/offline_pages/background/offliner_policy.h:14: // we often need about
4 retries with a 2 minute window, or 3 retries with a 3
On 2016/11/02 19:28:11, dougarnett wrote:
> retries => tries here? 

Done.

[Pete Williamson, 2016-11-03 19:15:36]

The CQ bit was checked by petewil@chromium.org

[Pete Williamson, 2016-11-03 19:15:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
dougarnett@chromium.org
Link to the patchset: https://codereview.chromium.org/2472593002/#ps20001
(title: "CR feedback")

[commit-bot: I haz the power, 2016-11-03 19:16:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-03 19:52:58]

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

[commit-bot: I haz the power, 2016-11-03 19:52:59]

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

[Dmitry Titov, 2016-11-03 22:12:06]

Please wait with landing before getting all LGTMs.

Are the changes introduced actually tested? For example, prefer untried requests
and other policy changes should cause visibel chanegs in bhavior of RC, right?

[Pete Williamson, 2016-11-03 22:50:29]

On 2016/11/03 22:12:06, Dmitry Titov wrote:
> Please wait with landing before getting all LGTMs.
> 
> Are the changes introduced actually tested? For example, prefer untried
requests
> and other policy changes should cause visibel chanegs in bhavior of RC, right?

Ah, my bad, I forgot I'd asked more than one reviewer.  I will wait before
submitting.

Yes, this has been tested two ways, once with Romax's test harness, which got
similar results to what DougArnett saw over 2G Poor, and once manually, which
successfully loaded 4 large pages.

The test harness used a 3 minute limit, so we know that we are no worse than
existing code, but it doesn't show that it is better.
The tests of a large page use the new limits, and we see all 4 large pages load
successfully.

[Pete Williamson, 2016-11-03 22:55:08]

The CQ bit was checked by petewil@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 22:55:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dmitry Titov, 2016-11-03 22:57:54]

My question was about unit tests, resolved offline, thanks!
lgtm

[commit-bot: I haz the power, 2016-11-04 00:40:29]

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

[commit-bot: I haz the power, 2016-11-04 00:40:30]

Dry run: This issue passed the CQ dry run.

[Pete Williamson, 2016-11-04 00:41:21]

The CQ bit was checked by petewil@chromium.org

[commit-bot: I haz the power, 2016-11-04 00:42:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-04 01:00:22]

Description was changed from

==========
Add retries for completed background loading attempts

We determined through manual testing that retries are needed for an
acceptable page display rate.  This adds back the retry limit, set to 3
for now, and has the unit test changes to adapt to the retry limit.

Since we now do retries, we set the policy for retries to prefer requests with
fewer retries, and among requests with the same number
of retries, to prefer earlier requests.  This will try the requests
in round-robin order so that one troublesome website will not stop 
the other requests.

We also removed the check temporarily allowing completed retries (under the
guise of started retries)

BUG=660991
==========

to

==========
Add retries for completed background loading attempts

We determined through manual testing that retries are needed for an
acceptable page display rate.  This adds back the retry limit, set to 3
for now, and has the unit test changes to adapt to the retry limit.

Since we now do retries, we set the policy for retries to prefer requests with
fewer retries, and among requests with the same number
of retries, to prefer earlier requests.  This will try the requests
in round-robin order so that one troublesome website will not stop 
the other requests.

We also removed the check temporarily allowing completed retries (under the
guise of started retries)

BUG=660991
==========

[commit-bot: I haz the power, 2016-11-04 01:00:23]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-04 01:02:13]

Description was changed from

==========
Add retries for completed background loading attempts

We determined through manual testing that retries are needed for an
acceptable page display rate.  This adds back the retry limit, set to 3
for now, and has the unit test changes to adapt to the retry limit.

Since we now do retries, we set the policy for retries to prefer requests with
fewer retries, and among requests with the same number
of retries, to prefer earlier requests.  This will try the requests
in round-robin order so that one troublesome website will not stop 
the other requests.

We also removed the check temporarily allowing completed retries (under the
guise of started retries)

BUG=660991
==========

to

==========
Add retries for completed background loading attempts

We determined through manual testing that retries are needed for an
acceptable page display rate.  This adds back the retry limit, set to 3
for now, and has the unit test changes to adapt to the retry limit.

Since we now do retries, we set the policy for retries to prefer requests with
fewer retries, and among requests with the same number
of retries, to prefer earlier requests.  This will try the requests
in round-robin order so that one troublesome website will not stop
the other requests.

We also removed the check temporarily allowing completed retries (under the
guise of started retries)

BUG=660991

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

[commit-bot: I haz the power, 2016-11-04 01:02:14]

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