Notify DataUseTabModel of navigations and tab closures

chrome/browser/android/data_usage/data_use_ui_tab_model.h

Index: chrome/browser/android/data_usage/data_use_ui_tab_model.h
diff --git a/chrome/browser/android/data_usage/data_use_ui_tab_model.h b/chrome/browser/android/data_usage/data_use_ui_tab_model.h
index b3ad1599e2f5022c951381064119c4ee7420d5dc..6b5159ab35e94203a5fa9a08c57309e8fe0ba745 100644
--- a/chrome/browser/android/data_usage/data_use_ui_tab_model.h
+++ b/chrome/browser/android/data_usage/data_use_ui_tab_model.h
@@ -14,6 +14,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "base/threading/thread_checker.h"
+#include "chrome/browser/android/data_usage/data_use_tab_model.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "ui/base/page_transition_types.h"
 
@@ -34,7 +35,8 @@ namespace android {
 // accessed only on the UI thread.
 // TODO(tbansal): DataUseTabModel should notify DataUseUITabModel when a tab
 // is removed from the list of tabs.
-class DataUseUITabModel : public KeyedService {
+class DataUseUITabModel : public KeyedService,
+                          public DataUseTabModel::TabDataUseObserver {
  public:
   explicit DataUseUITabModel(
       scoped_refptr<base::SingleThreadTaskRunner> io_task_runner);
@@ -60,8 +62,19 @@ class DataUseUITabModel : public KeyedService {
   // Calling this function resets the state of the tab.
   bool HasDataUseTrackingEnded(int32_t tab_id);
 
+  // Sets the weak pointer to DataUseTabModel that can be used on IO thread.
+  void SetIODataUseTabModel(
+      base::WeakPtr<DataUseTabModel> io_data_use_tab_model);
+
+  // Sets the weak pointer to DataUseTabModel that can be used on UI thread,
+  // and registers as an observer to DataUseTabModel::TabDataUseObserver.
+  void SetUIDataUseTabModel(
+      base::WeakPtr<DataUseTabModel> ui_data_use_tab_model);

[sclittle, 2015/11/16 23:13:42]

I don't understand why the DataUseTabModel needs to be used on the UI thread.
Shouldn't all the UI thread logic be part of the DataUseUITabModel instead?

[tbansal1, 2015/11/17 21:12:33]

Done.

+
  private:
   FRIEND_TEST_ALL_PREFIXES(DataUseUITabModelTest, EntranceExitState);
+  FRIEND_TEST_ALL_PREFIXES(DataUseUITabModelTest, ConvertTransitionType);
+  FRIEND_TEST_ALL_PREFIXES(DataUseUITabModelTest, ReportTabEventsTest);
 
   // DataUseTrackingEvent indicates the state of a tab.
   enum DataUseTrackingEvent {
@@ -75,9 +88,8 @@ class DataUseUITabModel : public KeyedService {
   typedef std::unordered_map<int32_t, DataUseTrackingEvent> TabEvents;
 
   // DataUseTabModel::Observer implementation:
-  // TODO(tbansal): Add override once DataUseTabModel is checked in.
-  void OnTrackingStarted(int32_t tab_id);
-  void OnTrackingEnded(int32_t tab_id);
+  void NotifyTrackingStarting(int32_t tab_id) override;
+  void NotifyTrackingEnding(int32_t tab_id) override;
 
   // Creates |event| for tab with id |tab_id| and value |event|, if there is no
   // existing entry for |tab_id|, and returns true. Otherwise, returns false
@@ -88,9 +100,27 @@ class DataUseUITabModel : public KeyedService {
   // returns true. Otherwise, returns false without modifying the entry.
   bool RemoveTabEvent(int32_t tab_id, DataUseTrackingEvent event);
 
+  // Converts |page_transition| to DataUseTabModel::TransitionType enum.
+  // Returns true if conversion was successful, and updates |transition_type|.
+  // Otherwise, returns false, and |transition_type| is not changed.
+  // |transition_type| must not be null.

[Raj, 2015/11/16 22:00:09]

nits: 
rename transition_type to output_transition_type to be more clear.

[tbansal1, 2015/11/17 21:12:33]

I think |transition_type| is okay, since it  is a pointer and not getting passed
by a const ref (so it is clear that function may modify it). Also, the function
name makes it clear that something is getting converted.

+  bool ConvertTransitionType(
+      ui::PageTransition page_transition,
+      DataUseTabModel::TransitionType* transition_type) const;
+
+  // |io_data_use_tab_model_| is used to call DataUseTabModel methods.
+  // |ui_data_use_tab_model_| is used to register as an observer to
+  // DataUseTabModel::TabDataUseObserver.
+  base::WeakPtr<DataUseTabModel> io_data_use_tab_model_;
+  base::WeakPtr<DataUseTabModel> ui_data_use_tab_model_;

[sclittle, 2015/11/16 23:13:42]

It doesn't make sense to have a WeakPtr tied to a different thread than the
DataUseTabModel, since the DataUseTabModel will always be destroyed on the IO
thread. See my other comments about WeakPtr thread un-safety.

[tbansal1, 2015/11/17 21:12:33]

Done.

+
   // |tab_events_| stores tracking events of multiple tabs.
   TabEvents tab_events_;
 
+  // True if |this| is registered as an observer to
+  // DataUseTabModel::TabDataUseObserver.
+  bool registered_as_observer_;
+
   // |io_task_runner_| accesses DataUseTabModel members on IO thread.
   scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_;