Crop urls in Data Saver feedback reports

components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc

Index: components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc
diff --git a/components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc b/components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc
index 4bc17d0f9fd7a63b252993c43d3918992881b109..aa061320f9c706dde6d2f28393e1db4a2cce9531 100644
--- a/components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc
+++ b/components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc
@@ -9,6 +9,7 @@
 #include "base/basictypes.h"
 #include "base/json/json_writer.h"
 #include "base/stl_util.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/time/time.h"
 #include "base/values.h"
@@ -18,6 +19,8 @@ namespace {
 
 const size_t kMaxEventsToStore = 100;
 
+const int kLastBypassTimeDeltaMS = 300000;

[sclittle, 2015/11/03 01:50:46]

For clarity, could you change this to be in minutes and add a comment explaining
what this is for?

You can use base::TimeDelta::FromMinutes when comparing the times below.

[megjablon, 2015/11/04 01:19:58]

Done.

+
 struct StringToConstant {
   const char* name;
   const int constant;
@@ -239,9 +242,6 @@ std::string DataReductionProxyEventStore::SanitizedLastBypassEvent() const {
 
   std::string str_value;
   int int_value;
-  if (bypass_dict->GetString("time", &str_value))
-    last_bypass->SetString("bypass_time", str_value);
-
   if (params_dict->GetInteger("bypass_type", &int_value))
     last_bypass->SetInteger("bypass_type", int_value);
 
@@ -251,12 +251,30 @@ std::string DataReductionProxyEventStore::SanitizedLastBypassEvent() const {
   if (params_dict->GetString("bypass_duration_seconds", &str_value))
     last_bypass->SetString("bypass_seconds", str_value);
 
+  bool host_only = true;

[sclittle, 2015/11/03 01:50:45]

nit: could you use a more descriptive name here? e.g. truncate_url_to_host or
something.

[megjablon, 2015/11/04 01:19:58]

Done.

+  if (bypass_dict->GetString("time", &str_value)) {
+    last_bypass->SetString("bypass_time", str_value);
+
+    int64 bypass_time;

[sclittle, 2015/11/03 01:50:45]

nit: #include <stdint.h> and use int64_t here instead of int64.

[sclittle, 2015/11/03 01:50:46]

nit: It's not obvious here what the units are, could you rename this to variable
to bypass_ticks_ms or something?

[megjablon, 2015/11/04 01:19:58]

Done.

+    base::StringToInt64(str_value, &bypass_time);
+    int64 now = (base::TimeTicks::Now() - base::TimeTicks()).InMilliseconds();
+
+    // If the last bypass happened in the last 5 minutes, crop the url to the
+    // host.
+    if (now - bypass_time < kLastBypassTimeDeltaMS)

[sclittle, 2015/11/03 01:50:46]

For clarity, instead of comparing raw millisecond counts, could you just compare
them as TimeDeltas? e.g.

base::TimeTicks bypass_ticks = base::TimeTicks() +
base::TimeDelta::FromMilliseconds(bypass_time_ms);

if (base::TimeTicks::Now() - bypass_ticks <
base::TimeDelta::FromMinutes(kLastBypassTimeDeltaMinutes)) {
  ...

[megjablon, 2015/11/04 01:19:59]

Done.

+      host_only = false;
+  }
+
   if (params_dict->GetString("url", &str_value)) {
     GURL url(str_value);
-    GURL::Replacements replacements;
-    replacements.ClearQuery();
-    GURL clean_url = url.ReplaceComponents(replacements);
-    last_bypass->SetString("url", clean_url.spec());
+    if (host_only) {
+      last_bypass->SetString("url", url.host());
+    } else {
+      GURL::Replacements replacements;
+      replacements.ClearQuery();
+      GURL clean_url = url.ReplaceComponents(replacements);
+      last_bypass->SetString("url", clean_url.spec());
+    }
   }
 
   std::string json;