Split the data use into foreground, background and unknown

components/data_use_measurement/content/data_use_measurement.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/data_use_measurement/content/data_use_measurement.h"
 
 #include "base/metrics/histogram.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/strings/stringprintf.h"
 #include "build/build_config.h"
@@ skipping 62 matching lines @@
   if (!data_use_user_data) {
     data_use_user_data = new DataUseUserData(
         DataUseUserData::ServiceName::NOT_TAGGED, CurrentAppState());
     request->SetUserData(DataUseUserData::kUserDataKey, data_use_user_data);
   }
 }
 
 void DataUseMeasurement::OnBeforeRedirect(const net::URLRequest& request,
                                           const GURL& new_location) {
   // Recording data use of request on redirects.
   ReportDataUseUMA(request);

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

Is this called here as well as in OnCompleted because GetTotalSentBytes() and
GetTotalReceivedBytes() don't track data use from previous jobs? If so, we
should remember to take this out when those methods are fixed. See
http://crbug/651957. 

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

Added comment.

 }
 
 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
 }
 
 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
 }
 
 void DataUseMeasurement::OnCompleted(const net::URLRequest& request,
                                      bool started) {
   // TODO(amohammadkhan): Verify that there is no double recording in data use
   // of redirected requests.
   ReportDataUseUMA(request);
 #if defined(OS_ANDROID)
   MaybeRecordNetworkBytesOS();
 #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());
 
   DataUseUserData* attached_service_data = reinterpret_cast<DataUseUserData*>(
       request.GetUserData(DataUseUserData::kUserDataKey));
   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);
   }
 }
 
 // static
 bool DataUseMeasurement::IsUserInitiatedRequest(
     const net::URLRequest& request) {
   // Having ResourceRequestInfo in the URL request is a sign that the request is
   // for a web content from user. For now we could add a condition to check
   // ProcessType in info is content::PROCESS_TYPE_RENDERER, but it won't be
   // compatible with upcoming PlzNavigate architecture. So just existence of
@@ skipping 16 matching lines @@
   if (app_state_ != base::android::APPLICATION_STATE_HAS_RUNNING_ACTIVITIES)
     return DataUseUserData::BACKGROUND;
 #endif
   // If the OS is not Android, all the requests are considered Foreground.
   return DataUseUserData::FOREGROUND;
 }
 
 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");
 }
 
 #if defined(OS_ANDROID)
 void DataUseMeasurement::OnApplicationStateChange(
     base::android::ApplicationState application_state) {
   app_state_ = application_state;
   if (app_state_ != base::android::APPLICATION_STATE_HAS_RUNNING_ACTIVITIES)
     MaybeRecordNetworkBytesOS();
 }
@@ skipping 27 matching lines @@
       UMA_HISTOGRAM_COUNTS("DataUse.BytesSent.OS", bytes - tx_bytes_os_);
     }
     tx_bytes_os_ = bytes;
   }
 }
 #endif
 
 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(
       "DataUse.MessageSize." + DataUseUserData::GetServiceNameAsString(service),
       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);
   }
 }
 
 }  // namespace data_use_measurement