X-Chrome-Variations logic refactoring

chrome/browser/chrome_metrics_helper.h

Index: chrome/browser/chrome_metrics_helper.h
diff --git a/chrome/browser/chrome_metrics_helper.h b/chrome/browser/chrome_metrics_helper.h
new file mode 100644
index 0000000000000000000000000000000000000000..45a43be555eae0be9f822356cd05ab52a74dc196
--- /dev/null
+++ b/chrome/browser/chrome_metrics_helper.h
@@ -0,0 +1,89 @@
+// 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_CHROME_METRICS_HELPER_H_
+#define CHROME_BROWSER_CHROME_METRICS_HELPER_H_
+
+#include <set>
+#include <string>
+
+#include "base/basictypes.h"
+#include "base/metrics/field_trial.h"
+#include "base/synchronization/lock.h"
+#include "chrome/common/metrics/variations/variation_ids.h"
+
+namespace content {
+class ResourceContext;
+}
+
+namespace net {
+class HttpRequestHeaders;
+class URLFetcher;
+class URLRequest;
+}
+
+class GURL;
+class Profile;
+class ProfileIOData;
+
+template <typename T> struct DefaultSingletonTraits;
+
+// This class is a thread-safe singleton.
+class ChromeMetricsHelper : base::FieldTrialList::Observer {
+ public:
+  static ChromeMetricsHelper* GetInstance();
+
+  // Adds Chrome experiment and metrics state as custom headers to |request|.
+  // This is a best-effort attempt, and does not interrupt the request if the
+  // headers cannot be appended.
+  void AppendHeadersToRequest(net::URLRequest* request,
+                              content::ResourceContext* resource_context);
+  // Same as above but works with a |fetcher| and |profile|.  Both methods
+  // internally delegate to AppendHeaders(...).
+  void AppendHeadersToFetcher(net::URLFetcher* fetcher, const Profile* profile);
+
+  // base::FieldTrialList::Observer implementation.

[Peter Kasting, 2012/12/11 21:08:48]

Nit: If this isn't called directly through a ChromeMetricsHelper*, can you make
it private?  This will keep the public API for the class clearer.

[Bart N., 2012/12/11 23:18:56]

Sure.

+  // This will add the variation ID associated with |trial_name| and
+  // |group_name| to the variation ID cache.
+  virtual void OnFieldTrialGroupFinalized(
+      const std::string& trial_name,
+      const std::string& group_name) OVERRIDE;
+
+ private:
+  ChromeMetricsHelper();
+  virtual ~ChromeMetricsHelper();
+  friend struct DefaultSingletonTraits<ChromeMetricsHelper>;

[Peter Kasting, 2012/12/11 21:08:48]

Nit: Put friend declaration above constructor/destructor, then a blank line.

[Bart N., 2012/12/11 23:18:56]

Done.

+
+  void AppendHeaders(const GURL& url,

[Peter Kasting, 2012/12/11 21:08:48]

Nit: Comment?

[Bart N., 2012/12/14 18:21:42]

Done.

+                     bool incognito,
+                     bool uma_enabled,
+                     net::HttpRequestHeaders* headers);
+
+  // Prepares the variation IDs cache with initial values if not already done.
+  // This method also registers the caller with the FieldTrialList to receive
+  // new variation IDs.
+  void InitVariationIDsCacheIfNeeded();
+
+  // Takes whatever is currently in |variation_ids_set_| and recreates
+  // |variation_ids_header_| with it.
+  void UpdateVariationIDsHeaderValue();
+
+  // Guards |variation_ids_cache_initialized_|, |variation_ids_set_| and
+  // |variation_ids_header_|.
+  base::Lock lock_;
+
+  // Whether or not we've initialized the Cache.

[Peter Kasting, 2012/12/11 21:08:48]

Nit: Cache -> cache

[Bart N., 2012/12/11 23:18:56]

Done.

+  bool variation_ids_cache_initialized_;
+
+  // Keep a cache of variation IDs that are transmitted in headers to Google.
+  // This consists of a list of valid IDs, and the actual transmitted header.
+  // Note that since this cache is both initialized and accessed from the IO
+  // thread, we do not need to synchronize its uses.

[Peter Kasting, 2012/12/11 21:08:48]

I'm confused as to what this comment means.  It sounds like it's saying "all
access to these variables is on the same thread and thus unlocked" but the
comments on the lock say that we use it to protect these.  ??

[Bart N., 2012/12/11 23:18:56]

Ah, most of the code in this file is simply moved (no changes) from the existing
implementation. I forgot to update this comment, which is no longer true.

Done.

+  std::set<chrome_variations::VariationID> variation_ids_set_;
+  std::string variation_ids_header_;
+
+  DISALLOW_COPY_AND_ASSIGN(ChromeMetricsHelper);
+};
+
+#endif  // CHROME_BROWSER_CHROME_METRICS_HELPER_H_