Merge https://codereview.chromium.org/739033002 onto 2214 branch

Always send enabled state in extension update checks

This changes the extension update system to always send a flag for
whether each extension is enabled or not when checking for updates, and
if it is disabled what the reasons are. The behavior before this patch is
to only send this metric when UMA is turned on. We already do not send cookies
with these requests, so the privacy review team gave sign off for this.

BUG=433376

Review URL: https://codereview.chromium.org/739033002

Cr-Commit-Position: refs/heads/master@{#304817}
(cherry picked from commit cd3127f9c92eed5afaf4f72fa1bd431975d3cdd8)

Review URL: https://codereview.chromium.org/739493004

Cr-Commit-Position: refs/branch-heads/2214@{#115}
Cr-Branched-From: 03655fd3f6d72165dc3c9bd2c89807305316fe6c-refs/heads/master@{#303346}

4 files changed, 36 insertions, 49 deletions

diff --git a/chrome/browser/extensions/updater/extension_updater_unittest.cc b/chrome/browser/extensions/updater/extension_updater_unittest.cc
index 0d9b6d4..4fe3953 100644
--- a/chrome/browser/extensions/updater/extension_updater_unittest.cc
+++ b/chrome/browser/extensions/updater/extension_updater_unittest.cc
@@ -1687,39 +1687,49 @@ class ExtensionUpdaterTest : public testing::Test {
 
     // The urls could have been fetched in either order, so use the host to
     // tell them apart and note the query each used.
+    GURL url1_fetch_url;
+    GURL url2_fetch_url;
     std::string url1_query;
     std::string url2_query;
     if (fetched_urls[0].host() == url1.host()) {
+      url1_fetch_url = fetched_urls[0];
+      url2_fetch_url = fetched_urls[1];
+
       url1_query = fetched_urls[0].query();
       url2_query = fetched_urls[1].query();
     } else if (fetched_urls[0].host() == url2.host()) {
+      url1_fetch_url = fetched_urls[1];
+      url2_fetch_url = fetched_urls[0];
       url1_query = fetched_urls[1].query();
       url2_query = fetched_urls[0].query();
     } else {
       NOTREACHED();
     }
 
+    std::map<std::string, ParamsMap> url1_ping_data =
+        GetPingDataFromURL(url1_fetch_url);
+    ParamsMap url1_params = ParamsMap();
+    if (!url1_ping_data.empty() && ContainsKey(url1_ping_data, id))
+      url1_params = url1_ping_data[id];
+
     // First make sure the non-google query had no ping parameter.
-    std::string search_string = "ping%3D";
-    EXPECT_TRUE(url2_query.find(search_string) == std::string::npos);
+    EXPECT_TRUE(GetPingDataFromURL(url2_fetch_url).empty());
 
     // Now make sure the google query had the correct ping parameter.
-    bool ping_expected = false;
     bool did_rollcall = false;
     if (rollcall_ping_days != 0) {
-      search_string += "r%253D" + base::IntToString(rollcall_ping_days);
+      ASSERT_TRUE(ContainsKey(url1_params, "r"));
+      ASSERT_EQ(1u, url1_params["r"].size());
+      EXPECT_EQ(base::IntToString(rollcall_ping_days),
+                *url1_params["r"].begin());
       did_rollcall = true;
-      ping_expected = true;
     }
-    if (active_bit && active_ping_days != 0) {
-      if (did_rollcall)
-        search_string += "%2526";
-      search_string += "a%253D" + base::IntToString(active_ping_days);
-      ping_expected = true;
+    if (active_bit && active_ping_days != 0 && did_rollcall) {
+      ASSERT_TRUE(ContainsKey(url1_params, "a"));
+      ASSERT_EQ(1u, url1_params["a"].size());
+      EXPECT_EQ(base::IntToString(active_ping_days),
+                *url1_params["a"].begin());
     }
-    bool ping_found = url1_query.find(search_string) != std::string::npos;
-    EXPECT_EQ(ping_expected, ping_found) << "query was: " << url1_query
-        << " was looking for " << search_string;
 
     // Make sure the non-google query has no brand parameter.
     const std::string brand_string = "brand%3D";
@@ -1787,14 +1797,10 @@ class ExtensionUpdaterTest : public testing::Test {
   // This lets us run a test with some enabled and some disabled
   // extensions. The |num_enabled| value specifies how many enabled extensions
   // to have, and |disabled| is a vector of DisableReason bitmasks for each
-  // disabled extension we want. |enable_metrics| specifies whether we should
-  // have enabled/disable reason information in the ping parameter.
-  void TestPingMetrics(bool enable_metrics,
-                       int num_enabled,
+  // disabled extension we want.
+  void TestPingMetrics(int num_enabled,
                        const std::vector<int>& disabled) {
     ServiceForManifestTests service(prefs_.get());
-    service.set_enable_metrics(enable_metrics);
-
     ExtensionList enabled_extensions;
     ExtensionList disabled_extensions;
 
@@ -1849,16 +1855,10 @@ class ExtensionUpdaterTest : public testing::Test {
     std::map<std::string, ParamsMap> all_pings = GetPingDataFromURL(url);
 
     // Make sure that all the enabled extensions have "e=1" in their ping
-    // parameter if metrics are turned on, or don't have it if metrics are
-    // off.
+    // parameter.
     for (const auto& ext : enabled_extensions) {
       ASSERT_TRUE(ContainsKey(all_pings, ext->id()));
       ParamsMap& ping = all_pings[ext->id()];
-      if (!enable_metrics) {
-        EXPECT_FALSE(ContainsKey(ping, "e"));
-        EXPECT_FALSE(ContainsKey(ping, "dr"));
-        continue;
-      }
       EXPECT_FALSE(ContainsKey(ping, "dr"));
       ASSERT_TRUE(ContainsKey(ping, "e")) << url;
       std::set<std::string> e = ping["e"];
@@ -1877,11 +1877,6 @@ class ExtensionUpdaterTest : public testing::Test {
       ASSERT_TRUE(ContainsKey(all_pings, ext->id())) << url;
       ParamsMap& ping = all_pings[ext->id()];
 
-      if (!enable_metrics) {
-        EXPECT_FALSE(ContainsKey(ping, "e"));
-        EXPECT_FALSE(ContainsKey(ping, "dr"));
-        continue;
-      }
       ASSERT_TRUE(ContainsKey(ping, "e")) << url;
       std::set<std::string> e = ping["e"];
       ASSERT_EQ(1u, e.size()) << url;
@@ -2238,21 +2233,18 @@ TEST_F(ExtensionUpdaterTest, TestDisabledReasons1) {
   disabled.push_back(Extension::DISABLE_USER_ACTION);
   disabled.push_back(Extension::DISABLE_PERMISSIONS_INCREASE |
                      Extension::DISABLE_CORRUPTED);
-  TestPingMetrics(true, 1, disabled);
-  TestPingMetrics(false, 1, disabled);
+  TestPingMetrics(1, disabled);
 }
 
 TEST_F(ExtensionUpdaterTest, TestDisabledReasons2) {
   std::vector<int> disabled;
-  TestPingMetrics(true, 1, disabled);
-  TestPingMetrics(false, 1, disabled);
+  TestPingMetrics(1, disabled);
 }
 
 TEST_F(ExtensionUpdaterTest, TestDisabledReasons3) {
   std::vector<int> disabled;
   disabled.push_back(0);
-  TestPingMetrics(true, 0, disabled);
-  TestPingMetrics(false, 0, disabled);
+  TestPingMetrics(0, disabled);
 }
 
 // TODO(asargent) - (http://crbug.com/12780) add tests for:
diff --git a/extensions/browser/updater/extension_downloader.cc b/extensions/browser/updater/extension_downloader.cc
index 3068027..da4ff41 100644
--- a/extensions/browser/updater/extension_downloader.cc
+++ b/extensions/browser/updater/extension_downloader.cc
@@ -952,13 +952,8 @@ ManifestFetchData* ExtensionDownloader::CreateManifestFetchData(
     const GURL& update_url,
     int request_id) {
   ManifestFetchData::PingMode ping_mode = ManifestFetchData::NO_PING;
-  if (update_url.DomainIs(ping_enabled_domain_.c_str())) {
-    if (enable_extra_update_metrics_) {
-      ping_mode = ManifestFetchData::PING_WITH_METRICS;
-    } else {
-      ping_mode = ManifestFetchData::PING;
-    }
-  }
+  if (update_url.DomainIs(ping_enabled_domain_.c_str()))
+    ping_mode = ManifestFetchData::PING_WITH_ENABLED_STATE;
   return new ManifestFetchData(
       update_url, request_id, brand_code_, manifest_query_params_, ping_mode);
 }
diff --git a/extensions/browser/updater/manifest_fetch_data.cc b/extensions/browser/updater/manifest_fetch_data.cc
index 2f73acb..6511a1e 100644
--- a/extensions/browser/updater/manifest_fetch_data.cc
+++ b/extensions/browser/updater/manifest_fetch_data.cc
@@ -22,7 +22,7 @@ namespace {
 // request. We want to stay under 2K because of proxies, etc.
 const int kExtensionsManifestMaxURLSize = 2000;
 
-void AddMetricsToPing(std::string* ping_value,
+void AddEnabledStateToPing(std::string* ping_value,
                       const ManifestFetchData::PingData* ping_data) {
   *ping_value += "&e=" + std::string(ping_data->is_enabled ? "1" : "0");
   if (!ping_data->is_enabled) {
@@ -125,8 +125,8 @@ bool ManifestFetchData::AddExtension(const std::string& id,
       if (ping_data->rollcall_days == kNeverPinged ||
           ping_data->rollcall_days > 0) {
         ping_value += "r=" + base::IntToString(ping_data->rollcall_days);
-        if (ping_mode_ == PING_WITH_METRICS)
-          AddMetricsToPing(&ping_value, ping_data);
+        if (ping_mode_ == PING_WITH_ENABLED_STATE)
+          AddEnabledStateToPing(&ping_value, ping_data);
         pings_[id].rollcall_days = ping_data->rollcall_days;
         pings_[id].is_enabled = ping_data->is_enabled;
       }
diff --git a/extensions/browser/updater/manifest_fetch_data.h b/extensions/browser/updater/manifest_fetch_data.h
index 8f33ab3..6ff3396 100644
--- a/extensions/browser/updater/manifest_fetch_data.h
+++ b/extensions/browser/updater/manifest_fetch_data.h
@@ -30,8 +30,8 @@ class ManifestFetchData {
     // Ping without extra metrics.
     PING,
 
-    // Ping with extra metrics.
-    PING_WITH_METRICS,
+    // Ping with information about enabled/disabled state.
+    PING_WITH_ENABLED_STATE,
   };
 
   // Each ping type is sent at most once per day.