The new toast experiments.

chrome/installer/util/google_chrome_distribution.cc

Index: chrome/installer/util/google_chrome_distribution.cc
===================================================================
--- chrome/installer/util/google_chrome_distribution.cc	(revision 147972)
+++ chrome/installer/util/google_chrome_distribution.cc	(working copy)
@@ -60,14 +60,15 @@
 
 // The following strings are the possible outcomes of the toast experiment
 // as recorded in the |client| field.
-const wchar_t kToastExpControlGroup[] =      L"01";
-const wchar_t kToastExpCancelGroup[] =       L"02";
-const wchar_t kToastExpUninstallGroup[] =    L"04";
-const wchar_t kToastExpTriesOkGroup[] =      L"18";
-const wchar_t kToastExpTriesErrorGroup[] =   L"28";
-const wchar_t kToastActiveGroup[] =          L"40";
-const wchar_t kToastUDDirFailure[] =         L"40";
-const wchar_t kToastExpBaseGroup[] =         L"80";
+const wchar_t kToastExpControlGroup[] =        L"01";
+const wchar_t kToastExpCancelGroup[] =         L"02";
+const wchar_t kToastExpUninstallGroup[] =      L"04";
+const wchar_t kToastExpTriesOkGroup[] =        L"18";
+const wchar_t kToastExpTriesOkDefaultGroup[] = L"28";

[Finnur, 2012/07/26 12:27:07]

This would change the result code that gets sent back to the server. Are you
sure it is OK to change the error code from 28 to 48 and reuse 28 for another
purpose?

[Mark Larson, 2012/07/26 16:32:13]

I'm also concerned about how this will show up on the backend. I think it's
probably OK, but it might make more sense to swap 28 and 48 for backward
compatibility. Is there a reason to change the meaning of 28?

BTW, you don't seem to use TriesOKDefaultGroup anywhere else.

[cpu_(ooo_6.6-7.5), 2012/07/27 03:06:23]

Done.

+const wchar_t kToastExpTriesErrorGroup[] =     L"48";
+const wchar_t kToastActiveGroup[] =            L"40";
+const wchar_t kToastUDDirFailure[] =           L"40";
+const wchar_t kToastExpBaseGroup[] =           L"80";
 
 // Substitute the locale parameter in uninstall URL with whatever
 // Google Update tells us is the locale. In case we fail to find
@@ -623,29 +624,41 @@
   // The big experiment in Feb 2011 used SJxx SKxx SLxx SMxx.
   // Note: the plugin infobar experiment uses PIxx codes.
   using namespace attrition_experiments;
+  struct FlavorDetails {
+    int heading_id;
+    int flags;
+  };

[Finnur, 2012/07/26 12:27:07]

This struct should probably move to line 617, as the comment there no longer
describes the struct it was intended to describe...

Also, document it?

+
   static const struct UserExperimentDetails {
     const wchar_t* locale;  // Locale to show this experiment for (* for all).
     const wchar_t* brands;  // Brand codes show this experiment for (* for all).
     int control_group;      // Size of the control group, in percentages.
-    const wchar_t prefix1;  // The first letter for the experiment code.
-    const wchar_t prefix2;  // The second letter for the experiment code. This
-                            // will be incremented by one for each additional
-                            // experiment flavor beyond the first.
-    int flavors;            // Numbers of flavors for this experiment. Should
-                            // always be positive and never exceed the number
-                            // of headings (below).
-    int headings[kMax];     // A list of IDs per experiment. 0 == no heading.
-  } kExperimentFlavors[] = {
-    // This list should be ordered most-specific rule first (catch-all, like all
-    // brands or all locales should be last).
-
-    // The experiment with the more compact bubble. This one is a bit special
-    // because it is split into two: CAxx is regular style bubble and CBxx is
-    // compact style bubble. See |compact_bubble| below.
-    {L"en-US", kBrief, 1, L'C', L'A', 2, { kEnUs3, kEnUs3, 0, 0 } },
-
-    // Catch-all rules.
-    {kAll, kAll, 1, L'B', L'A', 1, {kEnUs3, 0, 0, 0} },
+    const wchar_t* prefix;  // The two letter experiment code. The second letter
+                            // will be incremented with the flavor.
+    FlavorDetails flavors[kMax];
+  } kExperiments[] = {
+    // The first match from top to bottom is used so this list should be ordered
+    // most-specific rule first.
+    { L"*", L"CHMA",  // all locales, CHMA brand.

[Finnur, 2012/07/26 12:27:07]

nit: Capitalize first letter of comments in this line and below.

[cpu_(ooo_6.6-7.5), 2012/07/27 03:06:23]

Done.

+      25,             // 25 percent control group.
+      L"ZA",          // experiment is ZAxx, ZBxx, ZCxx, ZDxx etc.
+      // Three flavors.
+      { { IDS_TRY_TOAST_HEADING3, kDontBugMeAsButton | kUninstall | kWhyLink },

[Finnur, 2012/07/26 12:27:07]

I thought we'd decided not to have a Why link, since it doesn't actually work
(Chrome startup is halted at the toast dialog and pressing the link will launch
a new process that will also halt until you dismiss the dialog). No?

[cpu_(ooo_6.6-7.5), 2012/07/27 03:06:23]

No idea. Can you please raise the issue in the bug?

On 2012/07/26 12:27:07, Finnur wrote:

[Finnur, 2012/07/29 19:09:44]

OK, I refreshed my memory by looking up the bug...
http://code.google.com/p/chromium/issues/detail?id=110672
The consensus was that we should not show the link if Chrome is the default
browser...

On 2012/07/27 03:06:23, cpu wrote:

+        { IDS_TRY_TOAST_HEADING3, 0 },

[Finnur, 2012/07/26 12:27:07]

I'm confused about what these enums mean. What does 0 mean for example?  Is it
TryMe and Don'tBugMe?

Also, the Why link isn't being requested in the bug?

[cpu_(ooo_6.6-7.5), 2012/07/27 03:06:23]

it means don't bug me is a radio button, because the flag just controls how is
shown, and no uninstall or why link. Zero gives you the second ascii art example
in the header :)

+        { IDS_TRY_TOAST_HEADING3, kMakeDefault },
+        { 0, 0 },
+      }
+    },
+    { L"*", L"GGRV",  // all locales, GGRV is enterprise.
+      0,              // 0 percent control group.
+      L"EA",          // experiment is EAxx, EBxx, etc.

[Finnur, 2012/07/26 12:27:07]

Just curious... There's no experiment, so why have an experiment code?

[cpu_(ooo_6.6-7.5), 2012/07/27 03:06:23]

No very good reason: In case we f*ck up we can see in the omaha dashboard. We
would see EAxx, EBxx.

[Finnur, 2012/07/29 19:09:44]

Fair enough. Consider documenting on line 624?

On 2012/07/27 03:06:23, cpu wrote:

[cpu_(ooo_6.6-7.5), 2012/07/31 04:04:09]

Others missing. I'll change here after I talk to larson,or he will document.

On 2012/07/29 19:09:44, Finnur wrote:

+      // No flavors means no experiment.
+      { { 0, 0 },
+        { 0, 0 },
+        { 0, 0 },
+        { 0, 0 }
+      }
+    }
   };
 
   string16 locale;
@@ -656,50 +669,36 @@
   string16 brand;
   if (!GoogleUpdateSettings::GetBrand(&brand))
     brand = ASCIIToWide("");  // Could still be viable for catch-all rules.
-  if (brand == kEnterprise)
-    return false;
 
-  for (int i = 0; i < arraysize(kExperimentFlavors); ++i) {
-    // A maximum of four flavors are supported at the moment.
-    CHECK_LE(kExperimentFlavors[i].flavors, kMax);
-    CHECK_GT(kExperimentFlavors[i].flavors, 0);
-    // Make sure each experiment has valid headings.
-    for (int f = 0; f < kMax; ++f) {
-      if (f < kExperimentFlavors[i].flavors) {
-        CHECK_GT(kExperimentFlavors[i].headings[f], 0);
-      } else {
-        CHECK_EQ(kExperimentFlavors[i].headings[f], 0);
-      }
-    }
-    // The prefix has to be a valid two letter combo.
-    CHECK(kExperimentFlavors[i].prefix1 >= 'A');
-    CHECK(kExperimentFlavors[i].prefix2 >= 'A');
-    CHECK(kExperimentFlavors[i].prefix2 +
-          kExperimentFlavors[i].flavors - 1 <= 'Z');
-
-    if (kExperimentFlavors[i].locale != locale &&
-        kExperimentFlavors[i].locale != ASCIIToWide("*"))
+  for (int i = 0; i < arraysize(kExperiments); ++i) {
+    if (kExperiments[i].locale != locale &&
+        kExperiments[i].locale != ASCIIToWide("*"))
       continue;
 
     std::vector<string16> brand_codes;
-    base::SplitString(kExperimentFlavors[i].brands, L',', &brand_codes);
+    base::SplitString(kExperiments[i].brands, L',', &brand_codes);
     if (brand_codes.empty())
       return false;
     for (std::vector<string16>::iterator it = brand_codes.begin();
          it != brand_codes.end(); ++it) {
       if (*it != brand && *it != L"*")
         continue;
-
       // We have found our match.
+      const UserExperimentDetails& match = kExperiments[i];
+      // Find out how many flavors we have. Zero means no experiment.
+      int num_flavors = 0;
+      while (match.flavors[num_flavors].heading_id) { ++num_flavors; }
+      if (!num_flavors)
+        return false;
+
       if (flavor < 0)
-        flavor = base::RandInt(0, kExperimentFlavors[i].flavors - 1);
+        flavor = base::RandInt(0, num_flavors - 1);
       experiment->flavor = flavor;
-      experiment->heading = kExperimentFlavors[i].headings[flavor];
-      experiment->control_group = kExperimentFlavors[i].control_group;
-      experiment->prefix.resize(2);
-      experiment->prefix[0] = kExperimentFlavors[i].prefix1;
-      experiment->prefix[1] = kExperimentFlavors[i].prefix2 + flavor;
-      experiment->compact_bubble = (brand == kBrief) && (flavor == 1);
+      experiment->heading = match.flavors[flavor].heading_id;
+      experiment->control_group = match.control_group;
+      const wchar_t prefix[] = { match.prefix[0], match.prefix[1] + flavor, 0 };
+      experiment->prefix = prefix;
+      experiment->flags = match.flavors[flavor].flags;
       return true;
     }
   }