about_flags::ReportCustomFlags() should use signed 32-bit IDs.

On 32-bit architectures UMA Histogram IDs (enum
LoginCustomFlags) are negative. So HistogramBase::Sample
(which is int32_t) should be used.

BUG=408196
TEST=manual

Review URL: https://codereview.chromium.org/509923003

Cr-Commit-Position: refs/heads/master@{#293062}

chrome/browser/about_flags.cc

@@ -67,7 +67,7 @@ using base::UserMetricsAction;
 
 namespace about_flags {
 
-const uint32_t kBadSwitchFormatHistogramId = 0;
+const base::HistogramBase::Sample kBadSwitchFormatHistogramId = 0;
 
 // Macros to simplify specifying the type.
 #define SINGLE_VALUE_TYPE_AND_VALUE(command_line_switch, switch_value) \
@@ -2265,8 +2265,9 @@ void RecordUMAStatistics(FlagsStorage* flags_storage) {
   content::RecordAction(UserMetricsAction("StartupTick"));
 }
 
-uint32_t GetSwitchUMAId(const std::string& switch_name) {
-  return static_cast<uint32_t>(metrics::HashMetricName(switch_name));
+base::HistogramBase::Sample GetSwitchUMAId(const std::string& switch_name) {
+  return static_cast<base::HistogramBase::Sample>(
+      metrics::HashMetricName(switch_name));
 }
 
 void ReportCustomFlags(const std::string& uma_histogram_hame,

chrome/browser/about_flags.h

@@ -12,6 +12,7 @@
 #include <string>
 
 #include "base/command_line.h"
+#include "base/metrics/histogram_base.h"
 #include "base/strings/string16.h"
 
 class PrefService;
@@ -26,7 +27,7 @@ class FlagsStorage;
 
 // This value is reported as switch histogram ID if switch name has unknown
 // format.
-extern const uint32_t kBadSwitchFormatHistogramId;
+extern const base::HistogramBase::Sample kBadSwitchFormatHistogramId;
 
 // Enumeration of OSs.
 // This is exposed only for testing.
@@ -171,7 +172,7 @@ int GetCurrentPlatform();
 void RecordUMAStatistics(FlagsStorage* flags_storage);
 
 // Returns the UMA id for the specified switch name.
-uint32_t GetSwitchUMAId(const std::string& switch_name);
+base::HistogramBase::Sample GetSwitchUMAId(const std::string& switch_name);
 
 // Sends stats (as UMA histogram) about command_line_difference.
 // This is used on ChromeOS to report flags that lead to browser restart.

chrome/browser/about_flags_unittest.cc

@@ -41,19 +41,20 @@ const char kValueForMultiSwitch2[] = "value_for_multi_switch2";
 const char kEnableDisableValue1[] = "value1";
 const char kEnableDisableValue2[] = "value2";
 
-typedef std::map<std::string, uint32_t> SwitchToIdMap;
+typedef base::HistogramBase::Sample Sample;
+typedef std::map<std::string, Sample> SwitchToIdMap;
 
 // This is a helper function to the ReadEnumFromHistogramsXml().
 // Extracts single enum (with integer values) from histograms.xml.
 // Expects |reader| to point at given enum.
 // Returns map { value => label }.
 // Returns empty map on error.
-std::map<uint32_t, std::string> ParseEnumFromHistogramsXml(
+std::map<Sample, std::string> ParseEnumFromHistogramsXml(
     const std::string& enum_name,
     XmlReader* reader) {
   int entries_index = -1;
 
-  std::map<uint32_t, std::string> result;
+  std::map<Sample, std::string> result;
   bool success = true;
 
   while (true) {
@@ -80,8 +81,8 @@ std::map<uint32_t, std::string> ParseEnumFromHistogramsXml(
         success = false;
       }
 
-      uint32_t value;
-      if (has_value && !base::StringToUint(value_str, &value)) {
+      Sample value;
+      if (has_value && !base::StringToInt(value_str, &value)) {
         ADD_FAILURE() << "Bad " << enum_name << " enum entry (at index "
                       << entries_index << ", label='" << label
                       << "', value_str='" << value_str
@@ -105,7 +106,7 @@ std::map<uint32_t, std::string> ParseEnumFromHistogramsXml(
     // until possible.
     reader->Next();
   }
-  return (success ? result : std::map<uint32_t, std::string>());
+  return (success ? result : std::map<Sample, std::string>());
 }
 
 // Find and read given enum (with integer values) from histograms.xml.
@@ -117,10 +118,10 @@ std::map<uint32_t, std::string> ParseEnumFromHistogramsXml(
 // becomes:
 //   { 9 => "enable-pinch-virtual-viewport" }
 // Returns empty map on error.
-std::map<uint32_t, std::string> ReadEnumFromHistogramsXml(
+std::map<Sample, std::string> ReadEnumFromHistogramsXml(
     const std::string& enum_name,
     XmlReader* histograms_xml) {
-  std::map<uint32_t, std::string> login_custom_flags;
+  std::map<Sample, std::string> login_custom_flags;
 
   // Implement simple depth first search.
   while (true) {
@@ -131,7 +132,7 @@ std::map<uint32_t, std::string> ReadEnumFromHistogramsXml(
         if (!login_custom_flags.empty()) {
           EXPECT_TRUE(login_custom_flags.empty())
               << "Duplicate enum '" << enum_name << "' found in histograms.xml";
-          return std::map<uint32_t, std::string>();
+          return std::map<Sample, std::string>();
         }
 
         const bool got_into_enum = histograms_xml->Read();
@@ -147,7 +148,7 @@ std::map<uint32_t, std::string> ReadEnumFromHistogramsXml(
               << "' (looks empty) found in histograms.xml.";
         }
         if (login_custom_flags.empty())
-          return std::map<uint32_t, std::string>();
+          return std::map<Sample, std::string>();
       }
     }
     // Go deeper if possible (stops at the closing tag of the deepest node).
@@ -676,9 +677,9 @@ class AboutFlagsHistogramTest : public ::testing::Test {
   // This is a helper function to check that all IDs in enum LoginCustomFlags in
   // histograms.xml are unique.
   void SetSwitchToHistogramIdMapping(const std::string& switch_name,
-                                     const uint32_t switch_histogram_id,
-                                     std::map<std::string, uint32_t>* out_map) {
-    const std::pair<std::map<std::string, uint32_t>::iterator, bool> status =
+                                     const Sample switch_histogram_id,
+                                     std::map<std::string, Sample>* out_map) {
+    const std::pair<std::map<std::string, Sample>::iterator, bool> status =
         out_map->insert(std::make_pair(switch_name, switch_histogram_id));
     if (!status.second) {
       EXPECT_TRUE(status.first->second == switch_histogram_id)
@@ -690,9 +691,9 @@ class AboutFlagsHistogramTest : public ::testing::Test {
   // This method generates a hint for the user for what string should be added
   // to the enum LoginCustomFlags to make in consistent.
   std::string GetHistogramEnumEntryText(const std::string& switch_name,
-                                        uint32_t value) {
+                                        Sample value) {
     return base::StringPrintf(
-        "<int value=\"%u\" label=\"%s\"/>", value, switch_name.c_str());
+        "<int value=\"%d\" label=\"%s\"/>", value, switch_name.c_str());
   }
 };
 
@@ -708,7 +709,7 @@ TEST_F(AboutFlagsHistogramTest, CheckHistograms) {
   XmlReader histograms_xml;
   ASSERT_TRUE(histograms_xml.LoadFile(
       FilePathStringTypeToString(histograms_xml_file_path.value())));
-  std::map<uint32_t, std::string> login_custom_flags =
+  std::map<Sample, std::string> login_custom_flags =
       ReadEnumFromHistogramsXml("LoginCustomFlags", &histograms_xml);
   ASSERT_TRUE(login_custom_flags.size())
       << "Error reading enum 'LoginCustomFlags' from histograms.xml.";
@@ -722,7 +723,7 @@ TEST_F(AboutFlagsHistogramTest, CheckHistograms) {
          "Consider adding entry:\n"
       << "  " << GetHistogramEnumEntryText("BAD_FLAG_FORMAT", 0);
   // Check that all LoginCustomFlags entries have correct values.
-  for (std::map<uint32_t, std::string>::const_iterator it =
+  for (std::map<Sample, std::string>::const_iterator it =
            login_custom_flags.begin();
        it != login_custom_flags.end();
        ++it) {
@@ -732,7 +733,7 @@ TEST_F(AboutFlagsHistogramTest, CheckHistograms) {
           "", it->first, &histograms_xml_switches_ids);
       continue;
     }
-    const uint32_t uma_id = GetSwitchUMAId(it->second);
+    const Sample uma_id = GetSwitchUMAId(it->second);
     EXPECT_EQ(uma_id, it->first)
         << "histograms.xml enum LoginCustomFlags "
            "entry '" << it->second << "' has incorrect value=" << it->first
@@ -750,7 +751,7 @@ TEST_F(AboutFlagsHistogramTest, CheckHistograms) {
     // Skip empty placeholders.
     if (it->empty())
       continue;
-    const uint32_t uma_id = GetSwitchUMAId(*it);
+    const Sample uma_id = GetSwitchUMAId(*it);
     EXPECT_NE(kBadSwitchFormatHistogramId, uma_id)
         << "Command-line switch '" << *it
         << "' from about_flags.cc has UMA ID equal to reserved value "

tools/metrics/histograms/pretty_print.py

@@ -80,14 +80,27 @@ def TransformByAlphabetizing(node):
     # Put subnodes in a list of node,key pairs to allow for custom sorting.
     subtag, key_function = ALPHABETIZATION_RULES[node.tagName]
     subnodes = []
-    last_key = -1
+    sort_key = -1
+    pending_node_indices = []
     for c in node.childNodes:
       if (c.nodeType == xml.dom.minidom.Node.ELEMENT_NODE and
           c.tagName == subtag):
-        last_key = key_function(c)
-      # Subnodes that we don't want to rearrange use the last node's key,
-      # so they stay in the same relative position.
-      subnodes.append( (c, last_key) )
+        sort_key = key_function(c)
+        # Replace sort keys for delayed nodes.
+        for idx in pending_node_indices:
+          subnodes[idx][1] = sort_key
+        pending_node_indices = []
+      else:
+        # Subnodes that we don't want to rearrange use the next node's key,
+        # so they stay in the same relative position.
+        # Therefore we delay setting key until the next node is found.
+        pending_node_indices.append(len(subnodes))
+
+      subnodes.append( [c, sort_key] )
+
+    # Use last sort key for trailing unknown nodes.
+    for idx in pending_node_indices:
+      subnodes[idx][1] = sort_key
 
     # Sort the subnode list.
     subnodes.sort(key=lambda pair: pair[1])