Add filtering logic for client-side variations.

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
index 6f547f0c67eec049f39f8dadcaf2e91b207e9558..cac2c216df8dd6a67ba7f07c874c7d28ffbd08ae 100644
--- a/chrome/browser/metrics/variations_service.cc
+++ b/chrome/browser/metrics/variations_service.cc
@@ -5,6 +5,8 @@
 #include "chrome/browser/metrics/variations_service.h"
 
 #include "base/base64.h"
+#include "base/build_time.h"
+#include "base/version.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/singleton.h"
 #include "chrome/browser/browser_process.h"
@@ -22,7 +24,23 @@ namespace {
 const char kDefaultVariationsServer[] =
     "https://clients4.google.com/chrome-variations/seed";
 
-}// namespace
+// Maps chrome_variations::Study_Channel enum values to corresponding
+// chrome::VersionInfo::Channel enum values.
+chrome::VersionInfo::Channel ConvertStudyChannelToVersionChannel(
+    chrome_variations::Study_Channel study_channel) {
+  switch (study_channel) {
+    case chrome_variations::Study_Channel_CANARY:
+      return chrome::VersionInfo::CHANNEL_CANARY;
+    case chrome_variations::Study_Channel_DEV:
+      return chrome::VersionInfo::CHANNEL_DEV;
+    case chrome_variations::Study_Channel_BETA:
+      return chrome::VersionInfo::CHANNEL_BETA;
+    case chrome_variations::Study_Channel_STABLE:
+      return chrome::VersionInfo::CHANNEL_STABLE;
+  }

[MAD, 2012/05/05 13:51:17]

DCHECK on default please

And didn't you get:
warning C4715: 'ConvertStudyChannelToVersionChannel' : not all control paths
return a value
???

[Alexei Svitkine (slow), 2012/05/05 15:14:37]

It's fine with clang on the Mac. I believe clang knows that all possible
|study_channel| enum values are handled so: a) the function will always reach a
return and b) the default case is not necessarily.

I think there also exists a warning where you don't handle some enum values in a
switch that doesn't have a default case. I'm not sure if we build with that
warning enabled, though.

But you're right that this may not compile on Windows or possibly even with gcc
- I haven't tested since this patch depends on Jesse's patch and I'm not sure
there's a way to run try bots on this before Jesse's patch has landed. Is there?

Regardless, it's probably a good a idea to add DCHECK and return below, for
portability with compilers that aren't as smart as clang.

[Alexei Svitkine (slow), 2012/05/07 15:49:52]

Done.

+}
+
+}  // namespace
 
 // Static
 VariationsService* VariationsService::GetInstance() {
@@ -95,3 +113,82 @@ void VariationsService::StoreSeedData(const std::string& seed_data,
 }
 
 VariationsService::VariationsService() {}
+
+// static
+bool VariationsService::ShouldAddStudy(const chrome_variations::Study& study) {
+  const chrome::VersionInfo current_version_info;
+  if (!current_version_info.is_valid())
+    return false;
+
+  if (!CheckStudyChannel(study, chrome::VersionInfo::GetChannel()))
+    return false;
+
+  if (!CheckStudyVersion(study, current_version_info.Version()))
+    return false;
+
+  if (!CheckStudyDate(study, base::GetBuildTime()))

[MAD, 2012/05/05 13:51:17]

Please add a comment explaining why we use build time, e.g., to do as the
FieldTrials do, and avoid relying on system time that users might change.

[Alexei Svitkine (slow), 2012/05/07 15:49:52]

Done.

+    return false;
+
+  return true;

[MAD, 2012/05/05 13:51:17]

I wonder if we should add DVLOG(1)s when we filter out an experiment. What do
you think?

[Alexei Svitkine (slow), 2012/05/07 15:49:52]

Done.

+}
+
+// static
+bool VariationsService::CheckStudyChannel(
+    const chrome_variations::Study& study,
+    chrome::VersionInfo::Channel channel) {
+  for (int i = 0; i < study.channel_size(); i++) {
+    if (ConvertStudyChannelToVersionChannel(study.channel(i)) == channel)
+      return true;
+  }
+  return false;
+}
+
+// static
+bool VariationsService::CheckStudyVersion(const chrome_variations::Study& study,
+                                          const std::string& version_string) {
+  const Version current_version(version_string);
+  if (!current_version.IsValid()) {
+    DCHECK(false);

[MAD, 2012/05/05 13:51:17]

We try not to DCHECK AND handle the error usually... Unless we have a very good
reason to.

[Alexei Svitkine (slow), 2012/05/05 15:14:37]

I copied this from elsewhere. I think the reasoning is that the version string
passed in the parameter (which comes from Chrome's version and not from the
server) should always be valid. I think that's a reasonable thing to DCHECK()
for.

+    return false;
+  }
+
+  if (study.has_min_version()) {
+    const Version min_version(study.min_version());
+    if (!min_version.IsValid())
+      return false;
+    if (current_version.CompareTo(min_version) < 0)
+      return false;
+  }
+
+  if (study.has_max_version()) {
+    const Version max_version(study.max_version());
+    if (!max_version.IsValid())
+      return false;
+    if (current_version.CompareTo(max_version) > 0)
+      return false;
+  }
+
+  return true;
+}
+
+// static
+bool VariationsService::CheckStudyDate(const chrome_variations::Study& study,
+                                       const base::Time& date_time) {
+  const base::Time epoch = base::Time::UnixEpoch();
+
+  if (study.has_start_date()) {
+    const base::Time start_date =
+        epoch + base::TimeDelta::FromMilliseconds(study.start_date());
+    if (date_time < start_date)
+      return false;
+  }
+
+  if (study.has_expiry_date()) {
+    const base::Time expiry_date =
+        epoch + base::TimeDelta::FromMilliseconds(study.expiry_date());
+    if (date_time >= expiry_date)
+      return false;
+  }
+
+  return true;
+}