Remove one thread hop while fetching matching rules

chrome/browser/android/data_usage/external_data_use_observer.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/android/data_usage/external_data_use_observer.h"
 
 #include <utility>
 
 #include "base/containers/hash_tables.h"
 #include "base/message_loop/message_loop.h"
@@ skipping 121 matching lines @@
     data_usage::DataUseAggregator* data_use_aggregator,
     const scoped_refptr<base::SingleThreadTaskRunner>& io_task_runner,
     const scoped_refptr<base::SingleThreadTaskRunner>& ui_task_runner)
     : data_use_aggregator_(data_use_aggregator),
       data_use_tab_model_(new DataUseTabModel()),
       last_data_report_submitted_ticks_(base::TimeTicks()),
       pending_report_bytes_(0),
       ui_task_runner_(ui_task_runner),
       previous_report_time_(base::Time::Now()),
       last_matching_rules_fetch_time_(base::TimeTicks::Now()),
       external_data_use_observer_bridge_(new ExternalDataUseObserverBridge()),

[tbansal1, 2016/03/10 01:00:58]

change the order so that bridge is constructed before tab model.

[Raj, 2016/03/10 02:13:34]

Done.

       total_bytes_buffered_(0),
       fetch_matching_rules_duration_(
           base::TimeDelta::FromSeconds(GetFetchMatchingRulesDurationSeconds())),
       data_use_report_min_bytes_(GetMinBytes()),
       data_report_submit_timeout_(
           base::TimeDelta::FromMilliseconds(GetDataReportSubmitTimeoutMsec())),
 #if defined(OS_ANDROID)
       app_state_listener_(new base::android::ApplicationStatusListener(
           base::Bind(&ExternalDataUseObserver::OnApplicationStateChange,
                      base::Unretained(this)))),
 #endif
       registered_as_data_use_observer_(false),
       weak_factory_(this) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
   DCHECK(data_use_aggregator_);
   DCHECK(io_task_runner);
   DCHECK(ui_task_runner_);
   DCHECK(last_data_report_submitted_ticks_.is_null());
 
   ui_task_runner_->PostTask(FROM_HERE,
                             base::Bind(&DataUseTabModel::InitOnUIThread,
                                        base::Unretained(data_use_tab_model_),
-                                       io_task_runner, GetWeakPtr()));
+                                       external_data_use_observer_bridge_));
 
-  // Initialize the ExternalDataUseObserverBridge object. Initialization will
-  // also trigger the fetching of matching rules. It is okay to use
-  // base::Unretained here since |external_data_use_observer_bridge_| is
-  // owned by |this|, and is destroyed on UI thread when |this| is destroyed.
+  // Initialize the ExternalDataUseObserverBridge object. It is okay to use
+  // base::Unretained here since |external_data_use_observer_bridge_| is owned
+  // by |this|, and is destroyed on UI thread when |this| is destroyed.
   ui_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&ExternalDataUseObserverBridge::Init,
                  base::Unretained(external_data_use_observer_bridge_),
                  io_task_runner, GetWeakPtr(), data_use_tab_model_));
 }
 
 ExternalDataUseObserver::~ExternalDataUseObserver() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (registered_as_data_use_observer_)
     data_use_aggregator_->RemoveObserver(this);
 
+  // Delete |data_use_tab_model_| on the UI thread. |data_use_tab_model_| should
+  // be deleted before |external_data_use_observer_bridge_|.
+  if (!ui_task_runner_->DeleteSoon(FROM_HERE, data_use_tab_model_)) {
+    NOTIMPLEMENTED() << " DataUseTabModel was not deleted successfully";
+  }
+
   // Delete |external_data_use_observer_bridge_| on the UI thread.
   if (!ui_task_runner_->DeleteSoon(FROM_HERE,
                                    external_data_use_observer_bridge_)) {
     NOTIMPLEMENTED()
         << " ExternalDataUseObserverBridge was not deleted successfully";
   }
-
-  // Delete |data_use_tab_model_| on the UI thread.
-  if (!ui_task_runner_->DeleteSoon(FROM_HERE, data_use_tab_model_)) {
-    NOTIMPLEMENTED() << " DataUseTabModel was not deleted successfully";
-  }
 }
 
 void ExternalDataUseObserver::OnReportDataUseDone(bool success) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(!last_data_report_submitted_ticks_.is_null());
 
   if (success) {
     RecordDataUsageReportSubmission(DATAUSAGE_REPORT_SUBMISSION_SUCCESSFUL,
                                     pending_report_bytes_);
   } else {
@@ skipping 69 matching lines @@
 
   // It is okay to use base::Unretained here since
   // |external_data_use_observer_bridge_| is owned by |this|, and is destroyed
   // on UI thread when |this| is destroyed.
   ui_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&ExternalDataUseObserverBridge::FetchMatchingRules,
                  base::Unretained(external_data_use_observer_bridge_)));
 }
 
+void ExternalDataUseObserver::OnFetchMatchingRulesDone() {

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

I am more inclined to move the 15 minute rule fetch logic to DataUseTabModel.
Then:
 * last_matching_rules_fetch_time_ will be directly updated in tab model.
 * Fetch can happen based on whether control app is installed or not.
 * all the fetch logic will be in one place.
WDYT.

[tbansal1, 2016/03/10 01:00:58]

As discussed offline, I would suggest thinking of how you can re-architecture
all the classes so that it is easier to handle different kinds of data use
tracking. If DataUseTabModel is going to contain one model of data use tracking,
then probably it should not be doing general stuff.

[Raj, 2016/03/10 02:13:34]

Acknowledged.

+  last_matching_rules_fetch_time_ = base::TimeTicks::Now();
+}
+
 void ExternalDataUseObserver::DataUseLabelApplied(
     const data_usage::DataUse& data_use,
     const base::Time& start_time,
     const base::Time& end_time,
     const std::string* label,
     bool label_applied) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (!label_applied)
     return;
@@ skipping 147 matching lines @@
   size_t hash = 1;
   hash = hash * 23 + hash_function(k.label);
   hash = hash * 43 + k.connection_type;
   hash = hash * 83 + hash_function(k.mcc_mnc);
   return hash;
 }
 
 }  // namespace android
 
 }  // namespace chrome