Notify DataUseTabModel of navigations and tab closures

chrome/browser/android/data_usage/data_use_ui_tab_model.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/data_use_ui_tab_model.h"
 
 #include <utility>
 
 #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,
     ui::PageTransition page_transition,
     int32_t tab_id) const {
   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);
   if (it == tab_events_.end())
     return;
   tab_events_.erase(it);
 }
 
 void DataUseUITabModel::ReportCustomTabInitialNavigation(
     int32_t tab_id,
     const std::string& url,
     const std::string& package_name) {
   // 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());
+
   if (MaybeCreateTabEvent(tab_id, DATA_USE_TRACKING_STARTED))
     return;
   // Since tracking started before the UI could indicate that it ended, it is
   // not useful for UI to show that it started again.
   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());
 
   if (MaybeCreateTabEvent(tab_id, DATA_USE_TRACKING_ENDED))
     return;
   // Since tracking ended before the UI could indicate that it stated, it is not
   // useful for UI to show that it ended.
   RemoveTabEvent(tab_id, DATA_USE_TRACKING_STARTED);
 }
 
 bool DataUseUITabModel::HasDataUseTrackingStarted(int32_t tab_id) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));

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

nit: Just use the thread_checker everywhere, since AIUI this object is now only
ever used on the UI thread.

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

Done.

   DCHECK(thread_checker_.CalledOnValidThread());
 
   TabEvents::iterator it = tab_events_.find(tab_id);
   if (it == tab_events_.end())
     return false;
 
   return RemoveTabEvent(tab_id, DATA_USE_TRACKING_STARTED);
 }
 
 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);
   if (it == tab_events_.end())
     return false;
 
   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));
     return true;
   }
   return false;
 }
 
 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) {
     tab_events_.erase(it);
     return true;
   }
   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