Split the data use into foreground, background and unknown

components/data_use_measurement/content/data_use_measurement.cc

Index: components/data_use_measurement/content/data_use_measurement.cc
diff --git a/components/data_use_measurement/content/data_use_measurement.cc b/components/data_use_measurement/content/data_use_measurement.cc
index a1f257f85e671faa3c25720c32d1cc9ef5663292..1cb51a52583a3b61c4cbe335e772cf50a1f85f27 100644
--- a/components/data_use_measurement/content/data_use_measurement.cc
+++ b/components/data_use_measurement/content/data_use_measurement.cc
@@ -86,6 +86,7 @@ void DataUseMeasurement::OnBeforeRedirect(const net::URLRequest& request,
 void DataUseMeasurement::OnNetworkBytesReceived(const net::URLRequest& request,
                                                 int64_t bytes_received) {
   UMA_HISTOGRAM_COUNTS("DataUse.BytesReceived.Delegate", bytes_received);
+  ReportDataUseUMAOnNetworkAccess(request, DOWNSTREAM, bytes_received);
 #if defined(OS_ANDROID)
   bytes_transferred_since_last_traffic_stats_query_ += bytes_received;
 #endif
@@ -94,6 +95,7 @@ void DataUseMeasurement::OnNetworkBytesReceived(const net::URLRequest& request,
 void DataUseMeasurement::OnNetworkBytesSent(const net::URLRequest& request,
                                             int64_t bytes_sent) {
   UMA_HISTOGRAM_COUNTS("DataUse.BytesSent.Delegate", bytes_sent);
+  ReportDataUseUMAOnNetworkAccess(request, UPSTREAM, bytes_sent);
 #if defined(OS_ANDROID)
   bytes_transferred_since_last_traffic_stats_query_ += bytes_sent;
 #endif
@@ -109,17 +111,11 @@ void DataUseMeasurement::OnCompleted(const net::URLRequest& request,
 #endif
 }
 
-void DataUseMeasurement::ReportDataUseUMA(
-    const net::URLRequest& request) const {
-  // Counts rely on URLRequest::GetTotalReceivedBytes() and
-  // URLRequest::GetTotalSentBytes(), which does not include the send path,
-  // network layer overhead, TLS overhead, and DNS.
-  // TODO(amohammadkhan): Make these measured bytes more in line with number of
-  // bytes in lower levels.
-  int64_t total_upload_bytes = request.GetTotalSentBytes();
-  int64_t total_received_bytes = request.GetTotalReceivedBytes();
+void DataUseMeasurement::ReportDataUseUMAOnNetworkAccess(
+    const net::URLRequest& request,
+    TrafficDirection dir,
+    int64_t bytes) const {
   bool is_user_traffic = IsUserInitiatedRequest(request);
-
   bool is_connection_cellular =
       net::NetworkChangeNotifier::IsConnectionCellular(
           net::NetworkChangeNotifier::GetConnectionType());
@@ -129,35 +125,46 @@ void DataUseMeasurement::ReportDataUseUMA(
   DataUseUserData::ServiceName service_name =

[Not at Google. Contact bengr, 2016/10/06 22:17:47]

To avoid repeating the ternary operator, suggest:

DataUseUserData::ServiceName service_name = DataUseUserData::NOT_TAGGED;
DataUseUserData::AppState old_app_state = DataUseUserData::FOREGROUND;
DataUseUserData* attached_service_data = ...
if (attached_service_data) {
  service_name = attached_service_data->service_name();
  old_app_state = attached_service_data->app_state();
  if (old_app_state != new_app_state)
    attached_service_data->set_app_state(CurrentAppState());
}

[Raj, 2016/10/06 23:17:51]

Done.

       attached_service_data ? attached_service_data->service_name()
                             : DataUseUserData::NOT_TAGGED;
-  bool started_in_foreground =
-      attached_service_data
-          ? attached_service_data->app_state() == DataUseUserData::FOREGROUND
-          : CurrentAppState() == DataUseUserData::FOREGROUND;
+  DataUseUserData::AppState old_app_state =
+      attached_service_data ? attached_service_data->app_state()
+                            : DataUseUserData::FOREGROUND;
+  DataUseUserData::AppState new_app_state = old_app_state == CurrentAppState()

[Not at Google. Contact bengr, 2016/10/06 22:17:48]

Why not just use CurrentAppState()?

[Raj, 2016/10/06 23:17:51]

CurrentAppState() only returns FOREGROUND or BACKGROUND.
It should be compared with old_app_state to determine if the current data use
should be logged as FOREGROUND or BACKGROUND or UNKNOWN.

If the old and current app state is same, the current data use will be logged as
the same state - FOREGROUND or BACKGROUND.
If the old and current app state are different, the current data use will be
logged as UNKNOWN.

+                                                ? old_app_state
+                                                : DataUseUserData::UNKNOWN;
+
+  if (attached_service_data && old_app_state != new_app_state)
+    attached_service_data->set_app_state(CurrentAppState());
 
   RecordUMAHistogramCount(
       GetHistogramName(is_user_traffic ? "DataUse.TrafficSize.User"
                                        : "DataUse.TrafficSize.System",
-                       UPSTREAM, started_in_foreground, is_connection_cellular),
-      total_upload_bytes);
-  RecordUMAHistogramCount(
-      GetHistogramName(is_user_traffic ? "DataUse.TrafficSize.User"
-                                       : "DataUse.TrafficSize.System",
-                       DOWNSTREAM, started_in_foreground,
-                       is_connection_cellular),
-      total_received_bytes);
+                       dir, new_app_state, is_connection_cellular),
+      bytes);
 
   if (!is_user_traffic) {
-    ReportDataUsageServices(service_name, UPSTREAM, started_in_foreground,
-                            is_connection_cellular, total_upload_bytes);
-    ReportDataUsageServices(service_name, DOWNSTREAM, started_in_foreground,
-                            is_connection_cellular, total_received_bytes);
+    ReportDataUsageServices(service_name, dir, new_app_state,
+                            is_connection_cellular, bytes);
   }
+}
+
+void DataUseMeasurement::ReportDataUseUMA(

[Not at Google. Contact bengr, 2016/10/06 22:17:47]

Looks like this is just updating prefs and not reporting UMA. If so, please
rename.

[Raj, 2016/10/06 23:17:51]

Done.

+    const net::URLRequest& request) const {
+  bool is_connection_cellular =
+      net::NetworkChangeNotifier::IsConnectionCellular(
+          net::NetworkChangeNotifier::GetConnectionType());
+
+  DataUseUserData* attached_service_data = reinterpret_cast<DataUseUserData*>(

[Not at Google. Contact bengr, 2016/10/06 22:17:47]

Can this be a static_cast?

[Raj, 2016/10/06 23:17:51]

Done.

+      request.GetUserData(DataUseUserData::kUserDataKey));
+  DataUseUserData::ServiceName service_name =
+      attached_service_data ? attached_service_data->service_name()
+                            : DataUseUserData::NOT_TAGGED;
 
   // Update data use prefs for cellular connections.
   if (!metrics_data_use_forwarder_.is_null()) {
     metrics_data_use_forwarder_.Run(
-        attached_service_data->GetServiceNameAsString(service_name),
-        total_upload_bytes + total_received_bytes, is_connection_cellular);
+        DataUseUserData::GetServiceNameAsString(service_name),
+        request.GetTotalSentBytes() + request.GetTotalReceivedBytes(),
+        is_connection_cellular);
   }
 }
 
@@ -194,11 +201,14 @@ DataUseUserData::AppState DataUseMeasurement::CurrentAppState() const {
 std::string DataUseMeasurement::GetHistogramName(
     const char* prefix,
     TrafficDirection dir,
-    bool started_in_foreground,
+    DataUseUserData::AppState app_state,
     bool is_connection_cellular) const {
   return base::StringPrintf(
       "%s.%s.%s.%s", prefix, dir == UPSTREAM ? "Upstream" : "Downstream",
-      started_in_foreground ? "Foreground" : "Background",
+      app_state == DataUseUserData::UNKNOWN
+          ? "Unknown"
+          : (app_state == DataUseUserData::FOREGROUND ? "Foreground"
+                                                      : "Background"),
       is_connection_cellular ? "Cellular" : "NotCellular");
 }
 
@@ -246,7 +256,7 @@ void DataUseMeasurement::MaybeRecordNetworkBytesOS() {
 void DataUseMeasurement::ReportDataUsageServices(
     DataUseUserData::ServiceName service,
     TrafficDirection dir,
-    bool started_in_foreground,
+    DataUseUserData::AppState app_state,
     bool is_connection_cellular,
     int64_t message_size) const {
   RecordUMAHistogramCount(
@@ -254,8 +264,8 @@ void DataUseMeasurement::ReportDataUsageServices(
       message_size);
   if (message_size > 0) {
     IncreaseSparseHistogramByValue(
-        GetHistogramName("DataUse.MessageSize.AllServices", dir,
-                         started_in_foreground, is_connection_cellular),
+        GetHistogramName("DataUse.MessageSize.AllServices", dir, app_state,
+                         is_connection_cellular),
         service, message_size);
   }
 }