[Offline Pages] Adds UMA for result of attempts to immediately start background loading.

components/offline_pages/background/request_coordinator.cc

Index: components/offline_pages/background/request_coordinator.cc
diff --git a/components/offline_pages/background/request_coordinator.cc b/components/offline_pages/background/request_coordinator.cc
index 8aae54cfb739b5b3eadb46cd7abc254bd0ee4f1a..43205dce68fa5d7c15b278ad397498be2f2881f6 100644
--- a/components/offline_pages/background/request_coordinator.cc
+++ b/components/offline_pages/background/request_coordinator.cc
@@ -25,6 +25,24 @@ namespace offline_pages {
 namespace {
 const bool kUserRequest = true;
 
+enum class OfflinerImmediateStartStatus {
+  BUSY,

[fgorski, 2016/10/05 21:46:13]

Can you reorder these two?

Almost feels like it should be:
0 -  I Succeeded
1-5.. Here is why I didn't

WDYT?

[dougarnett, 2016/10/05 22:28:14]

Done.

+  STARTED,
+  NOT_ACCEPTED,
+  NO_CONNECTION,
+  NO_EFFECTIVE_CONNECTION,
+  NOT_STARTED_ON_SVELTE,
+  // NOTE: insert new values above this line and update histogram enum too.
+  STATUS_COUNT,
+};
+
+void RecordImmediateStart(OfflinerImmediateStartStatus immediate_start_status) {
+  UMA_HISTOGRAM_ENUMERATION(
+      "OfflinePages.Background.ImmediateStartStatus",
+      static_cast<int>(immediate_start_status),

[fgorski, 2016/10/05 21:46:13]

holte@: should be using int vs. int32_t here? I have another patch where I have
the same dilemma. 

[dougarnett, 2016/10/05 22:28:15]

Pete just had CL that used int so followed that example.

+      static_cast<int>(OfflinerImmediateStartStatus::STATUS_COUNT));
+}
+
 // Records the final request status UMA for an offlining request. This should
 // only be called once per Offliner::LoadAndSave request.
 void RecordOfflinerResultUMA(const ClientId& client_id,
@@ -372,10 +390,16 @@ bool RequestCoordinator::StartProcessing(
 
 void RequestCoordinator::StartProcessingIfConnected() {

[Pete Williamson, 2016/10/05 21:51:53]

Would it be better to have StartProcessingIfConnected just return the immediate
start status in a return value, and do the UMA from there?  If we could, it
would make the code easier to read.

It might also make sense to do the same for StartProcessing.

[dougarnett, 2016/10/05 22:28:14]

It is called in two places - so capture UMA in both or just for Adds not
Resumes? Or create a wrapping method? Improved later to get this in 55?

[Pete Williamson, 2016/10/05 22:45:03]

I'm happy if you want to improve this later in M55 instead of now, it's fine by
me to create a TODO.  I'm also open to any arguments if you think the code is
better as is.  I'd capture UMA for both, or creating a wrapping method is also
viable.

[dougarnett, 2016/10/05 23:20:37]

I tried wrapper - see if you like it better.

   // Make sure not already busy processing.
-  if (is_busy_) return;
+  if (is_busy_) {
+    RecordImmediateStart(OfflinerImmediateStartStatus::BUSY);
+    return;
+  }
 
   // Make sure we are not on svelte device to start immediately.
-  if (base::SysInfo::IsLowEndDevice()) return;
+  if (base::SysInfo::IsLowEndDevice()) {
+    RecordImmediateStart(OfflinerImmediateStartStatus::NOT_STARTED_ON_SVELTE);

[fgorski, 2016/10/05 21:46:13]

Just a thought: If you make this check above, you are effectively counting
svelte devices...

[dougarnett, 2016/10/05 22:28:14]

Happens anyway as far as unique devices goes on first Add (won't be busy).

+    return;
+  }
 
   // Make sure we have reasonable network quality (or at least a connection).
   if (network_quality_estimator_) {
@@ -383,10 +407,13 @@ void RequestCoordinator::StartProcessingIfConnected() {
     net::EffectiveConnectionType quality =
         network_quality_estimator_->GetEffectiveConnectionType();
     if (quality < net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_2G) {
+      RecordImmediateStart(
+          OfflinerImmediateStartStatus::NO_EFFECTIVE_CONNECTION);
       return;
     }
   } else if (GetConnectionType() ==
              net::NetworkChangeNotifier::ConnectionType::CONNECTION_NONE) {
+    RecordImmediateStart(OfflinerImmediateStartStatus::NO_CONNECTION);
     return;
   }
 
@@ -394,7 +421,11 @@ void RequestCoordinator::StartProcessingIfConnected() {
   // (i.e., assume no battery).
   // TODO(dougarnett): Obtain actual battery conditions (from Android/Java).
   DeviceConditions device_conditions(false, 0, GetConnectionType());
-  StartProcessing(device_conditions, base::Bind(&EmptySchedulerCallback));
+  if (StartProcessing(device_conditions, base::Bind(&EmptySchedulerCallback))) {

[fgorski, 2016/10/05 21:46:13]

nit: braces not needed

+    RecordImmediateStart(OfflinerImmediateStartStatus::STARTED);
+  } else {
+    RecordImmediateStart(OfflinerImmediateStartStatus::NOT_ACCEPTED);
+  }
 }
 
 void RequestCoordinator::TryNextRequest() {