Notify DataUseTabModel of navigations and tab closures

chrome/browser/android/data_usage/data_use_ui_tab_model.cc

Index: chrome/browser/android/data_usage/data_use_ui_tab_model.cc
diff --git a/chrome/browser/android/data_usage/data_use_ui_tab_model.cc b/chrome/browser/android/data_usage/data_use_ui_tab_model.cc
index 95e86286088001fabe436cda92c6f9375ccb4a8d..b0f379c28f3d684c5fb940a2cb1d35545c2dbb58 100644
--- a/chrome/browser/android/data_usage/data_use_ui_tab_model.cc
+++ b/chrome/browser/android/data_usage/data_use_ui_tab_model.cc
@@ -8,7 +8,7 @@
 
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
-#include "base/single_thread_task_runner.h"
+#include "base/thread_task_runner_handle.h"
 #include "content/public/browser/browser_thread.h"
 #include "url/gurl.h"
 
@@ -16,29 +16,115 @@ namespace chrome {
 
 namespace android {
 
+// TabObserverOnIOThread registers as an observer to
+// DataUseTabModel::TabDataUseObserver, and gets notified when tracking has
+// started and ended on a tab. It passes these notifications to
+// DataUseUITabModel on UI thread.  TabObserverOnIOThread is not thread safe,
+// and should only be accessed only on IO thread.
+class DataUseUITabModel::TabObserverOnIOThread
+    : public base::RefCountedThreadSafe<TabObserverOnIOThread>,
+      public DataUseTabModel::TabDataUseObserver {
+ public:
+  // Sets the |data_use_tab_model_|, and registers as an observer to
+  // |data_use_tab_model_|. Can be called at most once.
+  TabObserverOnIOThread(
+      base::WeakPtr<DataUseUITabModel> data_use_ui_tab_model,
+      scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner,
+      base::WeakPtr<DataUseTabModel> data_use_tab_model);
+
+ private:
+  friend class DataUseUITabModelTest;
+
+  // Ref counted classes have private destructors to avoid any code deleting
+  // the object accidentally while there are still references to it.
+  friend class base::RefCountedThreadSafe<TabObserverOnIOThread>;
+
+  ~TabObserverOnIOThread();
+
+  // DataUseTabModel::Observer implementation:
+  void NotifyTrackingStarting(int32_t tab_id) override;
+  void NotifyTrackingEnding(int32_t tab_id) override;
+
+  // |data_use_ui_tab_model_| is notified of starting and ending of tracking.
+  // |data_use_ui_tab_model_| should only be used on UI thread.
+  base::WeakPtr<DataUseUITabModel> data_use_ui_tab_model_;
+
+  // |this| registers as an observer to |data_use_tab_model_|.
+  base::WeakPtr<DataUseTabModel> data_use_tab_model_;
+
+  // Used to call methods on |data_use_ui_tab_model_|.
+  scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner_;
+
+  base::ThreadChecker thread_checker_;
+
+  DISALLOW_COPY_AND_ASSIGN(TabObserverOnIOThread);
+};
+
+namespace {
+
+// Creates TabObserverOnIOThread object and passes |data_use_tab_model| to it on
+// IO thread.
+scoped_refptr<DataUseUITabModel::TabObserverOnIOThread>
+CreateTabObserverOnIOThread(
+    base::WeakPtr<DataUseTabModel> data_use_tab_model,

[sclittle, 2015/11/18 21:42:03]

nit: Pass in weak ptrs as const refs here, to avoid extra copy operations.

[tbansal1, 2015/11/19 00:47:07]

Done.

+    base::WeakPtr<DataUseUITabModel> data_use_ui_tab_model,
+    scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
+
+  scoped_refptr<DataUseUITabModel::TabObserverOnIOThread> tab_observer(
+      new DataUseUITabModel::TabObserverOnIOThread(
+          data_use_ui_tab_model, ui_task_runner, data_use_tab_model));
+  return tab_observer;
+}
+
+}  // namespace
+
 DataUseUITabModel::DataUseUITabModel(
     scoped_refptr<base::SingleThreadTaskRunner> io_task_runner)
-    : io_task_runner_(io_task_runner) {
+    : io_task_runner_(io_task_runner),
+      ui_task_runner_(base::ThreadTaskRunnerHandle::Get()),

[sclittle, 2015/11/18 21:42:03]

Is this |ui_task_runner_| necessary? Can't you just call
base::ThreadTaskRunnerHandle::Get() where you use it?

[tbansal1, 2015/11/19 00:47:07]

Done.

+      weak_factory_(this) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   DCHECK(io_task_runner_);
+  DCHECK(ui_task_runner_);
 }
 
-DataUseUITabModel::~DataUseUITabModel() {}
+DataUseUITabModel::~DataUseUITabModel() {
+  if (tab_data_use_observer_) {
+    DCHECK(thread_checker_.CalledOnValidThread());
+    tab_data_use_observer_->AddRef();
+
+    // Removes the last reference to |tab_observer|, so that it is destroyed on
+    // IO thread.
+    io_task_runner_->ReleaseSoon(FROM_HERE, tab_data_use_observer_.get());
+    tab_data_use_observer_ = nullptr;
+  }
+}
 
 void DataUseUITabModel::ReportBrowserNavigation(
     const GURL& gurl,
     ui::PageTransition page_transition,
     int32_t tab_id) const {
-  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_LE(0, tab_id);
 
-  // TODO(tbansal): Post to DataUseTabModel on IO thread.
+  DataUseTabModel::TransitionType transition_type;
+
+  if (ConvertTransitionType(page_transition, &transition_type)) {
+    io_task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&DataUseTabModel::OnNavigationEvent, io_data_use_tab_model_,
+                   tab_id, transition_type, gurl));
+  }
 }
 
 void DataUseUITabModel::ReportTabClosure(int32_t tab_id) {
-  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_LE(0, tab_id);
 
-  // TODO(tbansal): Post to DataUseTabModel on IO thread.
+  io_task_runner_->PostTask(FROM_HERE,
+                            base::Bind(&DataUseTabModel::OnTabCloseEvent,
+                                       io_data_use_tab_model_, tab_id));
 
   // Clear out local state.
   TabEvents::iterator it = tab_events_.find(tab_id);
@@ -51,11 +137,25 @@ void DataUseUITabModel::ReportCustomTabInitialNavigation(
     int32_t tab_id,
     const std::string& url,
     const std::string& package_name) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   // TODO(tbansal): Post to DataUseTabModel on IO thread.
 }
 
-void DataUseUITabModel::OnTrackingStarted(int32_t tab_id) {
-  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+void DataUseUITabModel::SetIODataUseTabModel(
+    base::WeakPtr<DataUseTabModel> io_data_use_tab_model) {

[sclittle, 2015/11/18 21:42:03]

nit: pass in the WeakPtr as a const ref

[tbansal1, 2015/11/19 00:47:06]

Done.

+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  io_data_use_tab_model_ = io_data_use_tab_model;
+
+  base::PostTaskAndReplyWithResult(
+      io_task_runner_.get(), FROM_HERE,
+      base::Bind(&CreateTabObserverOnIOThread, io_data_use_tab_model_,
+                 weak_factory_.GetWeakPtr(), ui_task_runner_),

[sclittle, 2015/11/18 21:42:03]

Could you remove |ui_task_runner_| and just call
base::ThreadTaskRunnerHandle::Get() here?

[tbansal1, 2015/11/19 00:47:06]

Done.

+      base::Bind(&DataUseUITabModel::SetTabDataUseObserver,
+                 weak_factory_.GetWeakPtr()));
+}
+
+void DataUseUITabModel::NotifyTrackingStarting(int32_t tab_id) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (MaybeCreateTabEvent(tab_id, DATA_USE_TRACKING_STARTED))
@@ -65,8 +165,7 @@ void DataUseUITabModel::OnTrackingStarted(int32_t tab_id) {
   RemoveTabEvent(tab_id, DATA_USE_TRACKING_ENDED);
 }
 
-void DataUseUITabModel::OnTrackingEnded(int32_t tab_id) {
-  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+void DataUseUITabModel::NotifyTrackingEnding(int32_t tab_id) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (MaybeCreateTabEvent(tab_id, DATA_USE_TRACKING_ENDED))
@@ -77,7 +176,6 @@ void DataUseUITabModel::OnTrackingEnded(int32_t tab_id) {
 }
 
 bool DataUseUITabModel::HasDataUseTrackingStarted(int32_t tab_id) {
-  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   DCHECK(thread_checker_.CalledOnValidThread());
 
   TabEvents::iterator it = tab_events_.find(tab_id);
@@ -88,7 +186,6 @@ bool DataUseUITabModel::HasDataUseTrackingStarted(int32_t tab_id) {
 }
 
 bool DataUseUITabModel::HasDataUseTrackingEnded(int32_t tab_id) {
-  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   DCHECK(thread_checker_.CalledOnValidThread());
 
   TabEvents::iterator it = tab_events_.find(tab_id);
@@ -98,8 +195,17 @@ bool DataUseUITabModel::HasDataUseTrackingEnded(int32_t tab_id) {
   return RemoveTabEvent(tab_id, DATA_USE_TRACKING_ENDED);
 }
 
+void DataUseUITabModel::SetTabDataUseObserver(
+    scoped_refptr<TabObserverOnIOThread> tab_data_use_observer) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(!tab_data_use_observer_);
+  tab_data_use_observer_ = tab_data_use_observer;
+  DCHECK(tab_data_use_observer_);
+}
+
 bool DataUseUITabModel::MaybeCreateTabEvent(int32_t tab_id,
                                             DataUseTrackingEvent event) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   TabEvents::iterator it = tab_events_.find(tab_id);

[sclittle, 2015/11/18 21:42:03]

nit: this entire method could just be replaced with:

return tab_events_.insert(std::make_pair(tab_id, event)).second;

See http://www.cplusplus.com/reference/unordered_map/unordered_map/insert/

[tbansal1, 2015/11/19 00:47:07]

Done. This is good!

   if (it == tab_events_.end()) {
     tab_events_.insert(std::make_pair(tab_id, event));
@@ -110,6 +216,7 @@ bool DataUseUITabModel::MaybeCreateTabEvent(int32_t tab_id,
 
 bool DataUseUITabModel::RemoveTabEvent(int32_t tab_id,
                                        DataUseTrackingEvent event) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   TabEvents::iterator it = tab_events_.find(tab_id);

[sclittle, 2015/11/18 21:42:03]

nit: this entire method could just be replaced with:

return tab_events_.erase(tab_id) > 0;

See http://www.cplusplus.com/reference/unordered_map/unordered_map/erase/

[tbansal1, 2015/11/19 00:47:07]

Probably not, we match both the Key and Value before deleting.

   DCHECK(it != tab_events_.end());
   if (it->second == event) {
@@ -119,6 +226,86 @@ bool DataUseUITabModel::RemoveTabEvent(int32_t tab_id,
   return false;
 }
 
+bool DataUseUITabModel::ConvertTransitionType(
+    ui::PageTransition page_transition,
+    DataUseTabModel::TransitionType* transition_type) const {
+  if (!ui::PageTransitionIsValidType(page_transition) ||
+      !ui::PageTransitionIsMainFrame(page_transition) ||
+      !ui::PageTransitionIsNewNavigation(page_transition)) {
+    return false;
+  }
+
+  const int32_t mask = 0xFFFFFFFF ^ ui::PAGE_TRANSITION_QUALIFIER_MASK;
+
+  switch (page_transition & mask) {
+    case ui::PAGE_TRANSITION_LINK:
+      if ((page_transition & ui::PAGE_TRANSITION_FROM_API) != 0) {
+        // Clicking on bookmarks.
+        *transition_type = DataUseTabModel::TRANSITION_BOOKMARK;
+        return true;
+      }
+      return false;  // Newtab, clicking on a link.
+    case ui::PAGE_TRANSITION_TYPED:
+      *transition_type = DataUseTabModel::TRANSITION_OMNIBOX_NAVIGATION;
+      return true;
+    case ui::PAGE_TRANSITION_AUTO_BOOKMARK:
+      // Auto bookmark from newtab page.
+      *transition_type = DataUseTabModel::TRANSITION_BOOKMARK;
+      return true;
+    case ui::PAGE_TRANSITION_AUTO_TOPLEVEL:
+      // History menu.
+      *transition_type = DataUseTabModel::TRANSITION_HISTORY_ITEM;
+      return true;
+    case ui::PAGE_TRANSITION_GENERATED:
+      // Omnibox search (e.g., searching for "tacos").
+      *transition_type = DataUseTabModel::TRANSITION_OMNIBOX_SEARCH;
+      return true;
+    case ui::PAGE_TRANSITION_RELOAD:
+      // Restored tabs.
+      return false;
+    default:
+      return false;
+  }
+}
+
+DataUseUITabModel::TabObserverOnIOThread::TabObserverOnIOThread(
+    base::WeakPtr<DataUseUITabModel> data_use_ui_tab_model,

[sclittle, 2015/11/18 21:42:03]

nit: pass the WeakPtrs as const WeakPtr& to avoid extra copy operations

[tbansal1, 2015/11/19 00:47:07]

Done.

+    scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner,
+    base::WeakPtr<DataUseTabModel> data_use_tab_model)
+    : data_use_ui_tab_model_(data_use_ui_tab_model),
+      data_use_tab_model_(data_use_tab_model),
+      ui_task_runner_(ui_task_runner) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
+  DCHECK(ui_task_runner_);
+  DCHECK(data_use_tab_model_);

[sclittle, 2015/11/18 21:42:03]

This DCHECK isn't guaranteed, the tab model could have been destroyed before the
posted Create() task executed. Instead, just use an if statement around adding
the observer.

[tbansal1, 2015/11/19 00:47:07]

Done.

+
+  data_use_tab_model_->AddObserver(this);
+}
+
+DataUseUITabModel::TabObserverOnIOThread::~TabObserverOnIOThread() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (data_use_tab_model_)
+    data_use_tab_model_->RemoveObserver(this);
+}
+
+void DataUseUITabModel::TabObserverOnIOThread::NotifyTrackingStarting(
+    int32_t tab_id) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  ui_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&DataUseUITabModel::NotifyTrackingStarting,
+                            data_use_ui_tab_model_, tab_id));
+}
+
+void DataUseUITabModel::TabObserverOnIOThread::NotifyTrackingEnding(
+    int32_t tab_id) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  ui_task_runner_->PostTask(FROM_HERE,
+                            base::Bind(&DataUseUITabModel::NotifyTrackingEnding,
+                                       data_use_ui_tab_model_, tab_id));
+}
+
 }  // namespace android
 
 }  // namespace chrome