Show all experiments, even those that are unavailable on the current platform

chrome/browser/about_flags.cc

 // Copyright (c) 2011 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/browser/about_flags.h"
 
 #include <algorithm>
 #include <iterator>
 #include <map>
 #include <set>
@@ skipping 24 matching lines @@
     Experiment::SINGLE_VALUE, command_line_switch, switch_value, NULL, 0
 #define SINGLE_VALUE_TYPE(command_line_switch) \
     SINGLE_VALUE_TYPE_AND_VALUE(command_line_switch, "")
 #define MULTI_VALUE_TYPE(choices) \
     Experiment::MULTI_VALUE, "", "", choices, arraysize(choices)
 
 namespace {
 
 const unsigned kOsAll = kOsMac | kOsWin | kOsLinux | kOsCrOS;
 
+std::map<int, std::string> nameMap;

[Nico, 2011/12/16 20:25:07]

Global non-pods need both static initialization and exit time destruction, both
things that we're trying to get rid of (and both things that will turn bots red
nowadays)

+
+std::map<int, std::string> OSNames() {
+  if (nameMap.empty()) {
+    nameMap[kOsMac] = "Mac";
+    nameMap[kOsWin] = "Windows";
+    nameMap[kOsLinux] = "Linux";
+    nameMap[kOsCrOS] = "Chrome OS";

[Nico, 2011/12/16 20:25:07]

std::map<int, ...> with less than many keys is almost always better done as a
switch statement

+  }
+  return nameMap;
+}
+
 // Names for former Chrome OS Labs experiments, shared with prefs migration
 // code.
 const char kMediaPlayerExperimentName[] = "media-player";
 const char kAdvancedFileSystemExperimentName[] = "advanced-file-system";
 const char kVerticalTabsExperimentName[] = "vertical-tabs";
 
 const Experiment::Choice kPrerenderFromOmniboxChoices[] = {
   { IDS_FLAGS_PRERENDER_FROM_OMNIBOX_AUTOMATIC, "", "" },
   { IDS_FLAGS_PRERENDER_FROM_OMNIBOX_ENABLED, switches::kPrerenderFromOmnibox,
     switches::kPrerenderFromOmniboxSwitchValueEnabled },
@@ skipping 569 matching lines @@
 
 ListValue* GetFlagsExperimentsData(PrefService* prefs) {
   std::set<std::string> enabled_experiments;
   GetSanitizedEnabledFlags(prefs, &enabled_experiments);
 
   int current_platform = GetCurrentPlatform();
 
   ListValue* experiments_data = new ListValue();
   for (size_t i = 0; i < num_experiments; ++i) {
     const Experiment& experiment = experiments[i];
-    if (!(experiment.supported_platforms & current_platform))
-      continue;
 
     DictionaryValue* data = new DictionaryValue();
     data->SetString("internal_name", experiment.internal_name);
     data->SetString("name",
                     l10n_util::GetStringUTF16(experiment.visible_name_id));
     data->SetString("description",
                     l10n_util::GetStringUTF16(
                         experiment.visible_description_id));
+    bool supported = experiment.supported_platforms & current_platform;
+    data->SetBoolean("supported", supported);
+
+    ListValue* supported_platforms = new ListValue();
+    std::map<int, std::string> os_names = OSNames();
+    for (std::map<int, std::string>::const_iterator iter = os_names.begin();
+         iter != os_names.end(); ++iter) {
+      if (experiment.supported_platforms & iter->first) {
+        supported_platforms->Append(new StringValue(iter->second));
+      }
+    }

[Nico, 2011/12/16 20:25:07]

supported_platforms->Append(new StringValue("Mac"));
supported_platforms->Append(new StringValue("Windows"));
supported_platforms->Append(new StringValue("Linux"));
supported_platforms->Append(new StringValue("ChromeOS"));

does the same thing, is easier to understand, and less code

[Tyler Breisacher (Chromium), 2011/12/16 21:18:11]

Wouldn't that append all four platforms to every single experiment? We want to
only show the platform(s) where this particular experiment is available. I'm not
sure how to do this more clearly.

[Nico, 2011/12/16 21:38:04]

Ah, right. Either write the function you have above with a switch and do

for (int platforms = experiment.supported_platforms; platforms; platforms &=
platforms - 1)
  supported_platforms->Append(new StringValue(NameForPlatform(platform &
-platform)));

(which I suppose is a bit hard to read) or do

void AddOsStrings(bitmask, list) {
  struct { int bit, const char str[] } kBitsToOs = {
    ...
  }
  for (int i = 0; i < ARRAYSIZE_UNSAFE(kBitsToOs); ++i)
    if (bitmask & kBitsToOs[i].bit)
      list->Append(new StringValue(kBitsToOs[i].str)
}

and call that from here

I like the second approach, since it also reduces the number of nested loops in
GetFlagsExperimentData

+    data->Set("supported_platforms", supported_platforms);
 
     switch (experiment.type) {
       case Experiment::SINGLE_VALUE:
         data->SetBoolean(
             "enabled",
             enabled_experiments.count(experiment.internal_name) > 0);
         break;
       case Experiment::MULTI_VALUE:
         data->Set("choices", CreateChoiceData(experiment, enabled_experiments));
         break;
@@ skipping 211 matching lines @@
 }
 
 const Experiment* GetExperiments(size_t* count) {
   *count = num_experiments;
   return experiments;
 }
 
 }  // namespace testing
 
 }  // namespace about_flags