Fix logging of Launch.FlagsAtStartup histogram.

components/flags_ui/flags_state.h

 // Copyright 2015 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.
 
 #ifndef COMPONENTS_FLAGS_UI_FLAGS_STATE_H_
 #define COMPONENTS_FLAGS_UI_FLAGS_STATE_H_
 
 #include <stddef.h>
 
 #include <map>
@@ skipping 55 matching lines @@
   ~FlagsState();
 
   // Reads the state from |flags_storage| and adds the command line flags
   // belonging to the active feature entries to |command_line|. Features are
   // appended via |enable_features_flag_name| and |disable_features_flag_name|.
   void ConvertFlagsToSwitches(FlagsStorage* flags_storage,
                               base::CommandLine* command_line,
                               SentinelsMode sentinels,
                               const char* enable_features_flag_name,
                               const char* disable_features_flag_name);
+
+  // Reads the state from |flags_storage| and adds the command line flags that
+  // correspond to enabled entries to |switches|. Does not populate any
+  // information about entries that enable/disable base::Feature states.

[Ilya Sherman, 2016/08/31 19:06:31]

Hmm.  I would expect that over time, the majority of the entries in about:flags
will correspond to base::Features, and those features might not always have an
active experiment associate with them.  Should we log a separate histogram for
enabled/disabled Features?

[Alexei Svitkine (slow), 2016/08/31 19:46:22]

Agreed. Filed a bug for it here: crbug.com/642858

+  void GetSwitchesFromFlags(FlagsStorage* flags_storage,

[Ilya Sherman, 2016/08/31 19:06:31]

nit: Could this be passed by const-ref?

[Alexei Svitkine (slow), 2016/08/31 19:46:22]

I checked, but it's not possible because it eventually calls
GetSanitizedEnabledFlags() which changes FlagsStorage.

+                            std::set<std::string>* switches);

[Ilya Sherman, 2016/08/31 19:06:31]

nit: Please return the set, rather than writing to it as an outparam.

[Alexei Svitkine (slow), 2016/08/31 19:46:22]

Done.

+
   bool IsRestartNeededToCommitChanges();
   void SetFeatureEntryEnabled(FlagsStorage* flags_storage,
                               const std::string& internal_name,
                               bool enable);
   void RemoveFlagsSwitches(
       std::map<std::string, base::CommandLine::StringType>* switch_list);
   void ResetAllFlags(FlagsStorage* flags_storage);
   void Reset();
 
   // Registers variations parameter values selected for features in about:flags.
@@ skipping 29 matching lines @@
   // compared. The embedder may use |extra_flag_sentinel_begin_flag_name| and
   // |extra_sentinel_end_flag_name| to specify other delimiters, if supported.
   static bool AreSwitchesIdenticalToCurrentCommandLine(
       const base::CommandLine& new_cmdline,
       const base::CommandLine& active_cmdline,
       std::set<base::CommandLine::StringType>* out_difference,
       const char* extra_flag_sentinel_begin_flag_name,
       const char* extra_flag_sentinel_end_flag_name);
 
  private:
+  // Keeps track of affected switches for each FeatureEntry, based on which
+  // choice is selected for it.
+  struct SwitchEntry {
+    // Corresponding base::Feature to toggle.
+    std::string feature_name;
+
+    // If |feature_name| is not empty, the state (enable/disabled) to set.
+    bool feature_state;
+
+    // The name of the switch to add.
+    std::string switch_name;
+
+    // If |switch_name| is not empty, the value of the switch to set.
+    std::string switch_value;
+
+    SwitchEntry();
+    ~SwitchEntry();
+  };

[Ilya Sherman, 2016/08/31 19:06:31]

Optional nit: FWIW, you could probably just declare "struct SwitchEntry;" in
this header file, and define the struct in the .cc file.  I'm not actually sure
which option is preferred.

[Alexei Svitkine (slow), 2016/08/31 19:46:22]

Done. Interestingly, I could keep the order of declaration in the .cc file since
the definition had go above the public functions that use this struct.

+
   // Adds mapping to |name_to_switch_map| to set the given switch name/value.
   void AddSwitchMapping(const std::string& key,
                         const std::string& switch_name,
                         const std::string& switch_value,
                         std::map<std::string, SwitchEntry>* name_to_switch_map);
 
   // Adds mapping to |name_to_switch_map| to toggle base::Feature |feature_name|
   // to state |feature_state|.
   void AddFeatureMapping(
       const std::string& key,
@@ skipping 29 matching lines @@
 
   void GetSanitizedEnabledFlags(FlagsStorage* flags_storage,
                                 std::set<std::string>* result);
 
   // Variant of GetSanitizedEnabledFlags that also removes any flags that aren't
   // enabled on the current platform.
   void GetSanitizedEnabledFlagsForCurrentPlatform(
       FlagsStorage* flags_storage,
       std::set<std::string>* result);
 
+  // Generates a flags to switches mapping based on the set of enabled flags
+  // from |flags_storage|. On output, |enable_entries| will contain the internal

[Ilya Sherman, 2016/08/31 19:06:31]

nit: s/enable_entries/enabled_entries

[Alexei Svitkine (slow), 2016/08/31 19:46:22]

Done.

+  // names of enabled flags and |name_to_switch_map| will contain information
+  // on how they map to command-line flags or features.
+  void GenerateFlagsToSwitchesMapping(
+      FlagsStorage* flags_storage,
+      std::set<std::string>* enabled_entries,
+      std::map<std::string, SwitchEntry>* name_to_switch_map);
+
   const FeatureEntry* feature_entries_;
   size_t num_feature_entries_;
 
   bool needs_restart_;
   std::map<std::string, std::string> flags_switches_;
 
   // Map from switch name to a set of string, that keeps track which strings
   // were appended to existing (list value) switches.
   std::map<std::string, std::set<std::string>> appended_switches_;
 
   DISALLOW_COPY_AND_ASSIGN(FlagsState);
 };
 
 }  // namespace flags_ui
 
 #endif  // COMPONENTS_FLAGS_UI_FLAGS_STATE_H_