count timeout as a completed attempt.

count timeout as a completed attempt.

BUG=659723
Committed: https://crrev.com/e7946c136e121bd0ba9c567a00bff412c742c8bd
Cr-Commit-Position: refs/heads/master@{#428835}

[Pete Williamson, 2016-10-28 01:04:49]

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

[Pete Williamson, 2016-10-28 01:04:49]

The unit tests were changed since we no don't call the OfflinerDoneTimeout,
which would set the variables being checked.  Also, we no longer call the
MarkAbortedAttempt() function, which could set them.

I think we don't need to go through long validation testing since we've upped
the immediate timeout to 8 minutes, and this change brings the actual code more
in line with our original intent in building the background offliner.  

If we discover that we need retries, we should do them properly, by adding retry
support back to the code.  We can put this into the code base, then check to see
over time if retry support seems to be needed.

[dougarnett, 2016-10-28 16:13:11]

https://codereview.chromium.org/2454123003/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2454123003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:215: // If we timed
out, let the offliner done callback handle it.
This will dramatically diminish the success rate for svelte devices
on 2G (arguably, our primary target) since they will see a 3 minute watchdog not
an 8 minute one (and we have data showing many pages 
that succeeded took 3 or 4 two minutes attempts before they succeeded).

We could make this conditional on IMMEDIATE_WINDOW because we can
tune its timeout to be a full page budget => complete attempt. But
if a full page budget is 8 minutes (or 5 minutes), then we can't say
that a 3 minute attempt (SCHEDULED_WINDOW) was a complete attempt.
We need to account for giving it a fair time window in some way. I'm
not sure just allowing one retry is the right approach but it is 
another mitigation option until we decide the right approach.

We have to be careful about this scheduled window behavior if the
immediate behavior will hide seeing its behavior most of the time
in our ad hoc use. We might need a test harness mode that turns
off immediate mode (can be via svelte device for now probably) to
be able to evaluate its tuning separate from the immediate mode. 
 
I suggest making conditional on processing state for now with TODO
to address timeouts and retries for scheduled window (or across 
modes).

[Pete Williamson, 2016-10-28 18:32:38]

Fixed per our discussion, bug to follow

https://codereview.chromium.org/2454123003/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2454123003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:215: // If we timed
out, let the offliner done callback handle it.
On 2016/10/28 16:13:11, dougarnett wrote:
> This will dramatically diminish the success rate for svelte devices
> on 2G (arguably, our primary target) since they will see a 3 minute watchdog
not
> an 8 minute one (and we have data showing many pages 
> that succeeded took 3 or 4 two minutes attempts before they succeeded).
> 
> We could make this conditional on IMMEDIATE_WINDOW because we can
> tune its timeout to be a full page budget => complete attempt. But
> if a full page budget is 8 minutes (or 5 minutes), then we can't say
> that a 3 minute attempt (SCHEDULED_WINDOW) was a complete attempt.
> We need to account for giving it a fair time window in some way. I'm
> not sure just allowing one retry is the right approach but it is 
> another mitigation option until we decide the right approach.
> 
> We have to be careful about this scheduled window behavior if the
> immediate behavior will hide seeing its behavior most of the time
> in our ad hoc use. We might need a test harness mode that turns
> off immediate mode (can be via svelte device for now probably) to
> be able to evaluate its tuning separate from the immediate mode. 
>  
> I suggest making conditional on processing state for now with TODO
> to address timeouts and retries for scheduled window (or across 
> modes).

Done.

[dougarnett, 2016-10-28 18:37:46]

lgtm

[Pete Williamson, 2016-10-31 20:41:07]

The CQ bit was checked by petewil@chromium.org

[Pete Williamson, 2016-10-31 20:41:08]

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/2454123003/#ps40001
(title: "merge")

[commit-bot: I haz the power, 2016-10-31 20:41:48]

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-10-31 22:13:57]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-31 22:15:28]

Description was changed from

==========
count timeout as a completed attempt.

BUG=659723
==========

to

==========
count timeout as a completed attempt.

BUG=659723

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

[commit-bot: I haz the power, 2016-10-31 22:15:29]

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