GCAPI should append to the existing experiment_labels instead of clobbering them.

chrome/installer/gcapi/gcapi_omaha_experiment.cc

Index: chrome/installer/gcapi/gcapi_omaha_experiment.cc
diff --git a/chrome/installer/gcapi/gcapi_omaha_experiment.cc b/chrome/installer/gcapi/gcapi_omaha_experiment.cc
index ac5899fe675e27fa09c8a2b557ae75fa38633b6a..e9c208f200e96456cee88558c68019d278bdd529 100644
--- a/chrome/installer/gcapi/gcapi_omaha_experiment.cc
+++ b/chrome/installer/gcapi/gcapi_omaha_experiment.cc
@@ -4,10 +4,13 @@
 
 #include "chrome/installer/gcapi/gcapi_omaha_experiment.h"
 
-#include "base/strings/string16.h"
+#include "base/basictypes.h"
+#include "base/strings/string_split.h"
+#include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/time/time.h"
 #include "chrome/installer/gcapi/gcapi.h"
+#include "chrome/installer/util/google_update_constants.h"
 #include "chrome/installer/util/google_update_experiment_util.h"
 #include "chrome/installer/util/google_update_settings.h"
 
@@ -17,42 +20,88 @@ using base::TimeDelta;
 namespace {
 
 // Returns the number of weeks since 2/3/2003.
-int GetCurrentRlzWeek() {
+int GetCurrentRlzWeek(const base::Time& current_time) {
   Time::Exploded february_third_2003_exploded = {2003, 2, 1, 3, 0, 0, 0, 0};
   Time f = Time::FromUTCExploded(february_third_2003_exploded);
-  TimeDelta delta = Time::Now() - f;
+  TimeDelta delta = current_time - f;
   return delta.InDays() / 7;
 }
 
-bool SetLabel(const wchar_t* brand_code, const wchar_t* label, int shell_mode) {
+bool SetExperimentLabel(const wchar_t* brand_code,
+                        const string16& label,
+                        int shell_mode) {
   if (!brand_code) {
     return false;
   }
 
-  int week_number = GetCurrentRlzWeek();
-  if (week_number < 0 || week_number > 999)
-    week_number = 999;
+  const bool system_level = shell_mode == GCAPI_INVOKED_UAC_ELEVATION;
 
-  string16 experiment_labels;
-  base::SStringPrintf(&experiment_labels,
-                      L"%ls=%ls_%d|%ls",
-                      label,
-                      brand_code,
-                      week_number,
-                      installer::BuildExperimentDateString().c_str());
+  string16 original_labels;
+  if (!GoogleUpdateSettings::ReadExperimentLabels(system_level,
+                                                  &original_labels)) {
+    return false;
+  }
+
+  // Split the original labels by the label separator.
+  std::vector<string16> entries;
+  base::SplitStringUsingSubstr(
+      original_labels, google_update::kExperimentLabelSep, &entries);
 
-  return GoogleUpdateSettings::SetExperimentLabels(
-      shell_mode == GCAPI_INVOKED_UAC_ELEVATION,
-      experiment_labels);
+  // Keep all labels, but the one we want to add/replace.
+  string16 new_labels;
+  for (std::vector<string16>::const_iterator it = entries.begin();
+       it != entries.end(); ++it) {
+    if (!it->empty() && !StartsWith(*it + L"=", label, true)) {

[Alexei Svitkine (slow), 2013/11/05 23:05:23]

Shouldn't this be "StartsWith(*it, label + "L=", true)"?

e.g. *it would be something like "foo=bar" and label would be "foo", so you're
testing whether "foo=bar" starts with "foo=", no?

If so, please make sure you have sufficient test coverage for this (maybe you do
and you forgot to run them after making this change...). Also, if you change it,
it probably makes sense to move |label + L"="| to a local var outside the loop,
to avoid the computation every iteration.

[gab, 2013/11/06 16:39:10]

Good catch! Added a new test which fails with this implementation and passes
with the new one.

Thanks!

+      new_labels += *it;
+      new_labels += google_update::kExperimentLabelSep;
+    }
+  }
+
+  new_labels.append(
+      gcapi_internals::GetGCAPIExperimentLabel(brand_code, label));
+
+  return GoogleUpdateSettings::SetExperimentLabels(system_level,
+                                                   new_labels);
 }
 
 }  // namespace
 
+namespace gcapi_internals {
+
+const wchar_t kReactivationLabel[] = L"reacbrand";
+const wchar_t kRelaunchLabel[] = L"relaunchbrand";
+
+string16 GetGCAPIExperimentLabel(const wchar_t* brand_code,
+                                 const string16& label) {
+  // Keeps a fixed time state for this GCAPI instance; this makes tests reliable
+  // when crossing time boundaries on the system clock and doesn't otherwise
+  // affect results of this short lived binary.
+  static time_t instance_time_value = 0;
+  if (instance_time_value == 0)
+    instance_time_value = base::Time::Now().ToTimeT();
+
+  base::Time instance_time = base::Time::FromTimeT(instance_time_value);
+
+  string16 gcapi_experiment_label;
+  base::SStringPrintf(&gcapi_experiment_label,
+                      L"%ls=%ls_%d|%ls",
+                      label.c_str(),
+                      brand_code,
+                      GetCurrentRlzWeek(instance_time),
+                      installer::BuildExperimentDateString(
+                          instance_time).c_str());
+  return gcapi_experiment_label;
+}
+
+}  // namespace gcapi_internals
+
 bool SetReactivationExperimentLabels(const wchar_t* brand_code,
                                      int shell_mode) {
-  return SetLabel(brand_code, L"reacbrand", shell_mode);
+  return SetExperimentLabel(brand_code, gcapi_internals::kReactivationLabel,
+                            shell_mode);
 }
 
 bool SetRelaunchExperimentLabels(const wchar_t* brand_code, int shell_mode) {
-  return SetLabel(brand_code, L"relaunchbrand", shell_mode);
+  return SetExperimentLabel(brand_code, gcapi_internals::kRelaunchLabel,
+                            shell_mode);
 }