RAPPOR implementation

components/rappor/rappor_service.h

+// Copyright (c) 2013 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 COMPONENTS_RAPPOR_RAPPOR_SERVICE_H_
+#define COMPONENTS_RAPPOR_RAPPOR_SERVICE_H_
+
+#include <string>
+#include <vector>
+
+#include "base/basictypes.h"
+#include "base/lazy_instance.h"
+#include "base/memory/weak_ptr.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"
+#include "url/gurl.h"
+
+class PrefRegistrySimple;
+
+namespace rappor {
+
+class RapporReporter;

[Ilya Sherman, 2014/01/10 11:00:32]

Hmm, where is this class defined?  I don't recall seeing an implementation for
it.

[Steven Holte, 2014/01/14 00:47:54]

Removed.

+
+// This class provides base functionality for logging rappor data.

[Ilya Sherman, 2014/01/10 11:00:32]

nit: What does the "base" in "base functionality" refer to?

[Steven Holte, 2014/01/14 00:47:54]

Rewrote comment.

+class RapporService {
+ public:
+  RapporService();
+  virtual ~RapporService();
+
+  // Starts the rappord system.  Should be called when starting up.

[Ilya Sherman, 2014/01/10 11:00:32]

nit: "rappord" -> "rappor"?

[Steven Holte, 2014/01/14 00:47:54]

Rewrote comment.

+  void Start(PrefService* pref_service, net::URLRequestContextGetter* context);
+
+  // Registers the names of all of the Preferences used by RapporService in the

[Ilya Sherman, 2014/01/10 11:00:32]

nit: No need to capitalize "Preferences" here, I think.

[Steven Holte, 2014/01/14 00:47:54]

Done.

+  // provided PrefRegistry. This should be called before calling Start().
+  static void RegisterPrefs(PrefRegistrySimple* registry);
+
+  // Records a set of samples on the rappor metric specified by |parameters|.
+  // Creates and initializes the metric, if it doesn't yet exist.
+  void RecordSamples(const RapporParameters& parameters,
+                     const std::vector<std::string>& samples);
+
+  // Utility method for recording a URL.  Breaks the URL up into parts and
+  // records samples for each of them.
+  void RecordUrl(const RapporParameters& parameters, const GURL& url);
+
+ private:
+  friend class RapporServiceTest;

[Ilya Sherman, 2014/01/10 11:00:32]

Please don't add friend classes to new code, unless there's something really
unusual going on.  Friending a class gives it too much permission to muck about
with internal state.  Instead, expose the needed functionality via a set of
protected methods that a test-specific subclass can access or mock out.

[Steven Holte, 2014/01/15 04:53:44]

Done.

+
+  // Generates a rappor_secret and stores it in preferences.  If already stored,

[Ilya Sherman, 2014/01/10 11:00:32]

nit: "rappor_secret" isn't a variable name defined anywhere close to hear (that
I'm spotting, at least), so probably best to remove the underscore.

[Steven Holte, 2014/01/15 04:53:44]

This referenced rappor_secret_, now references secret_.

+  // just retreives the stored value.

[Ilya Sherman, 2014/01/10 11:00:32]

Is it important that a client's rappor secret remains constant for a long period
of time?  (I think it is, if I correctly understand the purpose of the secret.) 
If so, please document that either here or in the implementation file.

[Steven Holte, 2014/01/15 04:53:44]

Rewrote this comment a bit.

+  void GenerateRapporSecret(PrefService* pref_service);
+
+  // Logs a single RapporMetric value to the rappor_metrics_proto_.
+  void LogRappor(const RapporMetric& rappor);

[Ilya Sherman, 2014/01/10 11:00:32]

nit: "LogRappor" -> "LogMetric"; "rappor" -> "metric"

[Steven Holte, 2014/01/15 04:53:44]

Done.

+
+  // Logs all of the collected RapporMetric metrics to the rappor_metric_proto_.
+  void LogRapporMetrics();
+
+  // Called whenever the logging interval elapses.

[Ilya Sherman, 2014/01/10 11:00:32]

What does the method do when it's called?  Please document.

[Steven Holte, 2014/01/15 04:53:44]

Done.

+  void OnLogInterval();
+
+  // Find a rappor metric by name, and create it if it doesn't already exist.

[Ilya Sherman, 2014/01/10 11:00:32]

nit: "and create it" -> "creating it"

[Steven Holte, 2014/01/15 04:53:44]

Done.

+  RapporMetric* GetRapporMetric(const RapporParameters& parameters);
+
+  // Client side secret used to generate fake bits.

[Ilya Sherman, 2014/01/10 11:00:32]

nit: "Client side" -> "Client-side"

[Steven Holte, 2014/01/15 04:53:44]

Done.

+  std::string rappor_secret_;

[Ilya Sherman, 2014/01/10 11:00:32]

nit: Probably no need for the "rappor_" prefix here or for any of the other
variable names, unless you're trying to distinguish multiple secrets within this
class.

[Steven Holte, 2014/01/15 04:53:44]

Done.

+
+  RapporMetricsProto rappor_metrics_proto_;

[Ilya Sherman, 2014/01/10 11:00:32]

nit: Docs, please.

[Steven Holte, 2014/01/15 04:53:44]

Removed

+
+  // Timer which schedules calls OnLogInterval()

[Ilya Sherman, 2014/01/10 11:00:32]

nit: "calls" -> "calls to"

[Steven Holte, 2014/01/15 04:53:44]

Done.

+  base::OneShotTimer<RapporService> log_rotation_timer_;
+
+  LogUploader* uploader_;

[Ilya Sherman, 2014/01/10 11:00:32]

nit: Docs.  Also, who owns the memory for this object?

[Steven Holte, 2014/01/15 04:53:44]

Changed to scoped_ptr.  Added docstring.

+
+  // We keep all registered histograms in a map, from name to histogram.
+  typedef std::map<std::string, RapporMetric*> RapporMap;

[Ilya Sherman, 2014/01/10 11:00:32]

Optional nit: This is definitely a personal style preference, but I'd prefer
that you not introduce this typedef, and just spell out the full type when it is
used.  IMO that is clearer, as it requires less cross-referencing.

[Steven Holte, 2014/01/15 04:53:44]

Done.

+  RapporMap rappors_;

[Ilya Sherman, 2014/01/10 11:00:32]

nit: metrics_;

[Steven Holte, 2014/01/15 04:53:44]

Changed to metrics_map_

+
+  // Lock protects access to above map.
+  base::Lock lock_;

[Ilya Sherman, 2014/01/10 11:00:32]

Why does the map need a lock?  Is this class thread-safe?  Why?  I'd strongly
encourage you to pick one thread and stick to it, unless there's a quite
compelling reason not to do so.

[Steven Holte, 2014/01/15 04:53:44]

I would expect all of the upload/report generation functions to be called on the
same thread, but that the public RecordSample() method might be called from a
different thread.  I'm not familiar enough with how threads get used in Chrome
to know whether that's likely.

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