Collect UMA statistics on which chrome://flags lead to chrome restart on ChromeOS.

chrome/browser/about_flags_unittest.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 "base/memory/scoped_vector.h"
 #include "base/prefs/pref_registry_simple.h"
 #include "base/prefs/testing_pref_service.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/browser/about_flags.h"
 #include "chrome/browser/pref_service_flags_storage.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "grit/chromium_strings.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/libxml/chromium/libxml_utils.h"

[Ilya Sherman, 2014/07/22 20:37:05]

FWIW, the Autofill code uses #include
"third_party/libjingle/source/talk/xmllite/xmlparser.h" for XML parsing.  I'm
not sure which is simpler to use.

 
 const char kFlags1[] = "flag1";
 const char kFlags2[] = "flag2";
 const char kFlags3[] = "flag3";
 const char kFlags4[] = "flag4";
 const char kFlags5[] = "flag5";
 
 const char kSwitch1[] = "switch";
 const char kSwitch2[] = "switch2";
 const char kSwitch3[] = "switch3";
 const char kValueForSwitch2[] = "value_for_switch2";
 
 const char kMultiSwitch1[] = "multi_switch1";
 const char kMultiSwitch2[] = "multi_switch2";
 const char kValueForMultiSwitch2[] = "value_for_multi_switch2";
 
 const char kEnableDisableValue1[] = "value1";
 const char kEnableDisableValue2[] = "value2";
 
 namespace about_flags {
 
+enum TestSwitchesHistogramIDs {
+  ID_UNKNOWN_FLAG = UMA_HISTOGRAM_ID_UNKNOWN_FLAG,
+  ID_BAD_FLAG_FORMAT = UMA_HISTOGRAM_ID_BAD_FLAG_FORMAT,
+  ID_kMultiSwitch1,
+  ID_kMultiSwitch2,
+  ID_kSwitch1,
+  ID_kSwitch2,
+  ID_kSwitch3,
+};
+
 const Experiment::Choice kMultiChoices[] = {
-  { IDS_PRODUCT_NAME, "", "" },
-  { IDS_PRODUCT_NAME, kMultiSwitch1, "" },
-  { IDS_PRODUCT_NAME, kMultiSwitch2, kValueForMultiSwitch2 },
+  { IDS_PRODUCT_NAME, "", "", 0},
+  { IDS_PRODUCT_NAME, kMultiSwitch1, "", ID_kMultiSwitch1},
+  { IDS_PRODUCT_NAME, kMultiSwitch2, kValueForMultiSwitch2, ID_kMultiSwitch2},
 };
 
 // The experiments that are set for these tests. The 3rd experiment is not
 // supported on the current platform, all others are.
 static Experiment kExperiments[] = {
   {
     kFlags1,
     IDS_PRODUCT_NAME,
     IDS_PRODUCT_NAME,
     0,  // Ends up being mapped to the current platform.
     Experiment::SINGLE_VALUE,
     kSwitch1,
     "",
+    ID_kSwitch1,
     NULL,
     NULL,
+    0,
     NULL,
     0
   },
   {
     kFlags2,
     IDS_PRODUCT_NAME,
     IDS_PRODUCT_NAME,
     0,  // Ends up being mapped to the current platform.
     Experiment::SINGLE_VALUE,
     kSwitch2,
     kValueForSwitch2,
+    ID_kSwitch2,
     NULL,
     NULL,
+    0,
     NULL,
     0
   },
   {
     kFlags3,
     IDS_PRODUCT_NAME,
     IDS_PRODUCT_NAME,
     0,  // This ends up enabling for an OS other than the current.
     Experiment::SINGLE_VALUE,
     kSwitch3,
     "",
+    ID_kSwitch3,
     NULL,
     NULL,
+    0,
     NULL,
     0
   },
   {
     kFlags4,
     IDS_PRODUCT_NAME,
     IDS_PRODUCT_NAME,
     0,  // Ends up being mapped to the current platform.
     Experiment::MULTI_VALUE,
     "",
     "",
+    0,
     "",
     "",
+    0,
     kMultiChoices,
     arraysize(kMultiChoices)
   },
   {
     kFlags5,
     IDS_PRODUCT_NAME,
     IDS_PRODUCT_NAME,
     0,  // Ends up being mapped to the current platform.
     Experiment::ENABLE_DISABLE_VALUE,
     kSwitch1,
     kEnableDisableValue1,
+    ID_kSwitch1,
     kSwitch2,
     kEnableDisableValue2,
+    ID_kSwitch2,
     NULL,
     3
   },
 };
 
+/*
+  This table contains histogram IDs for switches. Switch ID must never change!
+*/
+const char* const HistogramSwitchesOrdered[] = {
+    NULL, /* No flag */
+    NULL, /* Bad flag format */
+    NULL, /* reserved */
+    NULL, /* reserved */
+    NULL, /* reserved */
+    "disable-webrtc-hw-encoding",
+    "disable-minimize-on-second-launcher-item-click",
+    "disable-virtual-keyboard-overscroll",
+    "disable-virtual-keyboard-overscroll",
+    "enable-pinch-virtual-viewport",
+    "prefetch-search-results",
+    "enable-experimental-app-list",
+    "enable-devtools-experiments",
+    "enable-centered-app-list",
+    "enable-accelerated-overflow-scroll",
+    "enable-tcp-fastopen",
+    "enable-zero-suggest-personalized",
+    "enable-experimental-web-platform-features",
+    "use-simple-cache-backend",
+    "disable-search-button-in-omnibox",
+    "file-manager-enable-new-audio-player",
+    "disable-prefixed-encrypted-media",
+    "disable-origin-chip",
+    "disable-touch-adjustment",
+    "disable-offline-auto-reload",
+    "enable-fixed-position-compositing",
+    "enable-nacl",
+    "disable-saml-signin",
+    "disable-views-rect-based-targeting",
+    "enable-linkable-ephemeral-apps",
+    "enable-zero-copy",
+    "enable-session-crashed-bubble",
+    "enable-spelling-auto-correct",
+    "disable-suggestions-service",
+    "disable-app-list-app-info",
+    "allow-insecure-websocket-from-https-origin",
+    "enable-input-view",
+    "enable-web-midi",
+    "disable-app-list-voice-search",
+    "disable-offline-load-stale-cache",
+    "manual-enhanced-bookmarks",
+    "num-raster-threads",
+    "disable-cast",
+    "enable-instant-search-clicks",
+    "enable-zero-suggest-ether-noserp",
+    "enable-overlay-scrollbar",
+    "enable-spdy4",
+    "disable-boot-animation",
+    "disable-password-generation",
+    "disable-software-rasterizer",
+    "enable-avfoundation",
+    "disable-spdy-proxy-dev-auth-origin",
+    "disable-new-profile-management",
+    "mediadrm-enable-non-compositing",
+    "disable-text-input-focus-manager",
+    "enable-smooth-scrolling",
+    "enable-password-generation",
+    "disable-device-discovery",
+    "scroll-end-effect",
+    "enable-delegated-renderer",
+    "ash-enable-touch-view-testing",
+    "touch-events",
+    "disable-new-ntp",
+    "disable-permissions-bubbles",
+    "enable-network-portal-notification",
+    "disable-media-source",
+    "enable-encrypted-media",
+    "enable-apps-file-associations",
+    "enable-search-button-in-omnibox-for-str",
+    "disable-sync-app-list",
+    "file-manager-enable-new-gallery",
+    "enable-fast-unload",
+    "disable-fast-text-autosizing",
+    "tab-capture-upscale-quality",
+    "disable-threaded-compositing",
+    "enable-accelerated-fixed-root-background",
+    "enable-lcd-text",
+    "nacl-debug-mask",
+    "disable-transition-compositing",
+    "enable-embeddedsearch-api",
+    "enable-settings-window",
+    "force-device-scale-factor",
+    "disable-password-manager-reauthentication",
+    "disable-pinch-virtual-viewport",
+    "disable-webgl",
+    "save-page-as-mhtml",
+    "disable-zero-suggest",
+    "show-composited-layer-borders",
+    "enable-zero-suggest-most-visited",
+    "enable-answers-in-suggest",
+    "malware-interstitial-v3",
+    "enable-virtual-keyboard",
+    "disable-quic",
+    "default-tile-width",
+    "enable-automatic-password-saving",
+    "enable-search-button-in-omnibox-always",
+    "disable-input-view",
+    "enable-one-copy",
+    "overscroll-history-navigation",
+    "enable-quic-https",
+    "js-flags",
+    "enable-nacl-debug",
+    "enable-viewport-meta",
+    "enable-experimental-input-view-features",
+    "disable-gpu-rasterization",
+    "enable-print-preview-register-promos",
+    "enable-simplified-fullscreen",
+    "enable-accessibility-tab-switcher",
+    "enable-quic",
+    "enable-origin-chip-on-srp",
+    "fast-user-switching",
+    "enable-touch-editing",
+    "wallet-service-use-sandbox",
+    "enable-carrier-switching",
+    "disable-contextual-search",
+    "enable-zero-suggest-ether-serp",
+    "enable-cloud-devices",
+    "disable-quic-https",
+    "enable-touch-drag-drop",
+    "enable-permissions-bubbles",
+    "enable-first-run-ui-transitions",
+    "disable-device-discovery-notifications",
+    "enable-threaded-compositing",
+    "enable-easy-unlock",
+    "enable-origin-chip-always",
+    "enable-pinch",
+    "enable-bleeding-edge-rendering-fast-paths",
+    "disable-lcd-text",
+    "enable-streamlined-hosted-apps",
+    "disable-webrtc",
+    "enable-save-password-bubble",
+    "enable-apps-show-on-first-paint",
+    "enable-new-ntp",
+    "enable-text-input-focus-manager",
+    "enable-service-worker-sync",
+    "enable-harfbuzz-rendertext",
+    "enable-download-resumption",
+    "new-profile-management",
+    "disable-touch-editing",
+    "google-profile-info",
+    "enable-impl-side-painting",
+    "enable-distance-field-text",
+    "enable-deferred-image-decoding",
+    "manual-enhanced-bookmarks-optout",
+    "enable-search-button-in-omnibox-for-str-or-iip",
+    "enable-offline-auto-reload",
+    "enable-experimental-canvas-features",
+    "enable-app-install-alerts",
+    "enable-cloud-print-xps",
+    "max-tiles-for-interest-area",
+    "enable-app-list",
+    "disable-accelerated-video-decode",
+    "out-of-process-pdf",
+    "disable-session-crashed-bubble",
+    "enable-swipe-selection",
+    "disable-fixed-position-compositing",
+    "enable-web-based-signin",
+    "ssl-interstitial-v2-gray",
+    "enable-sync-app-list",
+    "disable-compositor-touch-hit-testing",
+    "disable-accelerated-fixed-root-background",
+    "enhanced-bookmarks-experiment",
+    "disable-pnacl",
+    "extension-content-verification",
+    "disable-touch-drag-drop",
+    "default-tile-height",
+    "disable-sync-synced-notifications",
+    "new-avatar-menu",
+    "allow-nacl-socket-api",
+    "enable-experimental-extension-apis",
+    "enable-app-window-controls",
+    "silent-debugger-extension-api",
+    "enable-suggestions-service",
+    "enable-contextual-search",
+    "enable-fast-text-autosizing",
+    "ash-touch-hud",
+    "disable-accelerated-overflow-scroll",
+    "disable-async-dns",
+    "disable-webaudio",
+    "disable-delegated-renderer",
+    "disable-save-password-bubble",
+    "enable-offline-load-stale-cache",
+    "disable-display-color-calibration",
+    "debug-packed-apps",
+    "enable-gpu-rasterization",
+    "disable-impl-side-painting",
+    "disable-distance-field-text",
+    "performance-monitor-gathering",
+    "disable-pinch",
+    "enable-syncfs-directory-operation",
+    "disable-ntp-other-sessions-menu",
+    "enable-spelling-feedback-field-trial",
+    "ssl-interstitial-v1",
+    "disable-gesture-requirement-for-media-playback",
+    "touch-scrolling-mode",
+    "enable-touchpad-three-finger-click",
+    "disable-quickoffice-component-app",
+    "enable-transition-compositing",
+    "disable-account-consistency",
+    "enable-request-tablet-site",
+    "tab-capture-downscale-quality",
+    "enable-service-worker",
+    "ash-debug-shortcuts",
+    "enable-sync-synced-notifications",
+    "ignore-gpu-blacklist",
+    "ssl-interstitial-v2-colorful",
+    "do-not-ignore-autocomplete-off",
+    "disable-accelerated-2d-canvas",
+    "enable-gesture-tap-highlight",
+    "reset-app-list-install-state",
+    "enable-scroll-prediction",
+    "enable-ephemeral-apps",
+    "enable-webgl-draft-extensions",
+    "disable-network-portal-notification",
+    "enable-device-discovery-notifications",
+    "disable-layer-squashing",
+    "disable-gesture-tap-highlight",
+    "enable-offline-auto-reload-visible-only",
+    "enable-spdy-proxy-dev-auth-origin",
+    "enable-translate-new-ux",
+    "no-pings",
+    "enable-scripts-require-action",
+    "disable-webrtc-hw-decoding",
+    "enable-virtual-keyboard-overscroll",
+    "disable-direct-write",
+    "extensions-on-chrome-urls",
+    "malware-interstitial-v2",
+    "enable-account-consistency",
+    "disable-offline-auto-reload-visible-only",
+    "disable-settings-window",
+    "disable-embedded-shared-worker",
+    "show-autofill-type-predictions",
+    "enable-async-dns",
+    "enable-prominent-url-app-flow",
+    "enable-high-dpi-fixed-position-compositing",
+    "force-gpu-rasterization",
+    "disable-device-enumeration",
+    "show-fps-counter",
+    "apps-keep-chrome-alive",
+    "enable-filemanager-mtp",
+    "enable-panels",
+    "disable-overlay-scrollbar",
+    "disable-zero-copy",
+    "disable-click-delay",
+};
+
 class AboutFlagsTest : public ::testing::Test {
  protected:
   AboutFlagsTest() : flags_storage_(&prefs_) {
     prefs_.registry()->RegisterListPref(prefs::kEnabledLabsExperiments);
     testing::ClearState();
+
+    const SwitchesHistogramIDs& switch_histogram_id = GetSwitchesHistogramIds();
+    for (SwitchesHistogramIDs::const_iterator i = switch_histogram_id.begin();
+         i != switch_histogram_id.end();
+         ++i) {
+      const int id = i->second;
+      if (static_cast<int>(histogram_id_to_switch_.size()) > id) {
+        EXPECT_FALSE(histogram_id_to_switch_[id])
+            << "Duplicate switch histogram ID: " << id << ": '"
+            << *(histogram_id_to_switch_[id]) << "' conflicts with '"
+            << i->first << "'";
+      } else {
+        histogram_id_to_switch_.resize(id + 1);
+      }
+      histogram_id_to_switch_[id] = new std::string(i->first);
+    }
   }
 
   virtual void SetUp() OVERRIDE {
     for (size_t i = 0; i < arraysize(kExperiments); ++i)
       kExperiments[i].supported_platforms = GetCurrentPlatform();
 
     int os_other_than_current = 1;
     while (os_other_than_current == GetCurrentPlatform())
       os_other_than_current <<= 1;
     kExperiments[2].supported_platforms = os_other_than_current;
 
     testing::SetExperiments(kExperiments, arraysize(kExperiments));
   }
 
   virtual void TearDown() OVERRIDE {
     testing::SetExperiments(NULL, 0);
   }
 
   TestingPrefServiceSimple prefs_;
   PrefServiceFlagsStorage flags_storage_;
+  ScopedVector<std::string> histogram_id_to_switch_;
 };
 
-
 TEST_F(AboutFlagsTest, NoChangeNoRestart) {
   EXPECT_FALSE(IsRestartNeededToCommitChanges());
   SetExperimentEnabled(&flags_storage_, kFlags1, false);
   EXPECT_FALSE(IsRestartNeededToCommitChanges());
 }
 
 TEST_F(AboutFlagsTest, ChangeNeedsRestart) {
   EXPECT_FALSE(IsRestartNeededToCommitChanges());
   SetExperimentEnabled(&flags_storage_, kFlags1, true);
   EXPECT_TRUE(IsRestartNeededToCommitChanges());
@@ skipping 75 matching lines @@
 
   CommandLine command_line2(CommandLine::NO_PROGRAM);
 
   ConvertFlagsToSwitches(&flags_storage_, &command_line2, kNoSentinels);
 
   EXPECT_TRUE(command_line2.HasSwitch(kSwitch1));
   EXPECT_FALSE(command_line2.HasSwitch(switches::kFlagSwitchesBegin));
   EXPECT_FALSE(command_line2.HasSwitch(switches::kFlagSwitchesEnd));
 }
 
+CommandLine::StringType CreateSwitch(const std::string& value) {
+#if defined(OS_WIN)
+  return ASCIIToUTF16(value);
+#else
+  return value;
+#endif
+}
+
 TEST_F(AboutFlagsTest, CompareSwitchesToCurrentCommandLine) {
   SetExperimentEnabled(&flags_storage_, kFlags1, true);
 
+  // double dash
+  const std::string kDD("--");
+
   CommandLine command_line(CommandLine::NO_PROGRAM);
   command_line.AppendSwitch("foo");
 
   CommandLine new_command_line(CommandLine::NO_PROGRAM);
   ConvertFlagsToSwitches(&flags_storage_, &new_command_line, kAddSentinels);
 
-  EXPECT_FALSE(AreSwitchesIdenticalToCurrentCommandLine(new_command_line,
-                                                        command_line));
+  EXPECT_FALSE(AreSwitchesIdenticalToCurrentCommandLine(
+      new_command_line, command_line, NULL));
+  {
+    std::set<CommandLine::StringType> difference;
+    EXPECT_FALSE(AreSwitchesIdenticalToCurrentCommandLine(
+        new_command_line, command_line, &difference));
+    EXPECT_EQ(1U, difference.size());
+    EXPECT_EQ(1U, difference.count(CreateSwitch(kDD + kSwitch1)));
+  }
 
   ConvertFlagsToSwitches(&flags_storage_, &command_line, kAddSentinels);
 
-  EXPECT_TRUE(AreSwitchesIdenticalToCurrentCommandLine(new_command_line,
-                                                       command_line));
+  EXPECT_TRUE(AreSwitchesIdenticalToCurrentCommandLine(
+      new_command_line, command_line, NULL));
+  {
+    std::set<CommandLine::StringType> difference;
+    EXPECT_TRUE(AreSwitchesIdenticalToCurrentCommandLine(
+        new_command_line, command_line, &difference));
+    EXPECT_TRUE(difference.empty());
+  }
 
   // Now both have flags but different.
   SetExperimentEnabled(&flags_storage_, kFlags1, false);
   SetExperimentEnabled(&flags_storage_, kFlags2, true);
 
   CommandLine another_command_line(CommandLine::NO_PROGRAM);
   ConvertFlagsToSwitches(&flags_storage_, &another_command_line, kAddSentinels);
 
-  EXPECT_FALSE(AreSwitchesIdenticalToCurrentCommandLine(new_command_line,
-                                                        another_command_line));
+  EXPECT_FALSE(AreSwitchesIdenticalToCurrentCommandLine(
+      new_command_line, another_command_line, NULL));
+  {
+    std::set<CommandLine::StringType> difference;
+    EXPECT_FALSE(AreSwitchesIdenticalToCurrentCommandLine(
+        new_command_line, another_command_line, &difference));
+    EXPECT_EQ(2U, difference.size());
+    EXPECT_EQ(1U, difference.count(CreateSwitch(kDD + kSwitch1)));
+    EXPECT_EQ(1U,
+              difference.count(
+                  CreateSwitch(kDD + kSwitch2 + "=" + kValueForSwitch2)));
+  }
 }
 
 TEST_F(AboutFlagsTest, RemoveFlagSwitches) {
   std::map<std::string, CommandLine::StringType> switch_list;
   switch_list[kSwitch1] = CommandLine::StringType();
   switch_list[switches::kFlagSwitchesBegin] = CommandLine::StringType();
   switch_list[switches::kFlagSwitchesEnd] = CommandLine::StringType();
   switch_list["foo"] = CommandLine::StringType();
 
   SetExperimentEnabled(&flags_storage_, kFlags1, true);
@@ skipping 185 matching lines @@
 TEST_F(AboutFlagsTest, NoSeparators) {
   testing::SetExperiments(NULL, 0);
   size_t count;
   const Experiment* experiments = testing::GetExperiments(&count);
     for (size_t i = 0; i < count; ++i) {
     std::string name = experiments->internal_name;
     EXPECT_EQ(std::string::npos, name.find(testing::kMultiSeparator)) << i;
   }
 }
 
+TEST_F(AboutFlagsTest, SwitchHistogramTableValid) {
+  if (histogram_id_to_switch_[0])
+    EXPECT_STREQ("", histogram_id_to_switch_[0]->c_str());
+  EXPECT_FALSE(histogram_id_to_switch_[1]); /* BAD_FLAG_FORMAT, */
+  EXPECT_FALSE(histogram_id_to_switch_[2]); /* RESERVED2 */
+  EXPECT_FALSE(histogram_id_to_switch_[3]); /* RESERVED3 */
+  EXPECT_FALSE(histogram_id_to_switch_[4]); /* RESERVED4 */
+  for (size_t i = 5; i < histogram_id_to_switch_.size(); ++i) {
+    if (histogram_id_to_switch_[i]) {
+      EXPECT_LT(i, arraysize(HistogramSwitchesOrdered))
+          << "Switch index " << i << " (switch name '"
+          << histogram_id_to_switch_[i]->c_str()
+          << "') is found in about_flags, but missing from the test.";
+    } else {
+      EXPECT_LT(i, arraysize(HistogramSwitchesOrdered))
+          << "Switch index " << i
+          << " is found in about_flags, but missing from the test.";
+    }
+    if (i >= arraysize(HistogramSwitchesOrdered))
+      continue;
+    if (histogram_id_to_switch_[i])
+      EXPECT_STREQ(HistogramSwitchesOrdered[i],
+                   histogram_id_to_switch_[i]->c_str())
+          << "Switch index " << i << " is wrong.";
+  }
+}
+
+// Returns empty map on error.
+std::map<int, std::string> ReadLoginCustomFlags(XmlReader& reader) {
+  size_t entries_index = 0;
+
+  std::map<int, std::string> result;
+  bool success = true;
+
+  while (true) {
+    const std::string node_name = reader.NodeName();
+    if (node_name == "enum" && reader.IsClosingElement())
+      break;
+
+    if (node_name == "int") {
+      ++entries_index;
+      std::string value_str;
+      std::string label;
+      if (!reader.NodeAttribute("value", &value_str)) {
+        LOG(ERROR) << "Bad LoginCustomFlags enum entry (at index "
+                   << entries_index << "): No 'value' attribute.";
+        success = false;
+      }
+      if (!reader.NodeAttribute("label", &label)) {
+        LOG(ERROR) << "Bad LoginCustomFlags enum entry (at index "
+                   << entries_index << "): No 'label' attribute.";
+        success = false;
+      }
+      int value;
+      if (!base::StringToInt(value_str, &value)) {
+        LOG(ERROR) << "Bad LoginCustomFlags enum entry (at index "
+                   << entries_index << "): No 'label' attribute.";
+        success = false;
+      }
+      if (success) {
+        result[value] = label;
+      }
+    }
+    reader.Next();
+  }
+  return (success ? result : std::map<int, std::string>());
+}
+
+// Returns empty map on error.
+std::map<int, std::string> ReadHistogramsXml() {
+  XmlReader histograms_xml;
+  std::map<int, std::string> login_custom_flags;
+  EXPECT_TRUE(
+      histograms_xml.LoadFile("tools/metrics/histograms/histograms.xml"));

[Ilya Sherman, 2014/07/08 04:22:11]

It seems more appropriate to use a presubmit check, rather than reading and
parsing histograms.xml via a test.

[Alexander Alekseev, 2014/07/08 20:45:36]

This has been discussed here (and in private talk).

presubmit check might be tricky here:

1) There is no list of all command-line switches . So one of the ideas discussed
was to grep all constant strings from all source files  like *switches*.cc . But
it is going to be very unstable. There are also cases like 
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/abo...

2) A lot of flags are under different #ifdefs, so you actually need to compile
code to parse the list in a stable way.


What this CL does:
1) It introduces full list of command-line switches used in about_flags.cc .
This list is now manually defined and fixed.

2) It actually verifies that compiled code matches histogram.xml.
That is why I've put all checks to test.

3) Unlike presubmit check, this CL cannot involve any damage to commits besides
files in CL. So it is less dangerous.


That is why I've added histograms.xml parsing.

[Ilya Sherman, 2014/07/09 00:10:50]

This list needs to be updated every time a new commandline switch is added. 
That's pretty painful.
The enum doesn't need to be compiled -- it has the same values on all platforms,
and is easy to grep through.
I don't understand this comment.
In addition to my concerns above, I find this test code far too complex for a
unit test.  I can't just look at the test and quickly grok what it's doing.  If
the test were to fail, it wouldn't be obvious to me whether that's caused by a
bug in my code, or a bug in the test code.

[Alexander Alekseev, 2014/07/11 18:13:27]

You mean "every time a new commandline switch is added to
about_flags" it has to be added to two other files. We do not have to
do it for all flags.
But enum doesn't have string representation of values. Adding switch
names as values seems more readable.
Presubmit checks run on each developer workstation on each commit.
If there is an error in presubmit code or environment, it can damage
the commit itself.
Probably I should add more comments on what's happening here.

[Ilya Sherman, 2014/07/12 00:25:10]

Correct, but new flags are expected to be added to about_flags, so the
difference is largely academic.
It looks like trimming UMA_HISTOGRAM_ID_ from the enum entry name gives the
switch name.
I still don't understand.  Is this a general argument that all presubmit checks
should be removed and replaced with tests?  The presubmit check that I'm
recommending is a read-only check, so I don't understand in what way it might
damage a commit.

[Alexander Alekseev, 2014/07/21 15:22:27]

I think that presubmit checks verify the submitted change itself (like style,
attributes, permissions, etc...). They cannot check the build result and should
not pretend they can. The functionality should be checked by tests. On the other
hand, tests cannot check code style, so presubmit hooks are handy.

I've added presubmit hook to check header style.

[Ilya Sherman, 2014/07/21 22:12:59]

Sorry, I think we're talking past each other.  My suggestion is that you add a
presubmit check that verifies that (a) the contents of the enum match the
contents of histograms.xml, and that (b) there aren't any unexpected changes to
the enum, i.e. that the enum is correctly treated as append-only.

You mentioned that a presubmit check cannot verify the build result.  I'm not
suggesting that it do so.  I'm not sure what lines you're thinking along for
this -- what build result needs to be verified?  Each entry in about:flags is
already verified, by the compiler, to have an enum member associated with it --
right?  What else about the build result needs to be verified?

[Alexander Alekseev, 2014/07/22 14:25:05]

As far as I understand, you do not like the idea of reading histograms.xml in a
unit test. And you'd like to drop this unit test completely and replace it with
precommit script, am I right?

The enum values in about_flags.cc are set by people and we need to check that
the mapping between strings (switch names, which are not constants names, but
actual strings that are passed to chrome binary) matches strings that we will
get in UMA.
This can be checked by compiled code only, because parsing about_flags.cc
requires compiler.

The precommit script you're pointing at, strict_enum_value_checker.py, is used
to gather usage of enum. But what we need to monitor is not enum, but actual
switch "strings" (I don't know what it should be called as you think that it's
not a name.).

And enum here is purely internal data. It means that someone, who adds a switch
to about_flags, is not interested in correct enum usage. If he makes a mistake
in enum value in about_flags.cc , nothing bad would happen to his functionality.
So we need to check that strings in about_flags exactly match strings in
histograms.xml. And string values are available at compile time only, so there
is no way to check it with precommit script.

And looking at precommit script that I've added here, it looks to me that manual
check of about_flags_switches_histogram_ids.h is less time-consuming then
support of 160 lines of non-trivial python code which definitely requires a test
for itself. At least for me as I'll be the owner of
about_flags_switches_histogram_ids.h ;-) (Probably I'll add it later, when I get
tired of pointing at the same errors, but I need some commit statistics.)

+
+  while (true) {
+    const std::string node_name = histograms_xml.NodeName();
+    if (node_name == "enum") {
+      std::string name;
+      if (histograms_xml.NodeAttribute("name", &name) &&
+          name == "LoginCustomFlags") {
+        EXPECT_TRUE(login_custom_flags.empty())
+            << "Duplicate enum LoginCustomFlags found in histograms.xml";
+        if (!login_custom_flags.empty())
+          return std::map<int, std::string>();
+
+        if (histograms_xml.Read()) {
+          login_custom_flags = ReadLoginCustomFlags(histograms_xml);
+          EXPECT_FALSE(login_custom_flags.empty());
+          if (login_custom_flags.empty())
+            return std::map<int, std::string>();
+        }
+      }
+    }
+    if (histograms_xml.Read())
+      continue;
+
+    if (histograms_xml.Next())
+      continue;
+
+    while (histograms_xml.Depth() && !histograms_xml.SkipToElement()) {
+    }
+
+    if (!histograms_xml.Depth())
+      break;
+  }
+  EXPECT_FALSE(login_custom_flags.empty())
+      << "enum LoginCustomFlags is not found in histograms.xml";
+  return login_custom_flags;
+}
+
+TEST_F(AboutFlagsTest, CheckHistograms) {
+  std::map<int, std::string> login_custom_flags = ReadHistogramsXml();
+
+  for (size_t i = 5; i < arraysize(HistogramSwitchesOrdered); ++i) {
+    EXPECT_TRUE(login_custom_flags.count(i))
+        << "histograms.xml enum LoginCustomFlags doesn't contain switch '"
+        << HistogramSwitchesOrdered[i] << "'";
+
+    if (login_custom_flags.count(i)) {
+      EXPECT_STREQ(HistogramSwitchesOrdered[i], login_custom_flags[i].c_str())
+          << "Bad histograms.xml enum LoginCustomFlags entry with value='" << i
+          << "'.";
+    }
+  }
+  // If maximum index in histograms.xml is greater than in test.
+  if (static_cast<size_t>(login_custom_flags.rbegin()->first) >=
+      arraysize(HistogramSwitchesOrdered)) {
+    for (std::map<int, std::string>::reverse_iterator ri =
+             login_custom_flags.rbegin();
+         ri != login_custom_flags.rend();
+         ++ri) {
+      if (static_cast<size_t>(ri->first) < arraysize(HistogramSwitchesOrdered))
+        break;
+      EXPECT_LT(static_cast<size_t>(ri->first),
+                arraysize(HistogramSwitchesOrdered))
+          << "Test has no data for histograms.xml enum LoginCustomFlags entry "
+             "with value='" << ri->first << "' label='" << ri->second << "'";
+    }
+  }
+}
+
 }  // namespace about_flags