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

chrome/browser/metrics/variations_service.cc

Index: chrome/browser/metrics/variations_service.cc
diff --git a/chrome/browser/metrics/variations_service.cc b/chrome/browser/metrics/variations_service.cc
new file mode 100644
index 0000000000000000000000000000000000000000..d93c8b19fdd7311407cd1257902fe1552c1fa5b8
--- /dev/null
+++ b/chrome/browser/metrics/variations_service.cc
@@ -0,0 +1,71 @@
+// 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.
+
+#include "chrome/browser/metrics/variations_service.h"
+
+#include "base/base64.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/singleton.h"

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

Is this used here?

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

Oops, no, not any more.
Fixed
On 2012/05/03 04:02:52, SteveT wrote:

+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/metrics/proto/trials_seed.pb.h"
+#include "chrome/browser/prefs/pref_service.h"
+#include "chrome/common/pref_names.h"
+#include "content/public/common/url_fetcher.h"
+#include "googleurl/src/gurl.h"
+#include "net/base/escape.h"

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

Is this used here?

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

"Oh god how did this get in here I am not good with computers"
http://goo.gl/66qWS
Fixed
On 2012/05/03 04:02:52, SteveT wrote:

+#include "net/base/load_flags.h"
+#include "net/url_request/url_request_status.h"
+
+const char* VariationsService::kDefaultVariationsServer =
+    "https://clients4.google.com/chrome-variations/seed";
+
+void VariationsService::LoadVariationsSeed(PrefService* local_prefs) {
+  std::string base64_seed_data = local_prefs->GetString(prefs::kVariationsSeed);
+  std::string seed_data;
+  base::Base64Decode(base64_seed_data, &seed_data);

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

What if this fails?

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

I've decided to clear the pref if either this or the parse fails. 
On 2012/05/03 04:02:52, SteveT wrote:

+  variations_seed_.ParseFromString(seed_data);
+}
+
+void VariationsService::FetchVariationsSeed() {
+  seed_request_pending_.reset(content::URLFetcher::Create(
+      GURL(kDefaultVariationsServer), content::URLFetcher::GET, this));
+  seed_request_pending_->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
+                                      net::LOAD_DO_NOT_SAVE_COOKIES);
+  seed_request_pending_->SetRequestContext(
+      g_browser_process->system_request_context());
+  seed_request_pending_->SetMaxRetries(5);
+  seed_request_pending_->Start();
+}
+
+void VariationsService::OnURLFetchComplete(const content::URLFetcher* source) {
+  bool error =
+      (source->GetStatus().status() != net::URLRequestStatus::SUCCESS ||
+       source->GetResponseCode() != 200);

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

nit: No need to cache this in a variable name, as it's used in only one place.

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

Done.

+  scoped_ptr<const content::URLFetcher> delete_ptr(
+      seed_request_pending_.release());

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

nit: Please add a comment explaining why we cannot immediately free the pending
request (assuming that's true -- otherwise just go ahead and reset() it here). 
This variable name could also be clearer -- perhaps something like
|current_request_|?

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

Ok, I've made this clearer. I have one question about it, but I'll leave a
comment in the next patch set.

+  if (error) {

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

nit: You can add an early "return;" stmt here, and consequently reduce the
indentation of the subsequent block.

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

Done.

+  } else {
+    std::string seed_data;
+    source->GetResponseAsString(&seed_data);
+
+    StoreSeedData(seed_data, g_browser_process->local_state());
+  }
+}
+
+void VariationsService::StoreSeedData(

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

hyper nit: if "std::string seed_data," fits on this line, move it back up, then
align "PrefService* local_prefs) {" to it's left hand side.

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

Done.

+    std::string seed_data, PrefService* local_prefs) {
+  // Only store the seed data if it parses correctly

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

ultra nit: correctly -> correctly.

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

Done.

+  chrome_variations::TrialsSeed seed;
+  if (seed.ParseFromString(seed_data)){
+    std::string base64_seed_data;
+    if (!base::Base64Encode(seed_data, &base64_seed_data)) {

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

nit: braces not needed for a single statement if body

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

Done.

+      return;
+    }
+    local_prefs->SetString(prefs::kVariationsSeed, base64_seed_data);
+  }
+}

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

Break here.

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

Done.

+// static
+void VariationsService::RegisterPrefs(PrefService* prefs) {
+  prefs->RegisterStringPref(prefs::kVariationsSeed, std::string());
+}