Fix logging of Launch.FlagsAtStartup histogram.

Fix logging of Launch.FlagsAtStartup histogram.

My initial change was logging the internal value,
rather than the actual switch value. This change
performs the necessary conversion.

BUG=639304
Committed: https://crrev.com/c1a0c4f2a642dbfb107a454dbc322692c7cf57ee
Cr-Commit-Position: refs/heads/master@{#415741}

[Alexei Svitkine (slow), 2016-08-31 16:23:53]

Patchset #1 (id:1) has been deleted

[Alexei Svitkine (slow), 2016-08-31 16:33:36]

The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run

[Alexei Svitkine (slow), 2016-08-31 16:33:49]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-08-31 16:33:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Alexei Svitkine (slow), 2016-08-31 16:34:17]

asvitkine@chromium.org changed reviewers:
+ isherman@chromium.org

[commit-bot: I haz the power, 2016-08-31 16:52:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-31 16:52:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

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

Thanks, Alexei.  LGTM % nits, and there's a comment in there about potentially
wanting to follow up with additional data recording.

https://codereview.chromium.org/2298223002/diff/40001/components/flags_ui/fla...
File components/flags_ui/flags_state.cc (right):

https://codereview.chromium.org/2298223002/diff/40001/components/flags_ui/fla...
components/flags_ui/flags_state.cc:247: 
nit: I'd omit this newline.

https://codereview.chromium.org/2298223002/diff/40001/components/flags_ui/fla...
components/flags_ui/flags_state.cc:267: }
nit: Please replace these four lines with a single DCHECK.

https://codereview.chromium.org/2298223002/diff/40001/components/flags_ui/fla...
File components/flags_ui/flags_state.h (right):

https://codereview.chromium.org/2298223002/diff/40001/components/flags_ui/fla...
components/flags_ui/flags_state.h:79: // information about entries that
enable/disable base::Feature states.
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?

https://codereview.chromium.org/2298223002/diff/40001/components/flags_ui/fla...
components/flags_ui/flags_state.h:80: void GetSwitchesFromFlags(FlagsStorage*
flags_storage,
nit: Could this be passed by const-ref?

https://codereview.chromium.org/2298223002/diff/40001/components/flags_ui/fla...
components/flags_ui/flags_state.h:81: std::set<std::string>* switches);
nit: Please return the set, rather than writing to it as an outparam.

https://codereview.chromium.org/2298223002/diff/40001/components/flags_ui/fla...
components/flags_ui/flags_state.h:149: };
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.

https://codereview.chromium.org/2298223002/diff/40001/components/flags_ui/fla...
components/flags_ui/flags_state.h:201: // from |flags_storage|. On output,
|enable_entries| will contain the internal
nit: s/enable_entries/enabled_entries

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

Thanks! Comments addressed.

https://codereview.chromium.org/2298223002/diff/40001/components/flags_ui/fla...
File components/flags_ui/flags_state.cc (right):

https://codereview.chromium.org/2298223002/diff/40001/components/flags_ui/fla...
components/flags_ui/flags_state.cc:247: 
On 2016/08/31 19:06:31, Ilya Sherman wrote:
> nit: I'd omit this newline.

Done.

https://codereview.chromium.org/2298223002/diff/40001/components/flags_ui/fla...
components/flags_ui/flags_state.cc:267: }
On 2016/08/31 19:06:31, Ilya Sherman wrote:
> nit: Please replace these four lines with a single DCHECK.

Done. DCHECK_NE didn't compile, so I just used a DCHECK.

https://codereview.chromium.org/2298223002/diff/40001/components/flags_ui/fla...
File components/flags_ui/flags_state.h (right):

https://codereview.chromium.org/2298223002/diff/40001/components/flags_ui/fla...
components/flags_ui/flags_state.h:79: // information about entries that
enable/disable base::Feature states.
On 2016/08/31 19:06:31, Ilya Sherman wrote:
> 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?

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

https://codereview.chromium.org/2298223002/diff/40001/components/flags_ui/fla...
components/flags_ui/flags_state.h:80: void GetSwitchesFromFlags(FlagsStorage*
flags_storage,
On 2016/08/31 19:06:31, Ilya Sherman wrote:
> nit: Could this be passed by const-ref?

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

https://codereview.chromium.org/2298223002/diff/40001/components/flags_ui/fla...
components/flags_ui/flags_state.h:81: std::set<std::string>* switches);
On 2016/08/31 19:06:31, Ilya Sherman wrote:
> nit: Please return the set, rather than writing to it as an outparam.

Done.

https://codereview.chromium.org/2298223002/diff/40001/components/flags_ui/fla...
components/flags_ui/flags_state.h:149: };
On 2016/08/31 19:06:31, Ilya Sherman wrote:
> 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.

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.

https://codereview.chromium.org/2298223002/diff/40001/components/flags_ui/fla...
components/flags_ui/flags_state.h:201: // from |flags_storage|. On output,
|enable_entries| will contain the internal
On 2016/08/31 19:06:31, Ilya Sherman wrote:
> nit: s/enable_entries/enabled_entries

Done.

[Alexei Svitkine (slow), 2016-08-31 19:46:54]

The CQ bit was checked by asvitkine@chromium.org

[Alexei Svitkine (slow), 2016-08-31 19:46:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2298223002/#ps60001
(title: "Address comments.")

[commit-bot: I haz the power, 2016-08-31 19:47:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-31 20:37:56]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2016-08-31 20:40:06]

Description was changed from

==========
Fix logging of Launch.FlagsAtStartup histogram.

My initial change was logging the internal value,
rather than the actual switch value. This change
performs the necessary conversion.

BUG=639304
==========

to

==========
Fix logging of Launch.FlagsAtStartup histogram.

My initial change was logging the internal value,
rather than the actual switch value. This change
performs the necessary conversion.

BUG=639304

Committed: https://crrev.com/c1a0c4f2a642dbfb107a454dbc322692c7cf57ee
Cr-Commit-Position: refs/heads/master@{#415741}
==========

[commit-bot: I haz the power, 2016-08-31 20:40:07]

Patchset 2 (id:??) landed as
https://crrev.com/c1a0c4f2a642dbfb107a454dbc322692c7cf57ee
Cr-Commit-Position: refs/heads/master@{#415741}