Notify DataUseTabModel of navigations and tab closures

chrome/browser/android/data_usage/data_use_tab_model.h

Index: chrome/browser/android/data_usage/data_use_tab_model.h
diff --git a/chrome/browser/android/data_usage/data_use_tab_model.h b/chrome/browser/android/data_usage/data_use_tab_model.h
index 2420de1a7a85465d58ade6bbbb54af066d2fd8a7..008cc4f08fa6d63dabf5a29f271bef066d05b626 100644
--- a/chrome/browser/android/data_usage/data_use_tab_model.h
+++ b/chrome/browser/android/data_usage/data_use_tab_model.h
@@ -14,13 +14,19 @@
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
-#include "base/observer_list_threadsafe.h"
 #include "base/threading/thread_checker.h"
 #include "base/time/time.h"
 #include "chrome/browser/android/data_usage/tab_data_use_entry.h"
-#include "components/data_usage/core/data_use.h"
 #include "url/gurl.h"
 
+namespace base {
+class SingleThreadTaskRunner;
+}  // namespace base

[bengr, 2015/11/23 18:26:25]

No need for the end comment for a single-line namespace declaration.

[tbansal1, 2015/11/23 19:58:08]

Done.

+
+namespace data_usage {
+struct DataUse;
+}  // namespace data_usage

[bengr, 2015/11/23 18:26:25]

No need for the end comment for a single-line namespace declaration.

[tbansal1, 2015/11/23 19:58:08]

Done.

+
 namespace chrome {
 
 namespace android {
@@ -46,12 +52,6 @@ class DataUseTabModel {
     // TODO(rajendrant): Remove this if not needed.
     TRANSITION_FROM_EXTERNAL_APP,
 
-    // Navigating to another app.
-    TRANSITION_TO_EXTERNAL_APP,
-
-    // Navigation from NavSuggest below omnibox.
-    TRANSITION_FROM_NAVSUGGEST,
-
     // Navigation from the omnibox when typing a URL.
     TRANSITION_OMNIBOX_NAVIGATION,
 
@@ -63,7 +63,7 @@ class DataUseTabModel {
   };
 
   // TabDataUseObserver provides the interface for getting notifications from
-  // the DataUseTabModel.
+  // the DataUseTabModel. TabDataUseObserver is called back on UI thread.
   class TabDataUseObserver {
    public:
     virtual ~TabDataUseObserver() {}
@@ -107,10 +107,20 @@ class DataUseTabModel {
   virtual bool GetLabelForDataUse(const data_usage::DataUse& data_use,
                                   std::string* output_label) const;
 
-  // Adds or removes observers from the observer list. These functions are
-  // thread-safe and can be called from any thread.
-  void AddObserver(TabDataUseObserver* observer);
-  void RemoveObserver(TabDataUseObserver* observer);
+  // Adds or removes observers from the observer list. Must be called on IO
+  // thread. |weak_ptr_observer| is notified on UI thread.
+  void AddObserver(const TabDataUseObserver* observer,
+                   const base::WeakPtr<TabDataUseObserver> weak_ptr_observer);
+  void RemoveObserver(const TabDataUseObserver* observer);
+
+ protected:

[bengr, 2015/11/23 18:26:25]

Since you've already friended a bunch of stuff, why not make these private?

[tbansal1, 2015/11/23 19:58:08]

Friending is used by tests in data_use_tab_model_unittest.cc.
"Protecting" makes it easier for derived test classes in
data_use_ui_tab_model_unittest.cc

[bengr, 2015/11/24 23:37:44]

Acknowledged.

+  // Notifies the observers that a data usage tracking session started for
+  // |tab_id|. Protected for testing.
+  void NotifyObserversOfTrackingStarting(int32_t tab_id);

[bengr, 2015/11/23 18:26:25]

You should probably use the SessionID::tab_id type everywhere, though I don't
feel strongly about that.

[tbansal1, 2015/11/23 19:58:08]

Done.

+
+  // Notifies the observers that an active data usage tracking session ended for
+  // |tab_id|. Protected for testing.
+  void NotifyObserversOfTrackingEnding(int32_t tab_id);
 
  private:
   friend class DataUseTabModelTest;
@@ -134,19 +144,13 @@ class DataUseTabModel {
   FRIEND_TEST_ALL_PREFIXES(DataUseTabModelTest,
                            ExpiredActiveTabEntryRemovaltimeHistogram);
 
+  typedef base::hash_map<const TabDataUseObserver*,
+                         const base::WeakPtr<TabDataUseObserver>> ObserverMap;
   typedef base::hash_map<int32_t, TabDataUseEntry> TabEntryMap;
 
   // Virtualized for unit test support.
   virtual base::TimeTicks Now() const;
 
-  // Notifies the observers that a data usage tracking session started for
-  // |tab_id|.
-  void NotifyObserversOfTrackingStarting(int32_t tab_id);
-
-  // Notifies the observers that an active data usage tracking session ended for
-  // |tab_id|.
-  void NotifyObserversOfTrackingEnding(int32_t tab_id);
-
   // Initiates a new tracking session with the |label| for tab with id |tab_id|.
   void StartTrackingDataUse(int32_t tab_id, const std::string& label);
 
@@ -163,9 +167,10 @@ class DataUseTabModel {
   // |data_use_observer_| outlives this instance.
   const ExternalDataUseObserver* data_use_observer_;
 
-  // List of observers waiting for tracking session start and end notifications.
-  const scoped_refptr<base::ObserverListThreadSafe<TabDataUseObserver>>
-      observer_list_;
+  // Map of observers that receive tracking session start and end
+  // notifications. Map is keyed by raw pointers, and notifications are posted

[bengr, 2015/11/23 18:26:25]

This comment is very confusing. Why do you need the raw pointers at all? Why not
just have  a list of WeakPtrs?

[tbansal1, 2015/11/23 19:58:08]

Raw pointers are needed to remove observers. It is not possible to compare weak
pointers.

[bengr, 2015/11/24 23:37:44]

Acknowledged.

+  // to the weak pointers on UI thread.
+  ObserverMap observers_;
 
   // Maintains the tracking sessions of multiple tabs.
   TabEntryMap active_tabs_;
@@ -173,10 +178,11 @@ class DataUseTabModel {
   // Maximum number of tab entries to maintain session information about.
   const size_t max_tab_entries_;
 
+  // |ui_task_runner_| is used to notify TabDataUseObserver on UI thread.
+  scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner_;
+
   base::ThreadChecker thread_checker_;
 
-  // |weak_factory_| is used for generating weak pointers to be passed to
-  // DataUseTabUIManager on the UI thread.
   base::WeakPtrFactory<DataUseTabModel> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DataUseTabModel);