Log base::Features in Launch.FlagsAtStartup

chrome/browser/about_flags.cc

 // Copyright (c) 2012 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 <iterator>
 #include <map>
 #include <set>
 #include <utility>
@@ skipping 2218 matching lines @@
       FlagsStateSingleton::GetFlagsState()->GetSwitchesFromFlags(flags_storage);
   ReportAboutFlagsHistogram("Launch.FlagsAtStartup", switches);
 }
 
 base::HistogramBase::Sample GetSwitchUMAId(const std::string& switch_name) {
   return static_cast<base::HistogramBase::Sample>(
       base::HashMetricName(switch_name));
 }
 
 void ReportAboutFlagsHistogram(const std::string& uma_histogram_name,
                                const std::set<std::string>& flags) {

[Alexei Svitkine (slow), 2016/09/15 19:59:18]

This function is also called from a different place whose result doesn't come
from GetSwitchesFromFlags(), so these changes are not safe.

I suggest keeping the semantics of the |flags| param the same and adding two
separate params for the enabled and disabled features. Then, for the place that
calls this function that doesn't use GetSwitchesFromFlags() you can pass empty
lists for the extra params.

[lawrencewu, 2016/09/16 15:34:56]

I needed only one extra parameter for all features (since the logic for
distinguishing enabled and disabled features happens in GetFeaturesFromFlags()),
and separated the body of ReportAboutFlagsHistogram() into two functions: one
for switches and one for features.

   for (const std::string& flag : flags) {
     int uma_id = about_flags::testing::kBadSwitchFormatHistogramId;
-    if (base::StartsWith(flag, "--", base::CompareCase::SENSITIVE)) {
-      // Skip '--' before switch name.
-      std::string switch_name(flag.substr(2));
 
-      // Kill value, if any.
-      const size_t value_pos = switch_name.find('=');
-      if (value_pos != std::string::npos)
-        switch_name.resize(value_pos);
+    std::string mutable_flag(flag);
+    // Kill value, if any.
+    const size_t value_pos = mutable_flag.find('=');
+    if (value_pos != std::string::npos)
+      mutable_flag.resize(value_pos);
 
-      uma_id = GetSwitchUMAId(switch_name);
-    } else {
-      NOTREACHED() << "ReportAboutFlagsHistogram(): flag '" << flag
-                   << "' has incorrect format.";
-    }
+    uma_id = GetSwitchUMAId(mutable_flag);
     DVLOG(1) << "ReportAboutFlagsHistogram(): histogram='" << uma_histogram_name
-             << "' '" << flag << "', uma_id=" << uma_id;
+             << "' '" << mutable_flag << "', uma_id=" << uma_id;
 
     // Sparse histogram macro does not cache the histogram, so it's safe
     // to use macro with non-static histogram name here.
     UMA_HISTOGRAM_SPARSE_SLOWLY(uma_histogram_name, uma_id);
   }
 }
 
 namespace testing {
 
 const base::HistogramBase::Sample kBadSwitchFormatHistogramId = 0;
 
 const FeatureEntry* GetFeatureEntries(size_t* count) {
   *count = arraysize(kFeatureEntries);
   return kFeatureEntries;
 }
 
 }  // namespace testing
 
 }  // namespace about_flags