Support needed to measure user and service traffic in Chrome.

chrome/browser/net/chrome_network_delegate_unittest.cc

Index: chrome/browser/net/chrome_network_delegate_unittest.cc
diff --git a/chrome/browser/net/chrome_network_delegate_unittest.cc b/chrome/browser/net/chrome_network_delegate_unittest.cc
index 3ca7bbe09a078acaab591f43fdcdc47c065fa020..49bb557ab1c9d0bf22110b1b5645c8a6efcffd2a 100644
--- a/chrome/browser/net/chrome_network_delegate_unittest.cc
+++ b/chrome/browser/net/chrome_network_delegate_unittest.cc
@@ -9,6 +9,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/prefs/pref_member.h"
+#include "base/test/histogram_tester.h"
 #include "chrome/browser/content_settings/cookie_settings_factory.h"
 #include "chrome/browser/net/safe_search_util.h"
 #include "chrome/common/pref_names.h"
@@ -16,7 +17,10 @@
 #include "chrome/test/base/testing_profile.h"
 #include "components/content_settings/core/browser/cookie_settings.h"
 #include "components/content_settings/core/common/pref_names.h"
+#include "components/data_use_measurement/core/data_use_user_data.h"
+#include "content/public/browser/resource_request_info.h"
 #include "content/public/common/content_switches.h"
+#include "content/public/common/resource_type.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "net/base/request_priority.h"
 #include "net/url_request/url_request.h"
@@ -152,6 +156,102 @@ TEST_F(ChromeNetworkDelegateSafeSearchTest, SafeSearch) {
   }
 }
 
+// This class tests recoding data use of user and services in histograms.
+class ChromeNetworkDelegateDataUseMeasurementTest : public testing::Test {

[mmenke, 2015/09/02 21:48:09]

This test fixture doesn't really seem to have anything
DataUseMeasurement-specific.  Can we just rename this ChromeNetworkDelegateTest,
and make the existing ChromeNetworkDelegateTest reuse this text fixture?  It
would just become a 1-line test, I believe.

EXPECT_FALSE(network_delegate_->FirstPartyOnlyCookieExperimentEnabled());

[amohammadkhan, 2015/09/03 04:31:18]

Done.

+ public:
+  ChromeNetworkDelegateDataUseMeasurementTest()
+      : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP) {

[mmenke, 2015/09/02 21:48:09]

context_(true) {  // Delay initialization

context_.set_network_delegate(...);
// Initialization depends on the network delegate, so should initialize the
// context after setting the network delegate
context_.Init();

[amohammadkhan, 2015/09/03 04:31:18]

Done.

+#if defined(ENABLE_EXTENSIONS)
+    forwarder_ = new extensions::EventRouterForwarder();
+#endif
+  }
+
+  void SetUp() override {
+    ChromeNetworkDelegate::InitializePrefsOnUIThread(
+        &enable_referrers_, NULL, NULL, NULL, profile_.GetTestingPrefService());
+  }
+
+ protected:
+  scoped_ptr<net::NetworkDelegate> CreateNetworkDelegate() {
+    scoped_ptr<ChromeNetworkDelegate> network_delegate(
+        new ChromeNetworkDelegate(forwarder(), &enable_referrers_));
+    return network_delegate.Pass();
+  }
+
+  void SetDelegate(net::NetworkDelegate* delegate) {
+    network_delegate_ = delegate;
+    context_.set_network_delegate(network_delegate_);

[mmenke, 2015/09/02 21:48:09]

The NetworkDelegate is being destroyed before the context that uses it here,
which is a little ugly.

[amohammadkhan, 2015/09/03 04:31:18]

I agree. I used a similar approach to other classes in this test file, but it
doesn't look something nice. But I think in this way we make a new
NetworkDelegate for every test and we are sure tests do not affect each other.

+  }
+
+  // This function queries a URLRequest. If |from_user| is true, it attaches a
+  // ResourceRequestInfo to the URLRequest, because requests from users have
+  // this info. If |from_user| is false, the request is presumed to be from a
+  // service, and the service name is set in the request's user data.
+  void QueryURLDataUse(bool from_user) {

[mmenke, 2015/09/02 21:48:09]

This only really depends on the context.  I'd suggest instead making this
static, or maybe even moving it out into an anonymous namespace, and then
calling it:

void RunURLRequest(URLRequestContext* context, bool from_renderer, const GURL&
url) {
  ...
}

[amohammadkhan, 2015/09/03 04:31:18]

Done.

+    scoped_ptr<net::URLRequest> request(context_.CreateRequest(
+        GURL("http://example.com"), net::DEFAULT_PRIORITY, &delegate_));
+    if (from_user) {
+      content::ResourceRequestInfo::AllocateForTesting(
+          request.get(), content::RESOURCE_TYPE_MAIN_FRAME, NULL, -2, -2, -2,
+          true, false, true, true);
+    } else {
+      request->SetUserData(
+          data_use_measurement::DataUseUserData::kUserDataKey,
+          new data_use_measurement::DataUseUserData(
+              data_use_measurement::DataUseUserData::SUGGESTIONS));
+    }
+    request->Start();
+    base::MessageLoop::current()->RunUntilIdle();

[mmenke, 2015/09/02 21:48:09]

I think it's much more robust to do:

// Use a new delegate, so we don't modify the state of the one that's part
// of the test fixture
net::TestDelegate test_delegate;
test_delegate.set_quit_on_complete(true);

scoped_ptr<net::URLRequest> request(context_.CreateRequest(
    GURL("http://example.com"), net::DEFAULT_PRIORITY, &test_delegate));

...
base::MessageLoop::Run();

[amohammadkhan, 2015/09/03 04:31:18]

Done.

+  }
+
+ private:
+  extensions::EventRouterForwarder* forwarder() {
+#if defined(ENABLE_EXTENSIONS)
+    return forwarder_.get();
+#else
+    return NULL;

[Lei Zhang, 2015/09/02 18:14:43]

nit: nullptr, ditto elsewhere.

[amohammadkhan, 2015/09/03 04:31:18]

Done.

+#endif
+  }
+
+  content::TestBrowserThreadBundle thread_bundle_;
+#if defined(ENABLE_EXTENSIONS)
+  scoped_refptr<extensions::EventRouterForwarder> forwarder_;
+#endif
+  TestingProfile profile_;
+  BooleanPrefMember enable_referrers_;
+  scoped_ptr<net::URLRequest> request_;
+  net::TestURLRequestContext context_;
+  net::NetworkDelegate* network_delegate_;

[mmenke, 2015/09/02 21:48:09]

Can't this just be:

net::NetworkDelegate network_delegate_;

And then remove all the initialization code for it?

[amohammadkhan, 2015/09/03 04:31:18]

I think it can't, because we are making a new NetworkDelegate for each test.

+  net::TestDelegate delegate_;
+};
+
+// This function tests data use measurement for different types of requests by
+// making two queries. First query is similar to a query from a service, so it
+// should affect DataUse.TrafficSize.System.Dimensions and
+// DataUse.MessageSize.ServiceName histograms. The second query is similar to a
+// query from a user, so it should affect DataUse.TrafficSize.User.Dimensions.
+// AppState and ConnectionType dimensions are always Foreground and NotCellular
+// respectively.
+TEST_F(ChromeNetworkDelegateDataUseMeasurementTest, MainTest) {

[mmenke, 2015/09/02 21:48:09]

I'd suggest two different tests.

[amohammadkhan, 2015/09/03 04:31:18]

Done.

+  scoped_ptr<net::NetworkDelegate> delegate(CreateNetworkDelegate());
+  SetDelegate(delegate.get());
+  base::HistogramTester histogram_tester;
+
+  QueryURLDataUse(0);  // A query from a service
+  histogram_tester.ExpectTotalCount(
+      "DataUse.TrafficSize.System.Downstream.Foreground.NotCellular", 1);
+  histogram_tester.ExpectTotalCount(
+      "DataUse.TrafficSize.System.Upstream.Foreground.NotCellular", 1);
+  // One upload and one download message, so totalCount shoudl be 2.

[mmenke, 2015/09/02 21:48:09]

nit:  Should

[mmenke, 2015/09/02 21:48:09]

Check that the user counts are 0?

[amohammadkhan, 2015/09/03 04:31:19]

Done.

[amohammadkhan, 2015/09/03 04:31:19]

Done.

+  histogram_tester.ExpectTotalCount("DataUse.MessageSize.Suggestions", 2);
+
+  QueryURLDataUse(1);  // A query from user
+  histogram_tester.ExpectTotalCount(
+      "DataUse.TrafficSize.User.Downstream.Foreground.NotCellular", 1);
+  histogram_tester.ExpectTotalCount(
+      "DataUse.TrafficSize.User.Upstream.Foreground.NotCellular", 1);

[mmenke, 2015/09/02 21:48:09]

I suggest querying the exact bucket we expect, and using a URL with a redirect.

[amohammadkhan, 2015/09/03 04:31:18]

The problem is that if I specify an exact size for a website it can be changed
later and this test will fail, so I can just check the existence of a bucket.

+}
+
 // Privacy Mode disables Channel Id if cookies are blocked (cr223191)
 class ChromeNetworkDelegatePrivacyModeTest : public testing::Test {
  public: