Remove one thread hop while fetching matching rules

chrome/browser/android/data_usage/data_use_tab_model.cc

Index: chrome/browser/android/data_usage/data_use_tab_model.cc
diff --git a/chrome/browser/android/data_usage/data_use_tab_model.cc b/chrome/browser/android/data_usage/data_use_tab_model.cc
index 77b6257c9f274d2455203d5c2269c6bd7b248b0b..3930ee3535cf0b23aeec873162e94262c241bd4c 100644
--- a/chrome/browser/android/data_usage/data_use_tab_model.cc
+++ b/chrome/browser/android/data_usage/data_use_tab_model.cc
@@ -40,6 +40,10 @@ const uint32_t kDefaultOpenTabExpirationDurationSeconds =
 const uint32_t kDefaultMatchingRuleExpirationDurationSeconds =
     60 * 60 * 24;  // 24 hours.
 
+// Default maximum number of UI navigation events that are buffered initially
+// until they are processed later.
+const uint32_t kDefaultMaxNavigationEventsBuffered = 100;
+
 const char kUMAExpiredInactiveTabEntryRemovalDurationHistogram[] =
     "DataUsage.TabModel.ExpiredInactiveTabEntryRemovalDuration";
 const char kUMAExpiredActiveTabEntryRemovalDurationHistogram[] =
@@ -133,6 +137,7 @@ DataUseTabModel::DataUseTabModel()
       closed_tab_expiration_duration_(GetClosedTabExpirationDuration()),
       open_tab_expiration_duration_(GetOpenTabExpirationDuration()),
       is_control_app_installed_(false),
+      ui_navigation_buffer(new std::vector<DataUseUINavigationBuffer>()),
       weak_factory_(this) {
   // Detach from current thread since rest of DataUseTabModel lives on the UI
   // thread and the current thread may not be UI thread..
@@ -149,16 +154,15 @@ base::WeakPtr<DataUseTabModel> DataUseTabModel::GetWeakPtr() {
 }
 
 void DataUseTabModel::InitOnUIThread(
-    const scoped_refptr<base::SingleThreadTaskRunner>& io_task_runner,
-    const base::WeakPtr<ExternalDataUseObserver>& external_data_use_observer) {
+    const ExternalDataUseObserverBridge* external_data_use_observer_bridge) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(io_task_runner);
+  DCHECK(external_data_use_observer_bridge);
 
   tick_clock_.reset(new base::DefaultTickClock());
-  data_use_matcher_.reset(new DataUseMatcher(
-      GetWeakPtr(), io_task_runner, external_data_use_observer,
-      GetDefaultMatchingRuleExpirationDuration()));
+  data_use_matcher_.reset(
+      new DataUseMatcher(GetWeakPtr(), external_data_use_observer_bridge,
+                         GetDefaultMatchingRuleExpirationDuration()));
 }
 
 void DataUseTabModel::OnNavigationEvent(SessionID::id_type tab_id,
@@ -170,6 +174,21 @@ void DataUseTabModel::OnNavigationEvent(SessionID::id_type tab_id,
   std::string current_label, new_label;
   bool is_package_match;

[tbansal1, 2016/03/08 18:15:00]

May be declare these variables after the if-block since they are not used in the
if-block.

 
+  if (ui_navigation_buffer) {
+    // Buffer the navigation event for later processing.
+    DataUseUINavigationBuffer ui_event;

[tbansal1, 2016/03/08 18:15:00]

If you want to force callers to fill all the fields of the struct, I would
suggest adding a constructor to the struct.

+    ui_event.tab_id = tab_id;
+    ui_event.transition = transition;
+    ui_event.url = url;
+    ui_event.package = package;
+    ui_navigation_buffer->push_back(ui_event);
+
+    if (ui_navigation_buffer->size() >= kDefaultMaxNavigationEventsBuffered) {

[tbansal1, 2016/03/08 18:15:00]

If the matching rules have not been fetched, what would be effect of this? It
seems that since there are no matching rules, none of the tabs will enter
tracking, and so processing would not have any effect.

May be add a comment to say what is expected to happen here.

+      ProcessBufferedNavigationEvents();
+    }
+    return;
+  }
+
   if (is_control_app_installed_ && !data_use_matcher_->HasValidRules())
     data_use_matcher_->FetchMatchingRules();
 
@@ -259,18 +278,22 @@ void DataUseTabModel::RegisterURLRegexes(
     return;
   data_use_matcher_->RegisterURLRegexes(app_package_name, domain_path_regex,
                                         label);
+  ProcessBufferedNavigationEvents();
 }
 
 void DataUseTabModel::OnControlAppInstallStateChange(
     bool is_control_app_installed) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK_NE(is_control_app_installed_, is_control_app_installed);
-  // If external control app is installed now, rules will be fetched on the next
-  // navigation event.
+
   is_control_app_installed_ = is_control_app_installed;
   std::vector<std::string> empty;
-  if (!is_control_app_installed_)  // Clear rules.
+  if (!is_control_app_installed_) {  // Clear rules.

[tbansal1, 2016/03/08 18:15:00]

nit: move the comment on next line?

[Raj, 2016/03/09 02:27:35]

Done.

     data_use_matcher_->RegisterURLRegexes(empty, empty, empty);
+    ProcessBufferedNavigationEvents();
+  } else {
+    data_use_matcher_->FetchMatchingRules();

[tbansal1, 2016/03/08 18:15:00]

Can we do this in a separate CL and merge it to M-50?

[Raj, 2016/03/09 02:27:35]

Done.

+  }
 }
 
 base::TimeTicks DataUseTabModel::NowTicks() const {
@@ -455,6 +478,17 @@ void DataUseTabModel::CompactTabEntries() {
   }
 }
 
+void DataUseTabModel::ProcessBufferedNavigationEvents() {
+  if (!ui_navigation_buffer)

[tbansal1, 2016/03/08 18:15:00]

DCHECK (thread_checker_...)

+    return;
+  scoped_ptr<std::vector<DataUseUINavigationBuffer>> tmp_ui_navigation_buffer =

[tbansal1, 2016/03/08 18:15:00]

Why move it to a tmp vector? Can we not iterate over the original vector, and
then empty/delete it afterwards?

+      std::move(ui_navigation_buffer);
+  for (const auto& ui_event : *tmp_ui_navigation_buffer.get()) {
+    OnNavigationEvent(ui_event.tab_id, ui_event.transition, ui_event.url,
+                      ui_event.package);
+  }
+}
+
 }  // namespace android
 
 }  // namespace chrome