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..40cd19ccf499399a14af6c37340f2cf8c80d53ba 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,21 +8,127 @@
 
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
-#include "base/single_thread_task_runner.h"
 #include "content/public/browser/browser_thread.h"
 #include "url/gurl.h"
 
+namespace {
+
+// Notifies |data_use_tab_model| of navigation event on IO thread.
+void OnNavigationEventOnIOThread(
+    base::WeakPtr<chrome::android::DataUseTabModel> data_use_tab_model,

[sclittle, 2015/11/17 22:50:11]

For readability, you could move this anonymous namespace inside of

namespace chrome {
namespace android {

so that you don't have to prefix everything with chrome::android:: here and
below.

[tbansal1, 2015/11/18 01:32:23]

Done.

+    int32_t tab_id,
+    chrome::android::DataUseTabModel::TransitionType transition,
+    const GURL gurl) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
+
+  if (!data_use_tab_model)
+    return;
+  data_use_tab_model->OnNavigationEvent(tab_id, transition, gurl);
+}
+
+// Notifies |data_use_tab_model| of tab closure on IO thread.
+void OnTabCloseEventOnIOThread(
+    base::WeakPtr<chrome::android::DataUseTabModel> data_use_tab_model,
+    int32_t tab_id) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
+
+  if (!data_use_tab_model)
+    return;
+  data_use_tab_model->OnTabCloseEvent(tab_id);
+}
+
+// Notifies |data_use_ui_tab_model| on UI thread that tracking has started on
+// tab with id |tab_id|.
+void NotifyTrackingStartingOnUIThread(
+    base::WeakPtr<chrome::android::DataUseUITabModel> | data_use_ui_tab_model |

[mmenke, 2015/11/17 22:48:40]

|?

[tbansal1, 2015/11/18 01:32:23]

Done.

+    ,

[sclittle, 2015/11/17 22:50:11]

I don't think this compiles, please fix this.

[tbansal1, 2015/11/18 01:32:23]

Done.

+    int32_t tab_id) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  if (!data_use_ui_tab_model)
+    return;
+  data_use_ui_tab_model->NotifyTrackingStarting(tab_id);
+}
+
+// Notifies |data_use_ui_tab_model| on UI thread that tracking has ended on tab
+// with id |tab_id|.
+void NotifyTrackingEndingOnUIThread(
+    base::WeakPtr<chrome::android::DataUseUITabModel> data_use_ui_tab_model,
+    int32_t tab_id) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  if (!data_use_ui_tab_model)
+    return;
+  data_use_ui_tab_model->NotifyTrackingEnding(tab_id);
+}
+
+// Sets TabObserverOnIOThread by calling DataUseUITabModel's method on UI
+// thread.
+void SetTabDataUseObserverOnUIThread(
+    base::WeakPtr<chrome::android::DataUseUITabModel> data_use_ui_tab_model,
+    scoped_refptr<chrome::android::DataUseUITabModel::TabObserverOnIOThread>
+        tab_data_use_observer) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  if (!data_use_ui_tab_model)
+    return;
+  data_use_ui_tab_model->SetTabDataUseObserver(tab_data_use_observer);
+}
+
+// Creates a TabObserverOnIOThread object on IO thread.
+void CreateTabObserverOnIOThread(
+    base::WeakPtr<chrome::android::DataUseUITabModel> data_use_ui_tab_model,
+    scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
+  scoped_refptr<chrome::android::DataUseUITabModel::TabObserverOnIOThread>
+      tab_observer(
+          new chrome::android::DataUseUITabModel::TabObserverOnIOThread(
+              data_use_ui_tab_model, ui_task_runner));
+  ui_task_runner->PostTask(FROM_HERE,
+                           base::Bind(&SetTabDataUseObserverOnUIThread,
+                                      data_use_ui_tab_model, tab_observer));
+}
+
+// Passes |data_use_tab_model| to |tab_data_use_observer| on IO thread.
+void SetDataUseTabModelForTabDataUseObserver(
+    scoped_refptr<chrome::android::DataUseUITabModel::TabObserverOnIOThread>
+        tab_data_use_observer,
+    base::WeakPtr<chrome::android::DataUseTabModel> data_use_tab_model) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
+
+  if (tab_data_use_observer)
+    tab_data_use_observer->SetIODataUseTabModel(data_use_tab_model);
+}
+
+}  // namespace
+
 namespace chrome {
 
 namespace android {
 
 DataUseUITabModel::DataUseUITabModel(
-    scoped_refptr<base::SingleThreadTaskRunner> io_task_runner)
-    : io_task_runner_(io_task_runner) {
+    scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
+    scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner)

[sclittle, 2015/11/17 22:50:11]

Instead of taking in a ui_task_runner, you could just use
base::ThreadTaskRunnerHandle::Get():
https://code.google.com/p/chromium/codesearch#chromium/src/base/thread_task_r...

[tbansal1, 2015/11/18 01:32:23]

Done.

+    : io_task_runner_(io_task_runner), weak_factory_(this) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   DCHECK(io_task_runner_);
+  DCHECK(ui_task_runner);
+  DCHECK(ui_task_runner->BelongsToCurrentThread());
+
+  io_task_runner_->PostTask(

[sclittle, 2015/11/17 22:50:11]

It might just be simpler to have CreateTabObserverOnIOThread return the
scoped_refptr<TabObserverOnIOThread>, and then here just call:

base::PostTaskAndReplyWithResult(
    io_task_runner_.get(), FROM_HERE,
    base::Bind(&CreateTabObserverOnIOThread, GetWeakPtr()),
    base::Bind(&DataUseUITabModel::SetTabDataUseObserverOnUIThread,
GetWeakPtr()));

See
https://code.google.com/p/chromium/codesearch#chromium/src/base/task_runner_u...

That way, you could make DataUseUITabModel::SetTabDataUseObserverOnUIThread
private, and you wouldn't have to take in a |ui_task_runner| in this
constructor.

[tbansal1, 2015/11/18 01:32:23]

Done.

+      FROM_HERE, base::Bind(&CreateTabObserverOnIOThread,
+                            weak_factory_.GetWeakPtr(), ui_task_runner));
 }
 
-DataUseUITabModel::~DataUseUITabModel() {}
+DataUseUITabModel::~DataUseUITabModel() {
+  if (tab_data_use_observer_) {
+    DCHECK(thread_checker_.CalledOnValidThread());
+    tab_data_use_observer_->AddRef();
+
+    // Removes the last refptr to |tab_observer|, so that it is destroyed on IO

[sclittle, 2015/11/17 22:50:11]

nit: Say "Removes the last reference" instead of "Removes the last refptr"

[tbansal1, 2015/11/18 01:32:23]

Done.

+    // thread.
+    io_task_runner_->ReleaseSoon(FROM_HERE, tab_data_use_observer_.get());
+    tab_data_use_observer_ = nullptr;
+  }
+}
 
 void DataUseUITabModel::ReportBrowserNavigation(
     const GURL& gurl,
@@ -31,14 +137,29 @@ void DataUseUITabModel::ReportBrowserNavigation(
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  // TODO(tbansal): Post to DataUseTabModel on IO thread.
+  if (tab_id < 0)

[sclittle, 2015/11/17 22:50:11]

For clarity, test for the -1 tab ID at the point where it's generated instead of
here, and just DCHECK here that it's a valid tab ID.

[tbansal1, 2015/11/18 01:32:23]

Done.

+    return;
+
+  DataUseTabModel::TransitionType transition_type;
+
+  if (ConvertTransitionType(page_transition, &transition_type)) {
+    io_task_runner_->PostTask(

[sclittle, 2015/11/17 22:50:11]

PostTask already has built-in support for cancelling callbacks for methods on
WeakPtrs if the WeakPtr is no longer valid, so you should just PostTask to the
DataUseTabModel method directly.

You can remove the anonymous function that you added to do this.

[tbansal1, 2015/11/18 01:32:22]

Done.

+        FROM_HERE,
+        base::Bind(&OnNavigationEventOnIOThread, 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());
 
-  // TODO(tbansal): Post to DataUseTabModel on IO thread.
+  if (tab_id < 0)
+    return;
+
+  io_task_runner_->PostTask(

[sclittle, 2015/11/17 22:50:11]

Same here, just PostTask directly on the DataUseTabModel method directly, since
PostTask will automatically cancel the task if the WeakPtr is invalid.

[tbansal1, 2015/11/18 01:32:23]

Done.

+      FROM_HERE,
+      base::Bind(&OnTabCloseEventOnIOThread, io_data_use_tab_model_, tab_id));
 
   // Clear out local state.
   TabEvents::iterator it = tab_events_.find(tab_id);
@@ -54,7 +175,20 @@ void DataUseUITabModel::ReportCustomTabInitialNavigation(
   // TODO(tbansal): Post to DataUseTabModel on IO thread.
 }
 
-void DataUseUITabModel::OnTrackingStarted(int32_t tab_id) {
+void DataUseUITabModel::SetIODataUseTabModel(
+    base::WeakPtr<DataUseTabModel> io_data_use_tab_model) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  // It is okay to use base::Unretained because this is only called at the time
+  // of profile creation.
+  io_data_use_tab_model_ = io_data_use_tab_model;
+  io_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&SetDataUseTabModelForTabDataUseObserver,

[sclittle, 2015/11/17 22:50:11]

Just post a task to create the observer here with the WeakPtrs, instead of
creating the observer in the constructor. It would simplify the lifecycle of the
Observer.

[tbansal1, 2015/11/18 01:32:23]

Done.

+                            tab_data_use_observer_, io_data_use_tab_model_));
+}
+
+void DataUseUITabModel::NotifyTrackingStarting(int32_t tab_id) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   DCHECK(thread_checker_.CalledOnValidThread());
 
@@ -65,7 +199,7 @@ void DataUseUITabModel::OnTrackingStarted(int32_t tab_id) {
   RemoveTabEvent(tab_id, DATA_USE_TRACKING_ENDED);
 }
 
-void DataUseUITabModel::OnTrackingEnded(int32_t tab_id) {
+void DataUseUITabModel::NotifyTrackingEnding(int32_t tab_id) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   DCHECK(thread_checker_.CalledOnValidThread());
 
@@ -98,8 +232,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());

[sclittle, 2015/11/17 22:50:11]

Should you also DCHECK that this method is only ever called once?

[tbansal1, 2015/11/18 01:32:23]

Added DCHECK

+  tab_data_use_observer_.swap(tab_data_use_observer);

[sclittle, 2015/11/17 22:50:11]

Why swap? Why not just assign it? The previous value must be NULL or else you'd
have to release it properly on the IO thread.

[tbansal1, 2015/11/18 01:32:23]

Done.

+  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);
   if (it == tab_events_.end()) {
     tab_events_.insert(std::make_pair(tab_id, event));
@@ -110,6 +253,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);
   DCHECK(it != tab_events_.end());
   if (it->second == event) {
@@ -119,6 +263,96 @@ 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,
+    scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner)
+    : data_use_ui_tab_model_(data_use_ui_tab_model),
+      ui_task_runner_(ui_task_runner),
+      registered_as_observer_(false) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
+  DCHECK(ui_task_runner_);
+}
+
+DataUseUITabModel::TabObserverOnIOThread::~TabObserverOnIOThread() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (data_use_tab_model_ && registered_as_observer_)
+    data_use_tab_model_->RemoveObserver(this);
+}
+
+void DataUseUITabModel::TabObserverOnIOThread::SetIODataUseTabModel(
+    base::WeakPtr<DataUseTabModel> data_use_tab_model) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  data_use_tab_model_ = data_use_tab_model;
+
+  if (registered_as_observer_)
+    return;

[sclittle, 2015/11/17 22:50:11]

This is weird. It looks like this Observer supports changing the DataUseTabModel
while it's in use, but it never actually removes itself as an observer anywhere.

I'd suggest redesigning the Observer to take in both WeakPtrs on construction
and add itself as an observer immediately. You could get rid of the
|registered_as_observer_| variable as well.

[tbansal1, 2015/11/18 01:32:23]

Done.

+  registered_as_observer_ = true;
+
+  if (data_use_tab_model_)
+    data_use_tab_model_->AddObserver(this);

[sclittle, 2015/11/17 22:50:11]

You should just DCHECK(data_use_tab_model_) when you get it, since there's no
point in having an Observer that doesn't observe anything.

[tbansal1, 2015/11/18 01:32:23]

Done.

+}
+
+void DataUseUITabModel::TabObserverOnIOThread::NotifyTrackingStarting(
+    int32_t tab_id) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  ui_task_runner_->PostTask(FROM_HERE,

[sclittle, 2015/11/17 22:50:11]

PostTask already has built-in support for cancelling callbacks for methods on
WeakPtrs if the WeakPtr is no longer valid.

You can get rid of the anonymous functions for this and the other call below and
just post the method directly on the WeakPtr.

[tbansal1, 2015/11/18 01:32:23]

Done.

+                            base::Bind(&NotifyTrackingStartingOnUIThread,
+                                       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(&NotifyTrackingEndingOnUIThread,
+                                       data_use_ui_tab_model_, tab_id));
+}
+
 }  // namespace android
 
 }  // namespace chrome