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

chrome/browser/metrics/variations_service.cc

+// 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 "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 23:46:24]

Is it still here?

[jwd, 2012/05/04 19:33:06]

I told you, I'm not good with computers. It should be fixed now. 

+#include "net/base/load_flags.h"
+#include "net/url_request/url_request_status.h"
+
+namespace {
+// Default server of Variations seed info.
+const char kDefaultVariationsServer[] =
+    "https://clients4.google.com/chrome-variations/seed";
+};

[SteveT, 2012/05/03 23:46:24]

No semicolons after namespace closing braces, methinks.

[jwd, 2012/05/04 19:33:06]

Done.

+
+VariationsService::VariationsService() {}
+VariationsService::~VariationsService() {}
+
+void VariationsService::LoadVariationsSeed(PrefService* local_prefs) {
+  std::string base64_seed_data = local_prefs->GetString(prefs::kVariationsSeed);
+  std::string seed_data;
+
+  // If the decode process fails, assume the pref value is corrupt, and clear
+  // it.

[SteveT, 2012/05/03 23:46:24]

With that in mind, it's actually kind of weird that it could get into prefs
corrupted in the first place, if your code

But it's good to have this check anyway. This makes it harder for malicious
doo-dads from changing a user's Local State prefs file on disk and adding some
badness here.

+  if (!(base::Base64Decode(base64_seed_data, &seed_data) &&

[Alexei Svitkine (slow), 2012/05/04 03:09:53]

Shouldn't you check for empty string first, since that's what you store
initially? (Avoids trying to base64 decode it, which probably would succeed, and
then trying to parse an empty string as a proto, which probably fails).

[jwd, 2012/05/04 19:33:06]

The empty string does parse. I figured it was because the only members of
TrialsSeed are optional or repeated. I didn't think short circuiting out for an
empty string was worth it the check.

+        variations_seed_.ParseFromString(seed_data))) {

[Alexei Svitkine (slow), 2012/05/04 03:09:53]

Nit: Change if (!(a && b)) to if (!a || !b). 

[jwd, 2012/05/04 19:33:06]

Done.

+    local_prefs->ClearPref(prefs::kVariationsSeed);

[MAD, 2012/05/04 12:29:57]

I would add a DCHECK here or at least a VLOG(1)...

[jwd, 2012/05/04 19:33:06]

Done.

+  }
+}
+
+void VariationsService::StartFetchingVariationsSeed() {
+  pending_seed_request_.reset(content::URLFetcher::Create(
+      GURL(kDefaultVariationsServer), content::URLFetcher::GET, this));
+  pending_seed_request_->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
+                                      net::LOAD_DO_NOT_SAVE_COOKIES);
+  pending_seed_request_->SetRequestContext(
+      g_browser_process->system_request_context());
+  pending_seed_request_->SetMaxRetries(5);
+  pending_seed_request_->Start();
+}
+
+void VariationsService::OnURLFetchComplete(const content::URLFetcher* source) {
+  if (pending_seed_request_.get() == source) {
+    // When we're done handling the request, the fetcher will be deleted.
+    scoped_ptr<const content::URLFetcher> current_request(

[Alexei Svitkine (slow), 2012/05/04 03:09:53]

Nit: rename to |request|.

Then you can have less wrapping below!

[jwd, 2012/05/04 19:33:06]

Done.

+        pending_seed_request_.release());
+    if (current_request->GetStatus().status() !=
+        net::URLRequestStatus::SUCCESS ||
+        current_request->GetResponseCode() != 200)
+      return;
+
+    std::string seed_data;
+    current_request->GetResponseAsString(&seed_data);
+
+    StoreSeedData(seed_data, g_browser_process->local_state());
+  }

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

I'm not sure if this function should be responsible for |source| if the equality
wasn't correct. TranslationManager deals with it here
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/translate/t....
But I haven't yet found another example of it (haven't looked VERY thoroughly).

[MAD, 2012/05/04 20:53:12]

You could add a DCHECK that you received the one you are expecting. There are
other cases where they handle it that way... Others simply don't check and
assume it's theirs...

[jwd, 2012/05/04 22:11:23]

Done.

+}
+
+// static
+void VariationsService::RegisterPrefs(PrefService* prefs) {
+  prefs->RegisterStringPref(prefs::kVariationsSeed, std::string());
+}
+
+void VariationsService::StoreSeedData(const std::string& seed_data,
+                                      PrefService* local_prefs) {
+  // Only store the seed data if it parses correctly.
+  chrome_variations::TrialsSeed seed;
+  if (seed.ParseFromString(seed_data)){

[Alexei Svitkine (slow), 2012/05/04 03:09:53]

Nit: space before {

[Alexei Svitkine (slow), 2012/05/04 03:09:53]

Nit: do an early return here too instead of a nested if, for consistency. Or
alternative a single if with those two conditions.

[jwd, 2012/05/04 19:33:06]

Done.

+    std::string base64_seed_data;
+    if (!base::Base64Encode(seed_data, &base64_seed_data))
+      return;

[MAD, 2012/05/04 12:29:57]

Please add VLOG(1) where we fail to do save...

[jwd, 2012/05/04 19:33:06]

Done.

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