ExternalDataUseObserver JNI integration

chrome/browser/android/data_usage/external_data_use_observer.cc

Index: chrome/browser/android/data_usage/external_data_use_observer.cc
diff --git a/chrome/browser/android/data_usage/external_data_use_observer.cc b/chrome/browser/android/data_usage/external_data_use_observer.cc
index 2ffa3ff0914f875aed67afa3d6ea08f1c98c5686..036d861f0298f48511ef1767a115215525a8005b 100644
--- a/chrome/browser/android/data_usage/external_data_use_observer.cc
+++ b/chrome/browser/android/data_usage/external_data_use_observer.cc
@@ -8,7 +8,10 @@
 
 #include "base/android/jni_string.h"
 #include "base/message_loop/message_loop.h"
+#include "base/metrics/field_trial.h"
+#include "base/strings/string_number_conversions.h"
 #include "components/data_usage/core/data_use.h"
+#include "components/variations/variations_associated_data.h"
 #include "content/public/browser/browser_thread.h"
 #include "jni/ExternalDataUseObserver_jni.h"
 #include "third_party/re2/re2/re2.h"
@@ -17,6 +20,20 @@
 using base::android::ConvertUTF8ToJavaString;
 using base::android::ToJavaArrayOfStrings;
 
+namespace {
+
+const char kExternalDataUseObserverFieldTrial[] = "ExternalDataUseObserver";
+
+// Default duration after which matching rules are periodically fetched. May be
+// overridden by the field trial.
+const int kDefaultFetchMatchingRulesDurationSeconds = 60 * 15;  // 15 minutes

[sclittle, 2015/11/06 23:42:49]

nit: add period at the end of the comment.

[tbansal1, 2015/11/07 01:32:43]

Done.

+
+// Default value of the minimum number of bytes that should be buffered before
+// a data use report is submitted. May be overridden by the field trial.
+const int64_t kDefaultDataUseReportMinBytes = 100 * 1000;  // 100 KB.

[sclittle, 2015/11/06 23:42:48]

nit: 1 KB = 1024 bytes

[tbansal1, 2015/11/07 01:32:43]

Done.

+
+}  // namespace
+
 namespace chrome {
 
 namespace android {
@@ -32,11 +49,19 @@ ExternalDataUseObserver::ExternalDataUseObserver(
       io_task_runner_(io_task_runner),
       ui_task_runner_(ui_task_runner),
       previous_report_time_(base::Time::Now()),
+      last_matching_rules_fetch_time_(base::TimeTicks::Now()),
+      total_bytes_buffered_(0),
+      fetch_matching_rules_duration_(base::TimeDelta::FromSeconds(
+          kDefaultFetchMatchingRulesDurationSeconds)),
+      data_use_report_min_bytes_(kDefaultDataUseReportMinBytes),
       io_weak_factory_(this),
       ui_weak_factory_(this) {
   DCHECK(data_use_aggregator_);
   DCHECK(io_task_runner_);
   DCHECK(ui_task_runner_);
+
+  PopulateFieldTrialParams();
+
   ui_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&ExternalDataUseObserver::CreateJavaObjectOnUIThread,
@@ -52,6 +77,35 @@ ExternalDataUseObserver::ExternalDataUseObserver(
   registered_as_observer_ = true;
 }
 
+const std::string
+ExternalDataUseObserver::GetExternalDataUseObserverFieldTrialName() const {
+  return kExternalDataUseObserverFieldTrial;
+}
+
+void ExternalDataUseObserver::PopulateFieldTrialParams() {
+  std::string field_trial = GetExternalDataUseObserverFieldTrialName();
+
+  int32_t fetch_matching_rules_duration_seconds = -1;
+  std::string variation_value = variations::GetVariationParamValue(
+      field_trial, "fetch_matching_rules_duration_seconds");
+  if (!variation_value.empty() &&
+      base::StringToInt(variation_value,
+                        &fetch_matching_rules_duration_seconds)) {
+    fetch_matching_rules_duration_ =
+        base::TimeDelta::FromSeconds(fetch_matching_rules_duration_seconds);
+  }
+  DCHECK_GE(fetch_matching_rules_duration_, base::TimeDelta());
+
+  uint64_t data_use_report_min_bytes = 0;

[sclittle, 2015/11/06 23:42:50]

Use an int64_t here instead of a uint64_t.

[tbansal1, 2015/11/07 01:32:43]

Done.

+  variation_value = variations::GetVariationParamValue(
+      field_trial, "data_use_report_min_bytes");
+  if (!variation_value.empty() &&
+      base::StringToUint64(variation_value, &data_use_report_min_bytes)) {
+    data_use_report_min_bytes_ = data_use_report_min_bytes;

[sclittle, 2015/11/06 23:42:50]

Please don't convert a uint64_t to an int64_t without a static cast or
something.

[tbansal1, 2015/11/07 01:32:43]

Done.

+  }
+  DCHECK_GE(data_use_report_min_bytes_, 0);
+}
+
 void ExternalDataUseObserver::CreateJavaObjectOnUIThread() {
   DCHECK(ui_task_runner_->BelongsToCurrentThread());
   JNIEnv* env = base::android::AttachCurrentThread();
@@ -154,6 +208,15 @@ void ExternalDataUseObserver::OnDataUse(
     const std::vector<const data_usage::DataUse*>& data_use_sequence) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
+  if (base::TimeTicks::Now() - last_matching_rules_fetch_time_ >=
+      fetch_matching_rules_duration_) {
+    last_matching_rules_fetch_time_ = base::TimeTicks::Now();

[sclittle, 2015/11/06 23:42:50]

Could you add a quick comment around here describing what this does?

[tbansal1, 2015/11/07 01:32:43]

Done.

+    ui_task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&ExternalDataUseObserver::FetchMatchingRulesOnUIThread,
+                   GetUIWeakPtr()));
+  }
+
   if (matching_rules_fetch_pending_) {
     // TODO(tbansal): Buffer reports.
   }
@@ -191,6 +254,7 @@ void ExternalDataUseObserver::BufferDataUseReport(
 
   DataUseReport report = DataUseReport(start_time, end_time, data_use->rx_bytes,
                                        data_use->tx_bytes);
+  total_bytes_buffered_ += (data_use->rx_bytes + data_use->tx_bytes);
 
   // Check if the |data_use_report_key| is already in the buffered reports.
   DataUseReports::iterator it =
@@ -200,6 +264,9 @@ void ExternalDataUseObserver::BufferDataUseReport(
     if (buffered_data_reports_.size() == kMaxBufferSize) {
       // TODO(tbansal): Add UMA to track impact of lost reports.
       // Remove the first entry.

[sclittle, 2015/11/06 23:42:49]

Why do you remove the first entry? They aren't in any order anyways, since it's
a hash map, so why not just ignore the new entry?

If you really want to remove the oldest entry, then you should use a linked hash
map:
https://code.google.com/p/chromium/codesearch#chromium/src/net/base/linked_ha...

[tbansal1, 2015/11/07 01:32:43]

Done.

+      total_bytes_buffered_ -=
+          (buffered_data_reports_.begin()->second.bytes_downloaded +
+           buffered_data_reports_.begin()->second.bytes_uploaded);
       buffered_data_reports_.erase(buffered_data_reports_.begin());
     }
     buffered_data_reports_.insert(std::make_pair(data_use_report_key, report));
@@ -225,6 +292,9 @@ void ExternalDataUseObserver::SubmitBufferedDataUseReport() {
   if (submit_data_report_pending_ || buffered_data_reports_.empty())
     return;
 
+  if (total_bytes_buffered_ < data_use_report_min_bytes_)
+    return;
+
   // TODO(tbansal): Keep buffering until enough data has been received.
 
   // Send one data use report.
@@ -234,6 +304,7 @@ void ExternalDataUseObserver::SubmitBufferedDataUseReport() {
 
   // Remove the entry from the map.
   buffered_data_reports_.erase(it);
+  total_bytes_buffered_ -= (report.bytes_downloaded + report.bytes_uploaded);
 
   submit_data_report_pending_ = true;