[Offline Pages] Adds classification of some prerender FinalStatus codes as canceled operations or a…

[Offline Pages] Adds classification of some prerender FinalStatus codes as canceled operations or as hard failures.
Initial set of codes are based on what I have experienced in manual evaluations
and what I am seeing in FinalStatus UMA for our Origin so far. We will want to
make decisions about more status codes in the future. Identifying the hard
failures as NO_RETRY cases is preparation for turning on a single retry for
other failures but does have an immediate benefit of trying to pick the next
request to process for hard failures rather than bail out of service window.
Another immediate benefit/effect is that I am seeing many FINAL_STATUS_CANCELLED
occurences in UMA, these would not be retried in the current code base but
they will be with this change applied.

BUG=649386
Committed: https://crrev.com/af0bd147487db4d88f06bb7a7f2e20e2286f3df1
Cr-Commit-Position: refs/heads/master@{#423256}

[dougarnett, 2016-09-22 17:18:04]

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

[commit-bot: I haz the power, 2016-09-22 17:18:32]

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

[dougarnett, 2016-09-22 17:41:55]

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

[dougarnett, 2016-09-22 17:41:56]

Cathy, would be interested in your review here but more specifically want to
expose to you the topic of these different prerendering/loader failure cases and
potential classification.

[chili, 2016-09-23 00:44:18]

Thanks for keeping me in loop! :)

https://codereview.chromium.org/2361883002/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/prerendering_loader.cc (right):

https://codereview.chromium.org/2361883002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.cc:22: bool
IsCanceledOperation(prerender::FinalStatus final_status) {
nit: while both spellings are valid, I think request coordinator used
'Cancelled' for most things

On the other hand, offliner final status uses one L, so feel free to ignore

https://codereview.chromium.org/2361883002/diff/20001/components/offline_page...
File components/offline_pages/background/request_coordinator_unittest.cc
(right):

https://codereview.chromium.org/2361883002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator_unittest.cc:555:
PumpLoop();
Just for my information... what does this do?

[dougarnett, 2016-09-23 18:20:01]

https://codereview.chromium.org/2361883002/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/prerendering_loader.cc (right):

https://codereview.chromium.org/2361883002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.cc:22: bool
IsCanceledOperation(prerender::FinalStatus final_status) {
On 2016/09/23 00:44:17, chili wrote:
> nit: while both spellings are valid, I think request coordinator used
> 'Cancelled' for most things
> 
> On the other hand, offliner final status uses one L, so feel free to ignore

Yeah, the Offliner statuses all use single L so that is what drove this method
name (ie, look at where it is used below). Also our Archiver uses single L. I
think we can ignore that Prerender FinalStatus uses double L but I do see our
SavePageResult uses double L and maybe we should fix that one to be more
consistent with our other spellings (maybe something to raise with Filip/Dmitry
though).

https://codereview.chromium.org/2361883002/diff/20001/components/offline_page...
File components/offline_pages/background/request_coordinator_unittest.cc
(right):

https://codereview.chromium.org/2361883002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator_unittest.cc:555:
PumpLoop();
On 2016/09/23 00:44:18, chili wrote:
> Just for my information... what does this do?

Drives posted tasks on the task runner - eg, to get callbacks called.

[Dmitry Titov, 2016-09-24 00:53:30]

For some reason, I had difficulty reading this through... Perhaps too overloaded
terminology? (canceled/failed mean completely different things etc).

https://codereview.chromium.org/2361883002/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/prerendering_loader.cc (right):

https://codereview.chromium.org/2361883002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.cc:20: // Classifies
whether final status represent a canceled operation vs. a full
Could you clarify the comment? Both options sound the same (canceled operation
vs failed attempt). Quite cryptic.

https://codereview.chromium.org/2361883002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.cc:191: // If we have
already loaded, then the prerender stack is now canceling
Same, this comment is not clear. loaded means cancel? Also, this 'baseline with
refinement' - can it all become just a single switch or a series of if
statements?

[Pete Williamson, 2016-09-27 19:25:08]

https://codereview.chromium.org/2361883002/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/prerendering_loader.cc (right):

https://codereview.chromium.org/2361883002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.cc:20: // Classifies
whether final status represent a canceled operation vs. a full
On 2016/09/24 00:53:30, Dmitry Titov wrote:
> Could you clarify the comment? Both options sound the same (canceled operation
> vs failed attempt). Quite cryptic.

This may be better to return an enum CANCELED or FAILED instead of true or
false.

https://codereview.chromium.org/2361883002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.cc:35: // should not be
retried.
Maybe combine this with the function above, and have RETRYABLE as a potentially
returned enum value.

https://codereview.chromium.org/2361883002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.cc:213: if
(final_status == prerender::FINAL_STATUS_UNSUPPORTED_SCHEME) {
Why stop emitting UMA for cancels?

https://codereview.chromium.org/2361883002/diff/20001/components/offline_page...
File components/offline_pages/background/request_coordinator_unittest.cc
(right):

https://codereview.chromium.org/2361883002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator_unittest.cc:555:
PumpLoop();
On 2016/09/23 18:20:01, dougarnett wrote:
> On 2016/09/23 00:44:18, chili wrote:
> > Just for my information... what does this do?
> 
> Drives posted tasks on the task runner - eg, to get callbacks called.

To clarify a bit more, many tasks that the request coordinator does use a task
queue.  PumpLoop() will take tasks from the task queue, and run until the task
queue is empty (so if a request in a task queue puts something else in the task
queue before it finishes, PumpLoop will run that too before returning.)
In this particular case, this is making sure that the RequestQueue has a chance
to complete the Add operation, and call the callback before the test proceeds to
the next step.

https://codereview.chromium.org/2361883002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator_unittest.cc:577:
EnableOfflinerCallback(true);
I don't think we want this line.  As I recall, it enables the code to call the
OfflinerDone callback itself after you start processing.  What we want is to
turn off the normal callback since we are calling it below.  Since we don't call
StartProcessing, it never comes into play here, and you should be able to remove
it safely.

https://codereview.chromium.org/2361883002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator_unittest.cc:586: //
Verify neither request is removed from the queue; wait for callbacks.
Comment looks old and may need updating (since there should now only be one
request in the queue).

[dougarnett, 2016-09-29 01:09:46]

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

[commit-bot: I haz the power, 2016-09-29 01:10:21]

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

[dougarnett, 2016-09-29 01:11:24]

PTAL - reworked into a single switch in new ClassifyFinalStatus()

https://codereview.chromium.org/2361883002/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/prerendering_loader.cc (right):

https://codereview.chromium.org/2361883002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.cc:20: // Classifies
whether final status represent a canceled operation vs. a full
On 2016/09/27 19:25:07, Pete Williamson wrote:
> On 2016/09/24 00:53:30, Dmitry Titov wrote:
> > Could you clarify the comment? Both options sound the same (canceled
operation
> > vs failed attempt). Quite cryptic.
> 
> This may be better to return an enum CANCELED or FAILED instead of true or
> false.

Done.

https://codereview.chromium.org/2361883002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.cc:35: // should not be
retried.
On 2016/09/27 19:25:07, Pete Williamson wrote:
> Maybe combine this with the function above, and have RETRYABLE as a
potentially
> returned enum value.

Done.

https://codereview.chromium.org/2361883002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.cc:191: // If we have
already loaded, then the prerender stack is now canceling
On 2016/09/24 00:53:30, Dmitry Titov wrote:
> Same, this comment is not clear. loaded means cancel? Also, this 'baseline
with
> refinement' - can it all become just a single switch or a series of if
> statements?

Done.

https://codereview.chromium.org/2361883002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.cc:213: if
(final_status == prerender::FINAL_STATUS_UNSUPPORTED_SCHEME) {
On 2016/09/27 19:25:07, Pete Williamson wrote:
> Why stop emitting UMA for cancels?

There was no UMA for cancels here - just setting the request_status which has
moved.

https://codereview.chromium.org/2361883002/diff/20001/components/offline_page...
File components/offline_pages/background/request_coordinator_unittest.cc
(right):

https://codereview.chromium.org/2361883002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator_unittest.cc:577:
EnableOfflinerCallback(true);
On 2016/09/27 19:25:08, Pete Williamson wrote:
> I don't think we want this line.  As I recall, it enables the code to call the
> OfflinerDone callback itself after you start processing.  What we want is to
> turn off the normal callback since we are calling it below.  Since we don't
call
> StartProcessing, it never comes into play here, and you should be able to
remove
> it safely.

Good catch - cleaning up the other identical cases.

https://codereview.chromium.org/2361883002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator_unittest.cc:586: //
Verify neither request is removed from the queue; wait for callbacks.
On 2016/09/27 19:25:07, Pete Williamson wrote:
> Comment looks old and may need updating (since there should now only be one
> request in the queue).

Done.

[commit-bot: I haz the power, 2016-09-29 02:05:34]

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

[commit-bot: I haz the power, 2016-09-29 02:05:35]

Dry run: This issue passed the CQ dry run.

[Pete Williamson, 2016-09-29 18:03:46]

lgtm

[dougarnett, 2016-09-29 18:55:37]

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

[dougarnett, 2016-09-29 18:55:38]

Steven, can you review histograms.xml?

https://codereview.chromium.org/2361883002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2361883002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:90773: +  <int value="9"
label="Prerendering failed (non-retryable)"/>
btw: there is another CL with 9 pending so one of these will merge to 10

[Steven Holte, 2016-09-29 20:13:27]

histograms lgtm, w/ either 9 or 10

[dougarnett, 2016-09-29 21:21:31]

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

[commit-bot: I haz the power, 2016-09-29 21:21:50]

Dry run: 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-09-29 22:26:04]

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

[commit-bot: I haz the power, 2016-09-29 22:26:05]

Dry run: This issue passed the CQ dry run.

[dougarnett, 2016-09-29 22:27:56]

fyi, merged now so this CL's enum item update to 10

[dougarnett, 2016-10-03 16:43:19]

On 2016/09/29 22:27:56, dougarnett wrote:
> fyi, merged now so this CL's enum item update to 10

Ping Dmitry - can you take another look?

[Dmitry Titov, 2016-10-05 17:44:56]

lgtm

[dougarnett, 2016-10-05 17:55:14]

The CQ bit was checked by dougarnett@chromium.org

[dougarnett, 2016-10-05 17:55:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
petewil@chromium.org, holte@chromium.org
Link to the patchset: https://codereview.chromium.org/2361883002/#ps60001
(title: "Merge")

[commit-bot: I haz the power, 2016-10-05 17:55:54]

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-05 19:46:32]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-05 19:48:16]

Description was changed from

==========
[Offline Pages] Adds classification of some prerender FinalStatus codes as
canceled operations or as hard failures.
Initial set of codes are based on what I have experienced in manual evaluations
and what I am seeing in FinalStatus UMA for our Origin so far. We will want to
make decisions about more status codes in the future. Identifying the hard
failures as NO_RETRY cases is preparation for turning on a single retry for
other failures but does have an immediate benefit of trying to pick the next
request to process for hard failures rather than bail out of service window.
Another immediate benefit/effect is that I am seeing many FINAL_STATUS_CANCELLED
occurences in UMA, these would not be retried in the current code base but
they will be with this change applied.

BUG=649386
==========

to

==========
[Offline Pages] Adds classification of some prerender FinalStatus codes as
canceled operations or as hard failures.
Initial set of codes are based on what I have experienced in manual evaluations
and what I am seeing in FinalStatus UMA for our Origin so far. We will want to
make decisions about more status codes in the future. Identifying the hard
failures as NO_RETRY cases is preparation for turning on a single retry for
other failures but does have an immediate benefit of trying to pick the next
request to process for hard failures rather than bail out of service window.
Another immediate benefit/effect is that I am seeing many FINAL_STATUS_CANCELLED
occurences in UMA, these would not be retried in the current code base but
they will be with this change applied.

BUG=649386

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

[commit-bot: I haz the power, 2016-10-05 19:48:18]

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