Support needed to measure user and service traffic in Chrome.

chrome/browser/net/data_use_measurement.h

Index: chrome/browser/net/data_use_measurement.h
diff --git a/chrome/browser/net/data_use_measurement.h b/chrome/browser/net/data_use_measurement.h
new file mode 100644
index 0000000000000000000000000000000000000000..905ed6042cd0c362f81a54069dc1c95ea87cf3b0
--- /dev/null
+++ b/chrome/browser/net/data_use_measurement.h
@@ -0,0 +1,92 @@
+// 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.
+
+#ifndef CHROME_BROWSER_NET_DATA_USE_MEASUREMENT_H_
+#define CHROME_BROWSER_NET_DATA_USE_MEASUREMENT_H_
+
+#include <string>
+
+#include "base/memory/ref_counted.h"
+#include "build/build_config.h"
+
+#if defined(OS_ANDROID)
+#include "base/android/application_status_listener.h"
+#endif
+
+namespace net {
+class URLRequest;
+}
+

[sclittle, 2015/08/19 23:32:39]

Put all the new classes and stuff in a namespace like chrome_browser_net

[amohammadkhan, 2015/08/21 23:06:32]

Done.

+// Specifies that data is received or sent, respectively.
+enum TrafficDirection { DOWNLOAD, UPLOAD };
+
+// The state of the application. Only available on Android and on other
+// platforms it is always FOREGROUND.

[sclittle, 2015/08/19 23:32:40]

These enums should be moved below into the DataUseMeasurement class.

[amohammadkhan, 2015/08/21 23:06:32]

Done.

+enum AppState { BACKGROUND, FOREGROUND };

[sclittle, 2015/08/19 23:32:39]

Why is this enum needed? Could you just use a boolean?

[amohammadkhan, 2015/08/21 23:06:32]

You are right a bool may be enough, but basically the states of app are more
than these two ( I think they are four), so I thought maybe we may  get
interested in all of those later, so by having the enum, it will be an easier
task to support those too.

+
+// The type of network connection that was used.
+enum ConnectionType { CELLULAR, NOT_CELLULAR };

[sclittle, 2015/08/19 23:32:40]

Why is this enum needed? Could you just use a boolean?

[amohammadkhan, 2015/08/21 23:06:32]

The similar reason to AppState enum. Again the connection types are more than
these two, so changing the code to support more detailed connection types will
be easier by having this enum.

[brettw, 2015/09/11 19:53:44]

I like the enums even for binary states in many cases, to avoid function calls
with (true, false, true, true) and you don't know what's going on.

However, these are in the global namespace. I don't think you should be
introducing values like FOREGROUND, etc. into the global namespace. Also, the
enum names "AppState" and "ConnectionType" might be overly generic in a product
as big as Chrome.

If these can be moved inside of DataUseManagement, that would be best.

[mmenke, 2015/09/11 19:57:07]

You're looking at the wrong patch set.

[amohammadkhan, 2015/09/11 20:11:26]

I totally agree with you and as mmenke@ said they are inside the namespace in
last patch.

+
+// The reason a request was sent. Whether the traffic originates from
+// user's request for a web site or a service is the source of the traffic.
+enum ChromeTrafficType { USER_TRAFFIC, SERVICE_TRAFFIC };
+
+class DataUseMeasurement
+    : public base::RefCountedThreadSafe<DataUseMeasurement> {

[sclittle, 2015/08/19 23:32:40]

Why does this class need to be refcounted?

[amohammadkhan, 2015/08/21 23:06:31]

I think I was over cautious because of callback function, I removed it.

+ public:
+  // This constructor initialize the

[sclittle, 2015/08/19 23:32:39]

finish this comment

[amohammadkhan, 2015/08/21 23:06:32]

Done.

+  DataUseMeasurement();
+
+  // Records the data use of the |request|. It is used for recording data use
+  // of User and Non-User Data in ChromeNetworkDelegate. After extracting
+  // necessary data, such as message size in different directions and its source
+  // (Whether it is User or non-user data), it calls the appropriate function
+  // to record the result in DataUse.User.{Dimensions},
+  // DataUse.NotUser.{Dimensions}, and DataUse.Service.ServiceName. The first
+  // histogram is updated if the traffic originated from user's request and the
+  // latter two are used if traffic is originated from a service.

[sclittle, 2015/08/19 23:32:40]

This comment is too wordy, could you cut this down and not describe the
implementation? Even just the first sentence might be enough.

[amohammadkhan, 2015/08/21 23:06:31]

Done.

+  void ReportDataUseUMA(const net::URLRequest* request);
+
+#if defined(OS_ANDROID)
+  // If a change in application status happens, this function is called and this
+  // function sets app_state_ appropriately.
+  void OnApplicationStateChange(
+      base::android::ApplicationState application_state);
+#endif
+
+ private:
+  friend class base::RefCountedThreadSafe<DataUseMeasurement>;
+  ~DataUseMeasurement(){}
+
+#if defined(OS_ANDROID)
+  // Application listener store the last known state of the application in this
+  // field.
+  base::android::ApplicationState app_state_;
+
+  // ApplicationStatusListener used to monitor that application is in foreground
+  // or in background.
+  base::android::ApplicationStatusListener* app_listener_;

[sclittle, 2015/08/19 23:32:40]

What owns this object? Please comment that here.

[amohammadkhan, 2015/08/21 23:06:31]

Done.

+#endif
+
+  // Helper function used for reporting data use of URLRequests.
+  void ReportDataUsageURLRequest(ChromeTrafficType service_type,

[sclittle, 2015/08/19 23:32:40]

Move these functions above the private attributes.

[amohammadkhan, 2015/08/21 23:06:32]

Done.

+                                 TrafficDirection dir,
+                                 int message_size);
+
+  // Helper function used to record data use of services.
+  void ReportDataUsage(const std::string& service_name,
+                       TrafficDirection dir,
+                       int message_size);
+
+  // Making the current dimension of measured data traffic.

[sclittle, 2015/08/19 23:32:39]

I don't understand this comment.

[amohammadkhan, 2015/08/21 23:06:32]

Done.

+  std::string DimensionMaker(TrafficDirection dir);
+
+  // Returns the current connection type (Cellular or Not cellular).
+  ConnectionType CurrentConnectionType();
+
+  // Returns the current application state (Foreground or Background). It always
+  // returns Foreground if Chrome is not running on Android.
+  AppState CurrentAppState();
+};
+#endif  // CHROME_BROWSER_NET_DATA_USE_MEASUREMENT_H_