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

chrome/installer/gcapi/gcapi_omaha_experiment.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/installer/gcapi/gcapi_omaha_experiment.h"
 
-#include "base/strings/string16.h"
+#include "base/basictypes.h"
+#include "base/lazy_instance.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"
 
 using base::Time;
 using base::TimeDelta;
 
 namespace {
 
 // Returns the number of weeks since 2/3/2003.
 int GetCurrentRlzWeek() {
   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;
   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());
+  if (!GoogleUpdateSettings::ReadExperimentLabels(system_level,
+                                                  &experiment_labels)) {
+    return false;
+  }
 
-  return GoogleUpdateSettings::SetExperimentLabels(
-      shell_mode == GCAPI_INVOKED_UAC_ELEVATION,
-      experiment_labels);
+  // First erase this label from the existing experiment labels if it exists.

[Alexei Svitkine (slow), 2013/09/03 15:46:35]

I think this logic is now sufficiently complicated that I'd rather see the
simpler code that uses SplitStringUsingSubstr() and iterates through the entries
appending them to form a new string.

Yes, that could would be less efficient, but it would be much more readable and
this is hardly performance critical and the input size is small anyway, so it
would still be very fast.

[gab, 2013/11/05 22:20:38]

Done.

+  size_t existing_label_begin = experiment_labels.find(label + L"=");
+  while (existing_label_begin != string16::npos &&
+         existing_label_begin != 0 &&
+         experiment_labels.at(
+             existing_label_begin - 1) != *google_update::kExperimentLabelSep) {
+    // Make sure |existing_label_begin|, if found, is either at the beginning of
+    // |experiment_labels| or right after the separator.
+    existing_label_begin = experiment_labels.find(label + L"=",
+                                                  existing_label_begin + 1);
+  }
+  if (existing_label_begin != string16::npos) {
+    size_t existing_label_end =
+        experiment_labels.find(google_update::kExperimentLabelSep,
+                               existing_label_begin);
+    // Note: if |existing_label_end == string16::npos| this will work anyways
+    // as npos is defined as the biggest possible unsigned integer and erase
+    // will thus, by definition, erase to the end of the string.
+    experiment_labels.erase(existing_label_begin,
+                            existing_label_end - existing_label_begin + 1);
+  }
+
+  // Then append the GCAPI experiment label to the |experiment_labels|.
+  if (!experiment_labels.empty())
+    experiment_labels.append(google_update::kExperimentLabelSep);
+  experiment_labels.append(
+      gcapi_internals::GetGCAPIExperimentLabel(brand_code, label));
+
+  return GoogleUpdateSettings::SetExperimentLabels(system_level,
+                                                   experiment_labels);
 }
 
 }  // namespace
 
+namespace gcapi_internals {
+
+const wchar_t kReactivationLabel[] = L"reacbrand";
+const wchar_t kRelaunchLabel[] = L"relaunchbrand";
+
+// Keeps a fixed state for time dependent constants for the life of this
+// GCAPI instance; this makes tests realiable when crossing time boundaries on

[Alexei Svitkine (slow), 2013/09/03 15:46:35]

Nit: reliable

[gab, 2013/11/05 22:20:38]

"real" "liable" is nice, no?! :)

+// the system clock.
+struct GCAPILabelState {
+  GCAPILabelState()
+      : week_number(GetCurrentRlzWeek()),
+        experiment_date_string(installer::BuildExperimentDateString()) {
+    if (week_number < 0 || week_number > 999)
+      week_number = 999;
+  }
+
+  int week_number;
+  string16 experiment_date_string;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(GCAPILabelState);
+};
+
+base::LazyInstance<GCAPILabelState> g_gcapi_label_state =
+    LAZY_INSTANCE_INITIALIZER;
+
+string16 GetGCAPIExperimentLabel(const wchar_t* brand_code,
+                                 const string16& label) {
+  const GCAPILabelState& gcapi_label_state = g_gcapi_label_state.Get();

[Alexei Svitkine (slow), 2013/09/03 15:46:35]

Instead of having this GCAPILabelState state in lazy instance, how about
changing the the functions GetCurrentRlzWeek() and
installer::BuildExperimentDateString() to take a base::Time parameter. That way,
you'll ensure the same Time is used for both the week and the time string
(removing the possibility of them being inconsistent if the time changes between
those two calls). 

If you do the above, you can then simplify things a lot here by no longer
needing a lazy instance of the GCAPILabelState structure - instead you can
simply cache the time value instead and call both functions with that param.

Then, you can make the GetGCAPIExperimentLabel() function do this:

static time_t time_value = 0;
if (time_value == 0)
  time_value = base::Time::Now().ToTimeT();

base::Time time = base::Time::FromTimeT(time_value);
...
GetCurrentRlzWeek(time);
installer::BuildExperimentDateString(time);

[gab, 2013/11/05 22:20:38]

Done.

+
+  string16 gcapi_experiment_label;
+  base::SStringPrintf(&gcapi_experiment_label,
+                      L"%ls=%ls_%d|%ls",
+                      label.c_str(),
+                      brand_code,
+                      gcapi_label_state.week_number,
+                      gcapi_label_state.experiment_date_string.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);
 }