Retrieving Chrome Variations seed from server and storing in local prefs. Loading seed data from lo…

chrome/browser/metrics/variations_service.h

+// Copyright (c) 2012 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_METRICS_VARIATIONS_SERVICE_H_
+#define CHROME_BROWSER_METRICS_VARIATIONS_SERVICE_H_
+#pragma once
+
+#include "base/compiler_specific.h"
+#include "base/memory/scoped_ptr.h"
+#include "chrome/browser/metrics/proto/trials_seed.pb.h"
+#include "content/public/common/url_fetcher_delegate.h"
+
+class PrefService;
+
+class VariationsService : public content::URLFetcherDelegate {
+ public:
+

[SteveT, 2012/05/03 04:02:52]

nit: extra break here not needed

[jwd, 2012/05/03 22:27:49]

fixed
On 2012/05/03 04:02:52, SteveT wrote:

+  VariationsService() {}
+

[SteveT, 2012/05/03 04:02:52]

nit: extra break here not needed

[jwd, 2012/05/03 22:27:49]

fixed
On 2012/05/03 04:02:52, SteveT wrote:

+  virtual ~VariationsService() {}

[Ilya Sherman, 2012/05/02 23:15:08]

nit: It's a violation of the style guide to inline the constructor and
destructor, since the class includes a non-trivial data member (the
|variations_seed_|).

[jwd, 2012/05/03 22:27:49]

fixed
On 2012/05/02 23:15:08, Ilya Sherman wrote:

+

[SteveT, 2012/05/03 04:02:52]

Methods below can probably be alphabetized (and in cc). Except for the overriden
one. You can just have that at the very top (or bottom) of the public: section.

+  // Loads the Variations seed data from the given local prefs.
+  void LoadVariationsSeed(PrefService* local_prefs);
+
+  void FetchVariationsSeed();

[SteveT, 2012/05/03 04:02:52]

nit: Maybe StartFetchingVariationsSeed is a better name.

[SteveT, 2012/05/03 04:02:52]

Document, please. Remember to point out where the fetch returns the data (ie -
in OnURLFetchComplete).

[jwd, 2012/05/03 22:27:49]

Agreed, fixed
On 2012/05/03 04:02:52, SteveT wrote:

[jwd, 2012/05/03 22:27:49]

Fixed
On 2012/05/03 04:02:52, SteveT wrote:

+
+  // content::URLFetcherDelegate implementation:
+  virtual void OnURLFetchComplete(const content::URLFetcher* source) OVERRIDE;
+
+  // Store the given seed data to the given local prefs. Note that |seed_data|
+  // is assumed to be the raw data stored in a string. It will Base64Encoded for

[SteveT, 2012/05/03 04:02:52]

nit: assumed to be the raw *what* data? Raw protobuf seralized data?

[jwd, 2012/05/03 22:27:49]

Fixed.
On 2012/05/03 04:02:52, SteveT wrote:

+  // storage. If the string is invalid or the encoding fails, the |local_prefs|
+  // is left as is.
+  void StoreSeedData(std::string seed_data, PrefService* local_prefs);

[SteveT, 2012/05/03 04:02:52]

I think you can make seed_data a const ref.

[SteveT, 2012/05/03 04:02:52]

I think StoreSeedData can probably be private?

[jwd, 2012/05/03 22:27:49]

Fixed
On 2012/05/03 04:02:52, SteveT wrote:

[jwd, 2012/05/03 22:27:49]

Fixed
On 2012/05/03 04:02:52, SteveT wrote:

+
+  // Register Variations related prefs in Lacal State.

[SteveT, 2012/05/03 04:02:52]

sp: Lacal -> Local

[jwd, 2012/05/03 22:27:49]

Fixed
On 2012/05/03 04:02:52, SteveT wrote:

+  static void RegisterPrefs(PrefService* prefs);
+
+  // Default server of Variations seed info.
+  static const char* kDefaultVariationsServer;

[Ilya Sherman, 2012/05/02 23:15:08]

nit: This should probably be "const char kDefaultVariationsServer[]"

[Ilya Sherman, 2012/05/02 23:15:08]

nit: Does this need to be exposed in the header file?

[SteveT, 2012/05/03 04:02:52]

Currently this doesn't need to be publically exposed as part of this class, so
you can just stick it in an unnamed-namespace in the cc instead.

[jwd, 2012/05/03 22:27:49]

Fixed
On 2012/05/03 04:02:52, SteveT wrote:

+
+ private:
+

[SteveT, 2012/05/03 04:02:52]

nit: extra break here not needed

[jwd, 2012/05/03 22:27:49]

Fixed
On 2012/05/03 04:02:52, SteveT wrote:

+  scoped_ptr<content::URLFetcher> seed_request_pending_;

[Ilya Sherman, 2012/05/02 23:15:08]

nit: The current name sounds like it is describing a boolean.  Perhaps
"pending_seed_request_"?

[SteveT, 2012/05/03 04:02:52]

Please document.

[jwd, 2012/05/03 22:27:49]

Agreed, fixed.
On 2012/05/02 23:15:08, Ilya Sherman wrote:

[jwd, 2012/05/03 22:27:49]

Fixed
On 2012/05/03 04:02:52, SteveT wrote:

+  // The variations seed data being used for this session.
+  chrome_variations::TrialsSeed variations_seed_;
+};
+
+#endif  // CHROME_BROWSER_METRICS_VARIATIONS_SERVICE_H_