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

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>
 
 #include "base/command_line.h"
 #include "base/memory/singleton.h"
+#include "base/metrics/sparse_histogram.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "cc/base/switches.h"
 #include "chrome/browser/bookmarks/enhanced_bookmarks_features.h"
 #include "chrome/browser/flags_storage.h"
 #include "chrome/common/chrome_content_client.h"
 #include "chrome/common/chrome_switches.h"
 #include "components/autofill/core/common/autofill_switches.h"
 #include "components/cloud_devices/common/cloud_devices_switches.h"
 #include "components/nacl/common/nacl_switches.h"
 #include "content/public/browser/user_metrics.h"
 #include "extensions/common/switches.h"
 #include "grit/chromium_strings.h"
 #include "grit/generated_resources.h"
 #include "grit/google_chrome_strings.h"
 #include "media/base/media_switches.h"
+#include "third_party/zlib/zlib.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/ui_base_switches.h"
 #include "ui/display/display_switches.h"
 #include "ui/events/event_switches.h"
 #include "ui/gfx/switches.h"
 #include "ui/gl/gl_switches.h"
 #include "ui/keyboard/keyboard_switches.h"
 #include "ui/native_theme/native_theme_switches.h"
 #include "ui/views/views_switches.h"
 
@@ skipping 12 matching lines @@
 #endif
 
 #if defined(ENABLE_APP_LIST)
 #include "ui/app_list/app_list_switches.h"
 #endif
 
 using base::UserMetricsAction;
 
 namespace about_flags {
 
+const uint32_t kBadSwitchFormatHistogramId = 0;
+
 // Macros to simplify specifying the type.
 #define SINGLE_VALUE_TYPE_AND_VALUE(command_line_switch, switch_value) \
     Experiment::SINGLE_VALUE, \
     command_line_switch, switch_value, NULL, NULL, NULL, 0
 #define SINGLE_VALUE_TYPE(command_line_switch) \
     SINGLE_VALUE_TYPE_AND_VALUE(command_line_switch, "")
 #define ENABLE_DISABLE_VALUE_TYPE_AND_VALUE(enable_switch, enable_value, \
                                             disable_switch, disable_value) \
     Experiment::ENABLE_DISABLE_VALUE, enable_switch, enable_value, \
     disable_switch, disable_value, NULL, 3
@@ skipping 1435 matching lines @@
   },
 #endif
 #if defined(OS_ANDROID)
   {
     "enable-accessibility-tab-switcher",
     IDS_FLAGS_ENABLE_ACCESSIBILITY_TAB_SWITCHER_NAME,
     IDS_FLAGS_ENABLE_ACCESSIBILITY_TAB_SWITCHER_DESCRIPTION,
     kOsAndroid,
     SINGLE_VALUE_TYPE(switches::kEnableAccessibilityTabSwitcher)
   },
-  {
-    // TODO(dmazzoni): remove this flag when native android accessibility
-    // ships in the stable channel. http://crbug.com/356775
-    "enable-accessibility-script-injection",
-    IDS_FLAGS_ENABLE_ACCESSIBILITY_SCRIPT_INJECTION_NAME,
-    IDS_FLAGS_ENABLE_ACCESSIBILITY_SCRIPT_INJECTION_DESCRIPTION,
-    kOsAndroid,
-    // Java-only switch: ContentSwitches.ENABLE_ACCESSIBILITY_SCRIPT_INJECTION.
-    SINGLE_VALUE_TYPE("enable-accessibility-script-injection")
-  },

[Ilya Sherman, 2014/08/05 20:14:46]

This diff seems unrelated to your CL.

[Alexander Alekseev, 2014/08/07 23:24:55]

As this CL doesn't have explicit UMA IDs, this part looks irrelevant.
But at the beginning we agreed that I would drop this (unused) part,
and it saves extra ID from UMA.

Do you think we should reserve ID for this switch?

[Ilya Sherman, 2014/08/08 03:49:45]

If this switch is obsolete, I'd prefer that you remove it in a separate, tiny
CL, since it's easy for the change to get lost in a large CL like this one.

[Alexander Alekseev, 2014/08/09 01:30:00]

Done.

 #endif
   {
     "enable-one-copy",
     IDS_FLAGS_ONE_COPY_NAME,
     IDS_FLAGS_ONE_COPY_DESCRIPTION,
     kOsAll,
     SINGLE_VALUE_TYPE(switches::kEnableOneCopy)
   },
   {
     "enable-zero-copy",
@@ skipping 408 matching lines @@
                               SentinelsMode sentinels);
   bool IsRestartNeededToCommitChanges();
   void SetExperimentEnabled(
       FlagsStorage* flags_storage,
       const std::string& internal_name,
       bool enable);
   void RemoveFlagsSwitches(
       std::map<std::string, CommandLine::StringType>* switch_list);
   void ResetAllFlags(FlagsStorage* flags_storage);
   void reset();
+  std::set<std::string> GetAllSwitchesForTesting() const;
 
   // Returns the singleton instance of this class
   static FlagsState* GetInstance() {
     return Singleton<FlagsState>::get();
   }
 
  private:
   bool needs_restart_;
   std::map<std::string, std::string> flags_switches_;
 
@@ skipping 170 matching lines @@
 
 void ConvertFlagsToSwitches(FlagsStorage* flags_storage,
                             CommandLine* command_line,
                             SentinelsMode sentinels) {
   FlagsState::GetInstance()->ConvertFlagsToSwitches(flags_storage,
                                                     command_line,
                                                     sentinels);
 }
 
 bool AreSwitchesIdenticalToCurrentCommandLine(
-    const CommandLine& new_cmdline, const CommandLine& active_cmdline) {
+    const CommandLine& new_cmdline,
+    const CommandLine& active_cmdline,
+    std::set<CommandLine::StringType>* out_difference) {
   std::set<CommandLine::StringType> new_flags =
       ExtractFlagsFromCommandLine(new_cmdline);
   std::set<CommandLine::StringType> active_flags =
       ExtractFlagsFromCommandLine(active_cmdline);
 
+  bool result = false;
   // Needed because std::equal doesn't check if the 2nd set is empty.
-  if (new_flags.size() != active_flags.size())
-    return false;
+  if (new_flags.size() == active_flags.size()) {
+    result =
+        std::equal(new_flags.begin(), new_flags.end(), active_flags.begin());
+  }
 
-  return std::equal(new_flags.begin(), new_flags.end(), active_flags.begin());
+  if (out_difference && !result) {
+    std::set_symmetric_difference(
+        new_flags.begin(),
+        new_flags.end(),
+        active_flags.begin(),
+        active_flags.end(),
+        std::inserter(*out_difference, out_difference->begin()));
+  }
+
+  return result;
 }
 
 void GetFlagsExperimentsData(FlagsStorage* flags_storage,
                              FlagAccess access,
                              base::ListValue* supported_experiments,
                              base::ListValue* unsupported_experiments) {
   std::set<std::string> enabled_experiments;
   GetSanitizedEnabledFlags(flags_storage, &enabled_experiments);
 
   int current_platform = GetCurrentPlatform();
@@ skipping 87 matching lines @@
     action += *it;
     content::RecordComputedAction(action);
   }
   // Since flag metrics are recorded every startup, add a tick so that the
   // stats can be made meaningful.
   if (flags.size())
     content::RecordAction(UserMetricsAction("AboutFlags_StartupTick"));
   content::RecordAction(UserMetricsAction("StartupTick"));
 }
 
+uint32 GetSwitchUMAId(const std::string& switch_name) {
+  return static_cast<unsigned>(

[Ilya Sherman, 2014/08/05 20:14:46]

Please cast directly to the returned type, i.e. uint32.  Also, please use a
checked_cast.

[Alexander Alekseev, 2014/08/07 23:24:56]

Done.

+      crc32(0,

[Ilya Sherman, 2014/08/05 20:14:46]

Optional: md5 is more commonly used for UMA code.  It's fine to use any hash
function you like here, but you might want to at least double-check whether
Dremel natively supports crc32 before using it.

[Alexander Alekseev, 2014/08/07 23:24:56]

On a server-side it looks like uint32, so it doesn't need special suport, does
it?

Also, MD5 doesn't fit 32bit integer, so what ID type should I use here?

[Ilya Sherman, 2014/08/08 03:49:45]

The only reason that special support might be useful is that it's convenient for
users to be able to type something like CRC("--enable-my-feature") in Dremel,
without having to compute the CRC themselves.
For other UMA code, we just trim the MD5 hash as needed.

+            reinterpret_cast<const Bytef*>(switch_name.c_str()),
+            switch_name.length()));
+}
+
+void ReportCustomFlags(const std::string& uma_histogram_hame,
+                       const std::set<std::string>& command_line_difference) {
+  for (std::set<std::string>::const_iterator it =
+           command_line_difference.begin();
+       it != command_line_difference.end();
+       ++it) {
+    int uma_id = about_flags::kBadSwitchFormatHistogramId;
+    if (it->size() > 2) {

[Ilya Sherman, 2014/08/05 20:14:46]

It seems more appropriate to check the actual characters of the prefix, rather
than just its length.

[Alexander Alekseev, 2014/08/07 23:24:56]

Done.

+      // Skip '--' before switch name.
+      std::string switch_name(it->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);
+
+      uma_id = GetSwitchUMAId(switch_name);
+    } else {
+      NOTREACHED() << "ReportCustomFlags(): flag '" << *it
+                   << "' has incorrect format.";
+    }
+    VLOG(1) << "ReportCustomFlags(): histogram='" << uma_histogram_hame << "' '"
+            << *it << "', 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_hame, uma_id);
+  }
+}
+
+std::set<std::string> GetAllSwitchesForTesting() {
+  return FlagsState::GetInstance()->GetAllSwitchesForTesting();
+}
+
 //////////////////////////////////////////////////////////////////////////////
 // FlagsState implementation.
 
 namespace {
 
 typedef std::map<std::string, std::pair<std::string, std::string> >
     NameToSwitchAndValueMap;
 
 void SetFlagToSwitchMapping(const std::string& key,
                             const std::string& switch_name,
@@ skipping 149 matching lines @@
 
   std::set<std::string> no_experiments;
   flags_storage->SetFlags(no_experiments);
 }
 
 void FlagsState::reset() {
   needs_restart_ = false;
   flags_switches_.clear();
 }
 
+std::set<std::string> FlagsState::GetAllSwitchesForTesting() const {
+  std::set<std::string> result;
+
+  for (size_t i = 0; i < num_experiments; ++i) {
+    const Experiment& e = experiments[i];

[Ilya Sherman, 2014/08/05 20:14:46]

nit: Please use complete variable names rather than abbreviations.

[Alexander Alekseev, 2014/08/07 23:24:56]

Done.

+    if (e.type == Experiment::SINGLE_VALUE) {
+      result.insert(e.command_line_switch);
+    } else if (e.type == Experiment::MULTI_VALUE) {
+      for (int j = 0; j < e.num_choices; ++j) {
+        result.insert(e.choices[j].command_line_switch);
+      }
+    } else {
+      DCHECK_EQ(e.type, Experiment::ENABLE_DISABLE_VALUE);
+      result.insert(e.command_line_switch);
+      result.insert(e.disable_command_line_switch);
+    }
+  }
+  return result;
+}

[Ilya Sherman, 2014/08/05 20:14:46]

Why isn't this in the testing namespace below?  In fact, it looks like you could
just use GetExperiments() from below, and then implement this function in the
test file.

[Alexander Alekseev, 2014/08/07 23:24:56]

Done.

+
 }  // namespace
 
 namespace testing {
 
 // WARNING: '@' is also used in the html file. If you update this constant you
 // also need to update the html file.
 const char kMultiSeparator[] = "@";
 
 void ClearState() {
   FlagsState::GetInstance()->reset();
@@ skipping 10 matching lines @@
 }
 
 const Experiment* GetExperiments(size_t* count) {
   *count = num_experiments;
   return experiments;
 }
 
 }  // namespace testing
 
 }  // namespace about_flags