Separate data use reporting logic in ExternalDataUseObserver

chrome/browser/android/data_usage/external_data_use_reporter.h

Index: chrome/browser/android/data_usage/external_data_use_reporter.h
diff --git a/chrome/browser/android/data_usage/external_data_use_observer.h b/chrome/browser/android/data_usage/external_data_use_reporter.h
similarity index 55%
copy from chrome/browser/android/data_usage/external_data_use_observer.h
copy to chrome/browser/android/data_usage/external_data_use_reporter.h
index 671b1d3d25783a92b9efe198b3c83aed14794e59..e6df1b2d7525248f0189fc073ccc41a4fe8f3621 100644
--- a/chrome/browser/android/data_usage/external_data_use_observer.h
+++ b/chrome/browser/android/data_usage/external_data_use_reporter.h
@@ -1,36 +1,27 @@
-// Copyright 2015 The Chromium Authors. All rights reserved.
+// Copyright 2016 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.
 
-#ifndef CHROME_BROWSER_ANDROID_DATA_USAGE_EXTERNAL_DATA_USE_OBSERVER_H_
-#define CHROME_BROWSER_ANDROID_DATA_USAGE_EXTERNAL_DATA_USE_OBSERVER_H_
+#ifndef CHROME_BROWSER_ANDROID_DATA_USAGE_EXTERNAL_DATA_USE_REPORTER_H_
+#define CHROME_BROWSER_ANDROID_DATA_USAGE_EXTERNAL_DATA_USE_REPORTER_H_
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <memory>
 #include <string>
-#include <vector>
 
 #include "base/containers/hash_tables.h"
-#include "base/gtest_prod_util.h"
 #include "base/macros.h"
-#include "base/memory/ref_counted.h"
-#include "base/memory/weak_ptr.h"
 #include "base/threading/thread_checker.h"
 #include "base/time/time.h"
 #include "chrome/browser/android/data_usage/data_use_tab_model.h"

[tbansal1, 2016/07/15 16:26:30]

You should only need to forward declare this, and move the include to the .cc.

[Raj, 2016/07/15 18:54:41]

Done.

-#include "components/data_usage/core/data_use_aggregator.h"
 #include "net/base/network_change_notifier.h"
 
 #if defined(OS_ANDROID)
 #include "base/android/application_status_listener.h"
 #endif
 
-namespace base {
-class SingleThreadTaskRunner;
-}
-
 namespace data_usage {
 struct DataUse;
 }
@@ -41,14 +32,13 @@ namespace android {
 
 class ExternalDataUseObserverBridge;
 
-// This class allows platform APIs that are external to Chromium to observe how
-// much data is used by Chromium on the current Android device. This class
-// registers as a data use observer with DataUseAggregator (as long as there is
-// at least one valid matching rule is present), filters the received
-// observations by applying the regex matching to the URLs of the requests, and
-// notifies the filtered data use to the platform. This class is not thread
-// safe, and must only be accessed on IO thread.
-class ExternalDataUseObserver : public data_usage::DataUseAggregator::Observer {
+// This class receives data use observations from ExternalDataUseObserver,
+// labels the data use using DataUseTabModel, and buffers the data use report.
+// The buffered reports are submitted to the platform when a minimum number of
+// data usage bytes is reached, or when Chromium goes into background, or when
+// the previous report submission times out. This class is not thread safe, and
+// must only be accessed on UI thread.
+class ExternalDataUseReporter {
  public:
   // Result of data usage report submission.  This enum must remain synchronized
   // with the enum of the same name in metrics/histograms/histograms.xml.
@@ -64,57 +54,38 @@ class ExternalDataUseObserver : public data_usage::DataUseAggregator::Observer {
     DATAUSAGE_REPORT_SUBMISSION_MAX = 4
   };
 
-  // External data use observer field trial name.
-  static const char kExternalDataUseObserverFieldTrial[];
+  // The caller should guarantee that |data_use_tab_model| and
+  // |external_data_use_observer_bridge| to be non-null during the lifetime of
+  // |this|.
+  ExternalDataUseReporter(
+      DataUseTabModel* data_use_tab_model,
+      ExternalDataUseObserverBridge* external_data_use_observer_bridge);
+  virtual ~ExternalDataUseReporter();
 
-  ExternalDataUseObserver(
-      data_usage::DataUseAggregator* data_use_aggregator,
-      const scoped_refptr<base::SingleThreadTaskRunner>& io_task_runner,
-      const scoped_refptr<base::SingleThreadTaskRunner>& ui_task_runner);
-  ~ExternalDataUseObserver() override;
+  // Notifies the ExternalDataUseReporter of data usage. The data use is
+  // labeled using |data_use_tab_model_|, buffered and then reported to
+  // |external_data_use_observer_bridge_| later.
+  void OnDataUse(const data_usage::DataUse& data_use);
 
-  // Returns the pointer to the DataUseTabModel object owned by |this|. The
-  // caller does not owns the returned pointer.
-  DataUseTabModel* GetDataUseTabModel() const;
-
-  // Called by ExternalDataUseObserverBridge::OnReportDataUseDone when a data
-  // use report has been submitted. |success| is true if the request was
-  // successfully submitted to the external data use observer by Java.
   void OnReportDataUseDone(bool success);
 
-  // Called by ExternalDataUseObserverBridge. |should_register| is true if
-  // |this| should register as a data use observer.
-  void ShouldRegisterAsDataUseObserver(bool should_register);
-
-  // Fetches the matching rules asynchronously.
-  void FetchMatchingRules();
-
-  base::WeakPtr<ExternalDataUseObserver> GetWeakPtr();
-
  private:
-  friend class DataUseTabModelTest;
-  friend class DataUseUITabModelTest;
-  friend class ExternalDataUseObserverTest;
-  FRIEND_TEST_ALL_PREFIXES(ExternalDataUseObserverTest, BufferDataUseReports);
-  FRIEND_TEST_ALL_PREFIXES(ExternalDataUseObserverTest, BufferSize);
-  FRIEND_TEST_ALL_PREFIXES(ExternalDataUseObserverTest, DataUseReportTimedOut);
+  friend class ExternalDataUseReporterTest;
+  FRIEND_TEST_ALL_PREFIXES(ExternalDataUseObserverTest, Variations);

[tbansal1, 2016/07/15 16:26:30]

Can the relevant part of the unittest be moved to e_d_u_reporter.unittest so
this class does not have to friend tests of other classes. Ideally, this class
should not know anything about e_d_u_o.

[Raj, 2016/07/15 18:54:41]

Done.

+  FRIEND_TEST_ALL_PREFIXES(ExternalDataUseReporterTest, BufferDataUseReports);
+  FRIEND_TEST_ALL_PREFIXES(ExternalDataUseReporterTest, BufferSize);
+  FRIEND_TEST_ALL_PREFIXES(ExternalDataUseReporterTest, DataUseReportTimedOut);
 #if defined(OS_ANDROID)
-  FRIEND_TEST_ALL_PREFIXES(ExternalDataUseObserverTest,
+  FRIEND_TEST_ALL_PREFIXES(ExternalDataUseReporterTest,
                            DataUseReportingOnApplicationStatusChange);
 #endif
-  FRIEND_TEST_ALL_PREFIXES(ExternalDataUseObserverTest, HashFunction);
-  FRIEND_TEST_ALL_PREFIXES(ExternalDataUseObserverTest,
+  FRIEND_TEST_ALL_PREFIXES(ExternalDataUseReporterTest, HashFunction);
+  FRIEND_TEST_ALL_PREFIXES(ExternalDataUseReporterTest,
                            MatchingRuleFetchOnControlAppInstall);
-  FRIEND_TEST_ALL_PREFIXES(ExternalDataUseObserverTest, MultipleMatchingRules);
-  FRIEND_TEST_ALL_PREFIXES(ExternalDataUseObserverTest,
-                           PeriodicFetchMatchingRules);
-  FRIEND_TEST_ALL_PREFIXES(ExternalDataUseObserverTest,
-                           RegisteredAsDataUseObserver);
-  FRIEND_TEST_ALL_PREFIXES(ExternalDataUseObserverTest, ReportsMergedCorrectly);
-  FRIEND_TEST_ALL_PREFIXES(DataUseUITabModelTest, ReportTabEventsTest);
-  FRIEND_TEST_ALL_PREFIXES(ExternalDataUseObserverTest,
+  FRIEND_TEST_ALL_PREFIXES(ExternalDataUseReporterTest, ReportsMergedCorrectly);
+  FRIEND_TEST_ALL_PREFIXES(ExternalDataUseReporterTest,
                            TimestampsMergedCorrectly);
-  FRIEND_TEST_ALL_PREFIXES(ExternalDataUseObserverTest, Variations);
+  FRIEND_TEST_ALL_PREFIXES(ExternalDataUseReporterTest, MultipleMatchingRules);
 
   // DataUseReportKey is a unique identifier for a data use report.
   struct DataUseReportKey {
@@ -181,29 +152,17 @@ class ExternalDataUseObserver : public data_usage::DataUseAggregator::Observer {
   typedef base::hash_map<DataUseReportKey, DataUseReport, DataUseReportKeyHash>
       DataUseReports;
 
-  // Maximum buffer size. If an entry needs to be added to the buffer that has
-  // size |kMaxBufferSize|, then the oldest entry will be removed.
+  // Maximum size of the data use report buffer. Once this limit is reached, new
+  // reports will be ignored.
+  // TODO(rajendrant): Instead of the ignoring the new data use report, remove
+  // the oldest report entry.
   static const size_t kMaxBufferSize;
 
-  // data_usage::DataUseAggregator::Observer implementation:
-  void OnDataUse(const data_usage::DataUse& data_use) override;
-
-  // Called by DataUseTabModel when tracking info has been retrieved for the
-  // |data_use| object. |tracking_info_valid| is true if |tracking_info| is
-  // populated that applies to the |data_use| object. |tracking_info| is owned
-  // by the caller.
-  void DataUseTrackingInfoRetrieved(
-      const data_usage::DataUse& data_use,
-      const base::Time& start_time,
-      const base::Time& end_time,
-      const DataUseTabModel::TrackingInfo* tracking_info,
-      bool tracking_info_valid);
-
-  // Adds |data_use| to buffered reports. |data_use| is the data use report
-  // received from DataUseAggregator. |label| is a non-empty label that applies
-  // to |data_use|. |tag| is the tag to be applied for this data use.
-  // |start_time| and |end_time| are the start, and end times of the interval
-  // during which bytes reported in |data_use| went over the network.
+  // Adds |data_use| to buffered reports. |data_use| is the received data use
+  // report. |label| is a non-empty label that applies to |data_use|. |tag| is
+  // the tag to be applied for this data use. |start_time| and |end_time| are
+  // the start, and end times of the interval during which bytes reported in
+  // |data_use| went over the network.
   void BufferDataUseReport(const data_usage::DataUse& data_use,
                            const std::string& label,
                            const std::string& tag,
@@ -225,18 +184,19 @@ class ExternalDataUseObserver : public data_usage::DataUseAggregator::Observer {
   void OnApplicationStateChange(base::android::ApplicationState new_state);
 #endif
 
-  // Aggregator that sends data use observations to |this|.
-  data_usage::DataUseAggregator* data_use_aggregator_;
-
-  // |external_data_use_observer_bridge_| is owned by |this|, and interacts with
-  // the Java code. It is created on IO thread but afterwards, should only be
-  // accessed on UI thread.
+  // Pointer to the ExternalDataUseObserverBridge in UI thread. Not owned by
+  // |this|.
   ExternalDataUseObserverBridge* external_data_use_observer_bridge_;
 
-  // Maintains tab sessions and is owned by |this|. It is created on IO thread
-  // but afterwards, should only be accessed on UI thread.
+  // Pointer to the DataUseTabModel in UI thread, Not owned by |this|.
   DataUseTabModel* data_use_tab_model_;
 
+#if defined(OS_ANDROID)
+  // Listens to when Chromium gets backgrounded and submits buffered data use
+  // reports.
+  std::unique_ptr<base::android::ApplicationStatusListener> app_state_listener_;
+#endif
+
   // Time when the currently pending data use report was submitted.
   // |last_data_report_submitted_ticks_| is null if no data use report is
   // currently pending.
@@ -246,25 +206,16 @@ class ExternalDataUseObserver : public data_usage::DataUseAggregator::Observer {
   // is currently pending.
   int64_t pending_report_bytes_;
 
-  // Buffered data reports that need to be submitted to the
-  // |external_data_use_observer_bridge_|.
-  DataUseReports buffered_data_reports_;
-
-  // |ui_task_runner_| is used to call methods on UI thread.
-  scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner_;
-
-  // Time when the data use reports were last received from DataUseAggregator.
+  // Time when the data use reports were last received.
   base::Time previous_report_time_;
 
-  // Time when the matching rules were last fetched.
-  base::TimeTicks last_matching_rules_fetch_time_;
-
   // Total number of bytes transmitted or received across all the buffered
   // reports.
   int64_t total_bytes_buffered_;
 
-  // Duration after which matching rules are periodically fetched.
-  const base::TimeDelta fetch_matching_rules_duration_;
+  // Buffered data reports that need to be submitted to the
+  // |external_data_use_observer_bridge_|.
+  DataUseReports buffered_data_reports_;
 
   // Minimum number of bytes that should be buffered before a data use report is
   // submitted.
@@ -274,24 +225,13 @@ class ExternalDataUseObserver : public data_usage::DataUseAggregator::Observer {
   // duration, it is considered as timed out.
   const base::TimeDelta data_report_submit_timeout_;
 
-#if defined(OS_ANDROID)
-  // Listens to when Chromium gets backgrounded and submits buffered data use
-  // reports.
-  std::unique_ptr<base::android::ApplicationStatusListener> app_state_listener_;
-#endif
-
-  // True if |this| is currently registered as a data use observer.
-  bool registered_as_data_use_observer_;
-
   base::ThreadChecker thread_checker_;
 
-  base::WeakPtrFactory<ExternalDataUseObserver> weak_factory_;
-
-  DISALLOW_COPY_AND_ASSIGN(ExternalDataUseObserver);
+  DISALLOW_COPY_AND_ASSIGN(ExternalDataUseReporter);
 };
 
 }  // namespace android
 
 }  // namespace chrome
 
-#endif  // CHROME_BROWSER_ANDROID_DATA_USAGE_EXTERNAL_DATA_USE_OBSERVER_H_
+#endif  // CHROME_BROWSER_ANDROID_DATA_USAGE_EXTERNAL_DATA_USE_REPORTER_H_