C++ Readability review for holte

components/rappor/rappor_service.h

 // Copyright 2014 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.
 
+// Change for readability
+
 #ifndef COMPONENTS_RAPPOR_RAPPOR_SERVICE_H_
 #define COMPONENTS_RAPPOR_RAPPOR_SERVICE_H_
 
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/prefs/pref_service.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
 #include "components/rappor/log_uploader.h"
 #include "components/rappor/proto/rappor_metric.pb.h"
 #include "components/rappor/rappor_metric.h"
 
 class PrefRegistrySimple;
 
 namespace rappor {
 
 // The type of data stored in a metric.
 enum RapporType {
   ETLD_PLUS_ONE_RAPPOR_TYPE = 0,

[ktl, 2014/03/17 11:22:25]

Comment? Is the zero value significant somewhere? If not, you could leave it
out.

[Steven Holte, 2014/03/18 03:13:07]

Added comments.  The 0 value is used implicitly because the enum values are
indexes into kRapporParametersForType.

   NUM_RAPPOR_TYPES
 };
 
 // This class provides an interface for recording samples for rappor metrics,
 // and periodically generates and uploads reports based on the collected data.
 class RapporService {
  public:
   RapporService();
   virtual ~RapporService();
 
   // Starts the periodic generation of reports and upload attempts.
   void Start(PrefService* pref_service, net::URLRequestContextGetter* context);

[ktl, 2014/03/17 11:22:25]

Does URLRequestContextGetter come from you direct include files? If not, you
should forward-declare it yourself.

[Steven Holte, 2014/03/18 03:13:07]

Done.

 
   // Records a sample of the rappor metric specified by |metric_name|.
   // Creates and initializes the metric, if it doesn't yet exist.
   void RecordSample(const std::string& metric_name,
                     RapporType type,
                     const std::string& sample);
 
   // Sets the cohort value.  For use by tests only.
   void SetCohortForTesting(uint32_t cohort) { cohort_ = cohort; }
 
@@ skipping 37 matching lines @@
   // exist.
   RapporMetric* LookUpMetric(const std::string& metric_name,
                              const RapporParameters& parameters);
 
   // Client-side secret used to generate fake bits.
   std::string secret_;
 
   // The cohort this client is assigned to. -1 is uninitialized.
   int32_t cohort_;
 
   // Timer which schedules calls to OnLogInterval()

[ktl, 2014/03/17 11:22:25]

Add period.

[Steven Holte, 2014/03/18 03:13:07]

Done.

   base::OneShotTimer<RapporService> log_rotation_timer_;
 
   // A private LogUploader instance for sending reports to the server.
   scoped_ptr<LogUploader> uploader_;

[ktl, 2014/03/17 11:22:25]

Do you still use scoped_ptr<> instead of more standard unique_ptr<>? In any
case, please include the appropriate header.

[Ilya Sherman, 2014/03/17 19:14:36]

Chromium code does not yet have unique_ptr support, due to needing to support a
wide variety of older compiler toolchains.  

[Steven Holte, 2014/03/18 03:13:07]

Added include.

 
   // We keep all registered metrics in a map, from name to metric.
   // The map owns the metrics it contains.
   std::map<std::string, RapporMetric*> metrics_map_;

[ktl, 2014/03/17 11:22:25]

#include <map>

In modern C++, it would be preferable to codify the ownership and write

std::map<std::string, std::unique_ptr<RapporMetric>>

instead. If your standard library supports that, you could try it instead. (Then
no STLDeleteValues will be needed, of course.)

[Ilya Sherman, 2014/03/17 19:14:36]

Unfortunately, Chromium code cannot yet use C++11.

 
   DISALLOW_COPY_AND_ASSIGN(RapporService);
 };
 
 }  // namespace rappor
 
 #endif  // COMPONENTS_RAPPOR_RAPPOR_SERVICE_H_