Adds UMA for PrerenderingOffliner request processing result status.

components/offline_pages/background/offliner.h

Index: components/offline_pages/background/offliner.h
diff --git a/components/offline_pages/background/offliner.h b/components/offline_pages/background/offliner.h
index 745fc6181b678670a377ce8ecaa41c2b0e22c38d..9e21ff6e33533aa6748292fcc20fb415d4c90369 100644
--- a/components/offline_pages/background/offliner.h
+++ b/components/offline_pages/background/offliner.h
@@ -16,14 +16,18 @@ class SavePageRequest;
 class Offliner {
  public:
   // Status of processing an offline page request.
-  enum class RequestStatus {
-    UNKNOWN,      // No status determined/reported yet.
-    LOADED,       // Page loaded but not (yet) saved.
-    SAVED,        // Offline page snapshot saved.
-    CANCELED,     // Request was canceled.
-    FAILED,       // Failed to load page.
-    FAILED_SAVE,  // Failed to save loaded page.
-    // TODO(dougarnett): Define a retry-able failure status.
+  // NOTE: These values correspond to the OfflinePagesBackgroundOfflinerStatus
+  // histogram enum.
+  enum RequestStatus {
+    UNKNOWN = 0,           // No status determined/reported yet.

[fgorski, 2016/06/29 18:47:03]

Value assignments are not necessary here.

[dougarnett, 2016/06/29 19:42:52]

But the histogram enum values need to match, right? So seemed a bit better to be
explicit so they are easier to check by inspection.

[jianli, 2016/06/29 20:47:34]

IMO, assigning value seems to often imply that it is ok to assign any random
value for enum constant.

[dougarnett, 2016/06/30 18:09:15]

I think that would be fairly easy to catch in code review though. I think it is
harder to catch the case when no-longer-used code is deleted from this enum and
thus changes the value of codes that follow it.

[Please let me know if I'm going against some chrome convention that was settled
such debate previously.]

[fgorski, 2016/06/30 18:50:54]

OK. I think dimich@ got that suggestion from a UMA reviewer recently so this can
stay. Can you please bring that up with UMA reviewer for confirmation?

+    LOADED = 1,            // Page loaded but not (yet) saved.
+    SAVED = 2,             // Offline page snapshot saved.

[fgorski, 2016/06/29 18:47:02]

Is it possible that a single request will end up recording multiple states?

e.g.:
loaded -> saved
loaded -> request_canceled ?
loaded -> save_failed


If so perhaps it makes sense to have a strategy for such cases and a good
explanation

[dougarnett, 2016/06/29 19:42:52]

No, just one final status recorded per request is what I aiming for. In the
histogram enum defn I am marking UNKNOWN and LOADED as not expected to be
recorded.

[fgorski, 2016/06/30 18:50:54]

Please document that in the code. I will have the same problem reading this code
a month from now.

[dougarnett, 2016/06/30 19:24:06]

Added comment to RecordStatusUMA method as it seemed the most applicable place
to note this.

+    REQUEST_CANCELED = 3,  // Request was canceled by requester (from above).
+    LOAD_CANCELED = 4,     // Load was canceled vs. failed (from below).

[fgorski, 2016/06/29 18:47:02]

Is that a cancel for reasons different than user canceling request?

[dougarnett, 2016/06/29 19:42:52]

This "from below" is a cancel from the prerenderer. There are a few cases where
the PrerenderManager will cancel a request (due to other requests or navigations
or low memory) that are not necessarily reflection of whether the page's
rendering would have succeeded.

The previous "from above" one, REQUEST_CANCELED, is a new status that is for
when the Coordinator cancels the request (the current use case for this is when
time limit expires). I was debating whether to record there results here or in
the Coordinator - if I did it in the Coordinator, I could characterize more
accurately as a TIMEOUT. But I ended up thinking maybe good, clean to have these
in the Offliner impl.

[fgorski, 2016/06/30 18:50:54]

Please document that in the code. I will have the same problem reading this code
a month from now.

[Dmitry Titov, 2016/06/30 18:54:04]

Maybe change name to say CANCELED_BY_PRERENDERER and
CANCELED_BY_REQUEST_COORDINATOR to try to make it clearer?

[dougarnett, 2016/06/30 19:24:06]

Yeah, I struggle with that a bit here as it is at odds with this being a pure
interface. Should we pile in specific codes here from its implementations? Or
should I define separate PrerenderingOffliner enum instead for the purpose of
the UMA?

+    LOAD_FAILED = 5,       // Failed to load page.
+    SAVE_FAILED = 6,       // Failed to save loaded page.
+    // NOTE: insert new values above this line and update histogram enum too.
+    STATUS_COUNT
   };
 
   // Reports the completion status of a request.