Fix logging of Launch.FlagsAtStartup histogram.

components/flags_ui/flags_state.h

Index: components/flags_ui/flags_state.h
diff --git a/components/flags_ui/flags_state.h b/components/flags_ui/flags_state.h
index 7d7e933e9b087a6c70fa26c36ff15227c54f5e6a..9d223bec69b669773ed4fbb16bde7e27acb364fb 100644
--- a/components/flags_ui/flags_state.h
+++ b/components/flags_ui/flags_state.h
@@ -73,6 +73,13 @@ class FlagsState {
                               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,
@@ -122,6 +129,25 @@ class FlagsState {
       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,
@@ -171,6 +197,15 @@ class FlagsState {
       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_;