Buffer UI navigation events in DataUseTabModel until rule fetch

chrome/browser/android/data_usage/external_data_use_observer_unittest.cc

Index: chrome/browser/android/data_usage/external_data_use_observer_unittest.cc
diff --git a/chrome/browser/android/data_usage/external_data_use_observer_unittest.cc b/chrome/browser/android/data_usage/external_data_use_observer_unittest.cc
index ae69a61d597d961c89b3d0ad07a4edfa1725af1c..ca327de71be95786b36418fc835c6e15989233e9 100644
--- a/chrome/browser/android/data_usage/external_data_use_observer_unittest.cc
+++ b/chrome/browser/android/data_usage/external_data_use_observer_unittest.cc
@@ -73,8 +73,16 @@ class ExternalDataUseObserverTest : public testing::Test {
         ->data_use_tab_model_->is_control_app_installed_ = true;
   }
 
+  void ReplaceExternalDataUseObserver() {
+    external_data_use_observer_.reset(new ExternalDataUseObserver(
+        data_use_aggregator_.get(), io_task_runner_.get(),
+        ui_task_runner_.get()));
+    // Wait for |external_data_use_observer_| to create the Java object.
+    base::RunLoop().RunUntilIdle();
+  }
+
   // Replaces |external_data_use_observer_| with a new ExternalDataUseObserver.
-  void ReplaceExternalDataUseObserver(
+  void ReplaceExternalDataUseObserverWithVariation(
       std::map<std::string, std::string> variation_params) {
     variations::testing::ClearAllVariationParams();
     ASSERT_TRUE(variations::AssociateVariationParams(
@@ -85,12 +93,7 @@ class ExternalDataUseObserverTest : public testing::Test {
 
     base::FieldTrialList::CreateFieldTrial(
         ExternalDataUseObserver::kExternalDataUseObserverFieldTrial, "Enabled");
-
-    external_data_use_observer_.reset(new ExternalDataUseObserver(
-        data_use_aggregator_.get(), io_task_runner_.get(),
-        ui_task_runner_.get()));
-    // Wait for |external_data_use_observer_| to create the Java object.
-    base::RunLoop().RunUntilIdle();
+    ReplaceExternalDataUseObserver();
   }
 
   void FetchMatchingRulesDone(const std::vector<std::string>& app_package_name,
@@ -390,7 +393,8 @@ TEST_F(ExternalDataUseObserverTest, PeriodicFetchMatchingRules) {
 // fetched.
 TEST_F(ExternalDataUseObserverTest, MatchingRuleFetchOnControlAppInstall) {
   {
-    // Matching rules not fetched on navigation if control app is not installed.
+    // Matching rules not fetched on navigation if control app is not installed,
+    // and navigation events will be buffered.
     external_data_use_observer()->last_matching_rules_fetch_time_ =
         base::TimeTicks();
     external_data_use_observer()
@@ -401,15 +405,21 @@ TEST_F(ExternalDataUseObserverTest, MatchingRuleFetchOnControlAppInstall) {
         std::string());
     base::RunLoop().RunUntilIdle();
     histogram_tester.ExpectTotalCount("DataUsage.MatchingRulesCount.Valid", 0);
+    EXPECT_EQ(1, external_data_use_observer()
+                     ->data_use_tab_model_->data_use_ui_navigations_->size());
+    external_data_use_observer()
+        ->data_use_tab_model_->data_use_ui_navigations_->clear();
   }
 
   {
-    // Matching rules are fetched when control app is installed.
+    // Matching rules are fetched  when control app is installed.

[tbansal1, 2016/03/29 16:20:41]

extra space?

[Raj, 2016/03/29 18:03:21]

Done.

     base::HistogramTester histogram_tester;
     external_data_use_observer()
         ->data_use_tab_model_->OnControlAppInstallStateChange(true);
     base::RunLoop().RunUntilIdle();
     histogram_tester.ExpectTotalCount("DataUsage.MatchingRulesCount.Valid", 1);
+    EXPECT_FALSE(external_data_use_observer()
+                     ->data_use_tab_model_->data_use_ui_navigations_.get());
   }
 
   {
@@ -585,7 +595,7 @@ TEST_F(ExternalDataUseObserverTest, Variations) {
       base::Int64ToString(kDataUseReportMinBytes);
 
   // Create another ExternalDataUseObserver object.
-  ReplaceExternalDataUseObserver(variation_params);
+  ReplaceExternalDataUseObserverWithVariation(variation_params);
   EXPECT_EQ(base::TimeDelta::FromSeconds(kFetchMatchingRulesDurationSeconds),
             external_data_use_observer()->fetch_matching_rules_duration_);
   EXPECT_EQ(
@@ -603,7 +613,7 @@ TEST_F(ExternalDataUseObserverTest, DataUseReportTimedOut) {
   variation_params["data_use_report_min_bytes"] = "0";
 
   // Create another ExternalDataUseObserver object.
-  ReplaceExternalDataUseObserver(variation_params);
+  ReplaceExternalDataUseObserverWithVariation(variation_params);
   histogram_tester.ExpectTotalCount(kUMAMatchingRuleFirstFetchDurationHistogram,
                                     0);
 
@@ -638,6 +648,71 @@ TEST_F(ExternalDataUseObserverTest, DataUseReportTimedOut) {
   histogram_tester.ExpectTotalCount(kUMAReportSubmissionDurationHistogram, 0);
 }
 
+// Tests the buffering of UI navigation events until matching rule fetch is
+// complete, or until the external control app not installed callback is
+// received, or until a maximum number of events are buffered, whichever comes
+// first.
+TEST_F(ExternalDataUseObserverTest, ProcessBufferedNavigationEvents) {

[tbansal1, 2016/03/29 16:20:41]

 Seems like this is essentially 3 different tests. Can this be split into 3
different tests?

[tbansal1, 2016/03/29 16:20:41]

This test does not really check if ProcessBufferedNavigations did something
useful or not. May be instead of checking the size(), you can see if
ProcessBufferedNavigations actually changed the state of the tab or not.

[Raj, 2016/03/29 18:03:21]

I have added a unittest in data_use_tab_model_unittest.cc to check if tracking
starts for the buffered events.

[Raj, 2016/03/29 18:03:21]

Done.

+  const uint32_t kDefaultMaxNavigationEventsBuffered = 100;
+  {
+    // Navigation events will be buffered until control app not installed
+    // callback is received.
+    external_data_use_observer()->data_use_tab_model_->OnNavigationEvent(
+        kDefaultTabId, DataUseTabModel::TRANSITION_LINK, GURL(kDefaultURL),
+        std::string());
+    external_data_use_observer()->data_use_tab_model_->OnNavigationEvent(
+        kDefaultTabId, DataUseTabModel::TRANSITION_LINK, GURL(kDefaultURL),
+        std::string());
+    EXPECT_EQ(2, external_data_use_observer()
+                     ->data_use_tab_model_->data_use_ui_navigations_->size());
+    external_data_use_observer()
+        ->data_use_tab_model_->OnControlAppInstallStateChange(false);
+    base::RunLoop().RunUntilIdle();
+    EXPECT_FALSE(external_data_use_observer()
+                     ->data_use_tab_model_->data_use_ui_navigations_.get());
+  }
+
+  {
+    // Navigation events will be buffered until control app is installed and
+    // matching rules are fetched.
+    ReplaceExternalDataUseObserver();
+    base::HistogramTester histogram_tester;
+    external_data_use_observer()->data_use_tab_model_->OnNavigationEvent(
+        kDefaultTabId, DataUseTabModel::TRANSITION_LINK, GURL(kDefaultURL),
+        std::string());
+    external_data_use_observer()->data_use_tab_model_->OnNavigationEvent(
+        kDefaultTabId, DataUseTabModel::TRANSITION_LINK, GURL(kDefaultURL),
+        std::string());
+    EXPECT_EQ(2, external_data_use_observer()
+                     ->data_use_tab_model_->data_use_ui_navigations_->size());
+    external_data_use_observer()
+        ->data_use_tab_model_->OnControlAppInstallStateChange(true);
+    base::RunLoop().RunUntilIdle();
+    histogram_tester.ExpectTotalCount("DataUsage.MatchingRulesCount.Valid", 3);
+    EXPECT_FALSE(external_data_use_observer()
+                     ->data_use_tab_model_->data_use_ui_navigations_.get());
+  }
+  {
+    // Navigation events will be buffered until the buffer reaches a maximum
+    // limit.
+    ReplaceExternalDataUseObserver();
+    for (size_t count = 1; count < kDefaultMaxNavigationEventsBuffered;

[tbansal1, 2016/03/29 16:20:41]

this is flaky. If somebody changes value at one place, this test will fail. If
you want to know the value of this outside the class, pass this as a parameter
in the constructor of DataUseTabModel.

[Raj, 2016/03/29 18:03:21]

I do not want to add this limit as another parameter to DataUseTabModel. I have
used a higher number in the testfile and expecting that the buffer will get
cleared eventually at some point (not at an exact size limit). WDYT.

+         count++) {

[tbansal1, 2016/03/29 16:20:41]

++count

[Raj, 2016/03/29 18:03:21]

Done.

+      external_data_use_observer()->data_use_tab_model_->OnNavigationEvent(
+          kDefaultTabId, DataUseTabModel::TRANSITION_LINK, GURL(kDefaultURL),
+          std::string());
+      EXPECT_EQ(i, external_data_use_observer()
+                       ->data_use_tab_model_->data_use_ui_navigations_->size());
+    }
+    // Next navigation event will trigger clearing of the buffer.
+    external_data_use_observer()->data_use_tab_model_->OnNavigationEvent(
+        kDefaultTabId, DataUseTabModel::TRANSITION_LINK, GURL(kDefaultURL),
+        std::string());
+    EXPECT_FALSE(external_data_use_observer()
+                     ->data_use_tab_model_->data_use_ui_navigations_.get());
+  }
+}
+
 }  // namespace android
 
 }  // namespace chrome