NQE: Cleanup observation buffer code

net/nqe/observation_buffer.h

 // 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 NET_NQE_OBSERVATION_BUFFER_H_
 #define NET_NQE_OBSERVATION_BUFFER_H_
 
 #include <float.h>
 
 #include <algorithm>
 #include <deque>
+#include <memory>
 #include <vector>
 
-#include "base/gtest_prod_util.h"
 #include "base/macros.h"
+#include "base/time/default_tick_clock.h"

[RyanSturm, 2016/09/26 18:44:55]

This pulls in base/time/tick_clock.h, but you should probably include it as well
due to the unique_ptr declared below.

[tbansal1, 2016/09/28 16:42:31]

Done.

 #include "base/time/time.h"
 #include "net/base/net_export.h"
+#include "net/nqe/network_quality_observation.h"

[RyanSturm, 2016/09/26 18:44:55]

Is network_quality_observation.h needed?

[tbansal1, 2016/09/28 16:42:31]

Yes. See Line 174:
std::deque<Observation<ValueType>> observations_;

 #include "net/nqe/network_quality_observation_source.h"
 #include "net/nqe/weighted_observation.h"
 
 namespace net {
 
 namespace nqe {
 
 namespace internal {
 
 // Stores observations sorted by time.
 template <typename ValueType>
 class NET_EXPORT_PRIVATE ObservationBuffer {
  public:
   explicit ObservationBuffer(double weight_multiplier_per_second)
-      : weight_multiplier_per_second_(weight_multiplier_per_second) {
+      : weight_multiplier_per_second_(weight_multiplier_per_second),
+        tick_clock_(new base::DefaultTickClock()) {
     static_assert(kMaximumObservationsBufferSize > 0U,
                   "Minimum size of observation buffer must be > 0");
     DCHECK_GE(weight_multiplier_per_second_, 0.0);
     DCHECK_LE(weight_multiplier_per_second_, 1.0);
   }
 
   ~ObservationBuffer() {}
 
   // Adds |observation| to the buffer. The oldest observation in the buffer
   // will be evicted to make room if the buffer is already full.
@@ skipping 21 matching lines @@
   void Clear() { observations_.clear(); }
 
   // Returns true iff the |percentile| value of the observations in this
   // buffer is available. Sets |result| to the computed |percentile|
   // value among all observations since |begin_timestamp|. If the value is
   // unavailable, false is returned and |result| is not modified. Percentile
   // value is unavailable if all the values in observation buffer are older
   // than |begin_timestamp|.
   // |result| must not be null.
   bool GetPercentile(const base::TimeTicks& begin_timestamp,
                      ValueType* result,

[RyanSturm, 2016/09/26 18:44:55]

nit: Would you mind moving result to be the last parameter if it is an out
param?

Only make this change if you fix all the out-params in this file.

[tbansal1, 2016/09/28 16:42:31]

Added TODO.

                      int percentile,
                      const std::vector<NetworkQualityObservationSource>&
                          disallowed_observation_sources) const {
     DCHECK(result);
     DCHECK_GE(Capacity(), Size());
     // Stores WeightedObservation in increasing order of value.
     std::vector<WeightedObservation<ValueType>> weighted_observations;
 
     // Total weight of all observations in |weighted_observations|.
     double total_weight = 0.0;
@@ skipping 24 matching lines @@
 
     // Computation may reach here due to floating point errors. This may happen
     // if |percentile| was 100 (or close to 100), and |desired_weight| was
     // slightly larger than |total_weight| (due to floating point errors).
     // In this case, we return the highest |value| among all observations.
     // This is same as value of the last observation in the sorted vector.
     *result = weighted_observations.at(weighted_observations.size() - 1).value;
     return true;
   }
 
+  void SetTickClockForTesting(std::unique_ptr<base::TickClock> tick_clock) {

[RyanSturm, 2016/09/26 18:44:55]

nit: Maybe add tick_clock as a parameter to the constructor. I prefer to avoid
ForTesting methods when possible.

[tbansal1, 2016/09/28 16:42:31]

Seems like net tends to use ForTesting() rather than passing it in constructor:
https://cs.chromium.org/search/?q=f:net+clock.*testing+-f:nqe&sq=package:chro...

+    tick_clock_ = std::move(tick_clock);
+  }
+
  private:
   // Maximum number of observations that can be held in the ObservationBuffer.
   static const size_t kMaximumObservationsBufferSize = 300;
 
   // Computes the weighted observations and stores them in
   // |weighted_observations| sorted by ascending |WeightedObservation.value|.
   // Only the observations with timestamp later than |begin_timestamp| are
   // considered. Also, sets |total_weight| to the total weight of all
   // observations. Should be called only when there is at least one
   // observation in the buffer.
   void ComputeWeightedObservations(
       const base::TimeTicks& begin_timestamp,
       std::vector<WeightedObservation<ValueType>>& weighted_observations,
       double* total_weight,
       const std::vector<NetworkQualityObservationSource>&
           disallowed_observation_sources) const {
     DCHECK_GE(Capacity(), Size());
 
     weighted_observations.clear();
     double total_weight_observations = 0.0;
-    base::TimeTicks now = base::TimeTicks::Now();
+    base::TimeTicks now = tick_clock_->NowTicks();
 
     for (const auto& observation : observations_) {
       if (observation.timestamp < begin_timestamp)
         continue;
       bool disallowed = false;
       for (const auto& disallowed_source : disallowed_observation_sources) {
         if (disallowed_source == observation.source)
           disallowed = true;
       }
       if (disallowed)
@@ skipping 17 matching lines @@
   // front of the queue.
   std::deque<Observation<ValueType>> observations_;
 
   // The factor by which the weight of an observation reduces every second.
   // For example, if an observation is 6 seconds old, its weight would be:
   //     weight_multiplier_per_second_ ^ 6
   // Calculated from |kHalfLifeSeconds| by solving the following equation:
   //     weight_multiplier_per_second_ ^ kHalfLifeSeconds = 0.5
   const double weight_multiplier_per_second_;
 
+  std::unique_ptr<base::TickClock> tick_clock_;
+
   DISALLOW_COPY_AND_ASSIGN(ObservationBuffer);
 };
 
 }  // namespace internal
 
 }  // namespace nqe
 
 }  // namespace net
 
 #endif  // NET_NQE_OBSERVATION_BUFFER_H_