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/files/file_path.h"
+#include "base/memory/scoped_vector.h"
+#include "base/path_service.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"
 
 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,
+};
+
+#define ID(x) static_cast<const about_flags::SwitchesUmaHistogramId>(x)
+
 const Experiment::Choice kMultiChoices[] = {
-  { IDS_PRODUCT_NAME, "", "" },
-  { IDS_PRODUCT_NAME, kMultiSwitch1, "" },
-  { IDS_PRODUCT_NAME, kMultiSwitch2, kValueForMultiSwitch2 },
+  { IDS_PRODUCT_NAME, "", "", ID(ID_UNKNOWN_FLAG) },
+  { IDS_PRODUCT_NAME, kMultiSwitch1, "", ID(ID_kMultiSwitch1) },
+  { IDS_PRODUCT_NAME,
+    kMultiSwitch2, kValueForMultiSwitch2, ID(ID_kMultiSwitch2) },
 };
 
+#undef ID
+
 // 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();
+
+    // Extract command-line switches from kExperiments to
+    // histogram_id_to_switch_ in the order of UMA IDs. Each switch is stored
+    // by the idex equal to its ID.
+    const SwitchesHistogramIDs& switch_histogram_id = GetSwitchesHistogramIds();
+    for (SwitchesHistogramIDs::const_iterator i = switch_histogram_id.begin();

[Ilya Sherman, 2014/07/29 07:16:45]

nit: Please use "it" or "iter" for generic iterator names; "i" makes the reader
think of an integer variable.

[Alexander Alekseev, 2014/07/31 00:18:07]

Done.

+         i != switch_histogram_id.end();
+         ++i) {
+      const int id = i->second;
+      if (static_cast<int>(histogram_id_to_switch_.size()) > id) {
+        // Check that enum values are not reused in kExperiments[], i.e.
+        // different switches have different UMA IDs values.
+        // (There is another check for equal switches below.)
+        EXPECT_FALSE(histogram_id_to_switch_[id])
+            << "Duplicate switch histogram ID: " << id << ": '"
+            << *(histogram_id_to_switch_[id]) << "' conflicts with '"
+            << i->first << "'.";
+      } else {

[Ilya Sherman, 2014/07/29 07:16:45]

nit: Please add ASSERT_EQ(id, histogram_id_to_switch_.size());

[Alexander Alekseev, 2014/07/31 00:18:08]

The order of IDs in about_flags is undefined, so it is not right.
(and some IDs might be missing)

[Ilya Sherman, 2014/07/31 00:37:51]

In that case, it really sounds like you ought to use a map.

+        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_;

[Ilya Sherman, 2014/07/29 07:16:45]

Why is this a ScopedVector, rather than just a regular vector; or better yet, a
map?

[Alexander Alekseev, 2014/07/31 00:18:08]

This is scoped vector to store the special value "unused", which usually means
that this is an ID of switch under #ifdef . Or may be a  deprecated switch.

This can be a map, but it is simplier to check that value exists. (i.e. you do
not need to use lower_bound() and check iterators).

[Ilya Sherman, 2014/07/31 00:37:51]

Checking whether a value exists in a map is easy: my_map.count(value).  That's
also much clearer than having special/magic "unused" values.

 };
 
-
 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("--");

[Ilya Sherman, 2014/07/29 07:16:45]

nit: Please name this something like "kDoubleDash" rather than the cryptic "kDD"
plus a comment.  In general, if you find yourself needing to write a comment to
explain a variable name, you should probably name the variable more clearly
instead.

[Alexander Alekseev, 2014/07/31 00:18:07]

Yes, you're right. Done.

+
   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;
   }
 }
 
+// Checks that enum values are not reused in kExperiments[], i.e. equal
+// switches have equal UMA IDs. (Different switches are checked in
+// AboutFlagsTest::AboutFlagsTest().)
+TEST_F(AboutFlagsTest, SwitchHistogramTableValid) {
+  if (histogram_id_to_switch_[0])
+    EXPECT_STREQ("", histogram_id_to_switch_[0]->c_str());

[Ilya Sherman, 2014/07/29 07:16:45]

nit: Prefer std::string() to ""

[Alexander Alekseev, 2014/07/31 00:18:08]

Done.

+  EXPECT_FALSE(histogram_id_to_switch_[1]); /* BAD_FLAG_FORMAT, */
+  EXPECT_FALSE(histogram_id_to_switch_[2]); /* RESERVED2 */

[Ilya Sherman, 2014/07/29 07:16:45]

Why is this RESERVED2 rather than RESERVED1?

[Alexander Alekseev, 2014/07/31 00:18:07]

The idea was to reflect index in constant name.
But I'm OK to rename it.

Done.

+  EXPECT_FALSE(histogram_id_to_switch_[3]); /* RESERVED3 */
+  EXPECT_FALSE(histogram_id_to_switch_[4]); /* RESERVED4 */

[Ilya Sherman, 2014/07/29 07:16:45]

Please assert that the array size is at least 5, since you index into it to at
least there.

[Alexander Alekseev, 2014/07/31 00:18:07]

Done.

+  for (size_t i = 5; i < histogram_id_to_switch_.size(); ++i) {
+    // Some values may be missing from histogram_id_to_switch_[] because
+    // they are under #ifdef and were not compiled.
+    if (histogram_id_to_switch_[i]) {
+      EXPECT_LT(i, arraysize(HistogramSwitchesOrdered))
+          << "Switch index " << i << " (switch name '"
+          << histogram_id_to_switch_[i]->c_str()

[Ilya Sherman, 2014/07/29 07:16:45]

nit: No need for c_str().

[Alexander Alekseev, 2014/07/31 00:18:08]

This is because of ScopedVector .

[Ilya Sherman, 2014/07/31 00:37:51]

I don't understand -- why does a ScopedVector require you to use c_str()? 
What's wrong with accessing the string value in the vector directly, i.e.
"*histogram_id_to_switch_[i]"?

+          << "') 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.";
+  }
+}
+
+// Expects |reader| to point at given enum.
+// Returns map { value => label }.
+// Returns empty map on error.

[Ilya Sherman, 2014/07/29 07:16:45]

Please expand this comment to be more detailed.

[Alexander Alekseev, 2014/07/31 00:18:08]

Done.

+std::map<int, std::string> ParseEnumFromHistogramsXml(

[Ilya Sherman, 2014/07/29 07:16:45]

Please move helper functions into the anonymous namespace.

[Alexander Alekseev, 2014/07/31 00:18:08]

Done.

+    const std::string& enum_name,
+    XmlReader& reader) {
+  size_t entries_index = 0;
+
+  std::map<int, std::string> result;
+  bool success = true;

[Ilya Sherman, 2014/07/29 07:16:45]

It looks like you can entirely replace this with early return statements, which
IMO would be clearer (as well as briefer).

[Alexander Alekseev, 2014/07/31 00:18:07]

This is to report maximum number of errors on a single run (not to stop on the
first one).

+
+  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 " << enum_name << " enum entry (at index "
+                   << entries_index << "): No 'value' attribute.";

[Ilya Sherman, 2014/07/29 07:16:45]

Did you mean to use an EXPECT_ or ASSERT_ statement?  Tests shouldn't print
output that isn't testing assertions.

[Alexander Alekseev, 2014/07/31 00:18:07]

Done.

+        success = false;
+      }
+      if (!reader.NodeAttribute("label", &label)) {
+        LOG(ERROR) << "Bad " << enum_name << " enum entry (at index "
+                   << entries_index << "): No 'label' attribute.";
+        success = false;
+      }
+      int value;
+      if (!base::StringToInt(value_str, &value)) {
+        LOG(ERROR) << "Bad " << enum_name << " 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>());
+}
+
+// Find and read given enum in histograms.xml.
+// Returns map { value => label } so that:
+//   <int value="9" label="enable-pinch-virtual-viewport"/>
+// becomes:
+//   { 9 => "enable-pinch-virtual-viewport" }
+// Returns empty map on error.
+std::map<int, std::string> ReadEnumFromHistogramsXml(
+    const std::string& enum_name,
+    XmlReader& histograms_xml) {
+  std::map<int, std::string> login_custom_flags;
+
+  while (true) {
+    const std::string node_name = histograms_xml.NodeName();
+    if (node_name == "enum") {
+      std::string name;
+      if (histograms_xml.NodeAttribute("name", &name) && name == enum_name) {
+        EXPECT_TRUE(login_custom_flags.empty())
+            << "Duplicate enum " << enum_name << " found in histograms.xml";
+
+        if (!login_custom_flags.empty())
+          return std::map<int, std::string>();
+
+        if (histograms_xml.Read()) {
+          login_custom_flags =
+              ParseEnumFromHistogramsXml(enum_name, 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;

[Ilya Sherman, 2014/07/29 07:16:45]

Please document what these four control statements are doing.

[Alexander Alekseev, 2014/07/31 00:18:07]

Done.

+  }
+  EXPECT_FALSE(login_custom_flags.empty()) << "enum " << enum_name
+                                           << " is not found in histograms.xml";
+  return login_custom_flags;
+}
+
+std::string FilePathStringTypeToString(const base::FilePath::StringType& path) {
+#if defined(OS_WIN)
+  return UTF16ToUTF8(path);
+#else
+  return path;
+#endif
+}
+
+TEST_F(AboutFlagsTest, CheckHistograms) {
+  base::FilePath histograms_xml_file_path;
+  ASSERT_TRUE(
+      PathService::Get(base::DIR_SOURCE_ROOT, &histograms_xml_file_path));
+  histograms_xml_file_path = histograms_xml_file_path.AppendASCII("tools")
+                                 .AppendASCII("metrics")
+                                 .AppendASCII("histograms")
+                                 .AppendASCII("histograms.xml");

[Ilya Sherman, 2014/07/29 07:16:45]

nit: I don't think this formatting is quite right -- the continued lines should
be indented a total of six spaces, rather than 33.

[Alexander Alekseev, 2014/07/31 00:18:07]

This is really strange, but it is a result of clang-format.
Set to 6 spaces manually.

+
+  XmlReader histograms_xml;
+  ASSERT_TRUE(histograms_xml.LoadFile(
+      FilePathStringTypeToString(histograms_xml_file_path.value())));
+  // Check that order and labels of <enum name="LoginCustomFlags" type="int">
+  // match HistogramSwitchesOrdered.
+  std::map<int, std::string> login_custom_flags =
+      ReadEnumFromHistogramsXml("LoginCustomFlags", histograms_xml);
+
+  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,
+  // report all extra items.
+  if (static_cast<size_t>(login_custom_flags.rbegin()->first) >=
+      arraysize(HistogramSwitchesOrdered)) {
+    for (std::map<int, std::string>::reverse_iterator ri =

[Ilya Sherman, 2014/07/29 07:16:45]

Again, please use "it" or "iter" for the iterator name.

[Alexander Alekseev, 2014/07/31 00:18:07]

Done.

+             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