about_flags.cc entry for WebRTC-H264WithOpenH264FFmpeg

chrome/browser/about_flags.cc

Index: chrome/browser/about_flags.cc
diff --git a/chrome/browser/about_flags.cc b/chrome/browser/about_flags.cc
index 192be1f264cd9115e9f4e717dfe13f088558b4df..85cd0e741abe6f6a0ee0b2f22a26e21c2b433cc1 100644
--- a/chrome/browser/about_flags.cc
+++ b/chrome/browser/about_flags.cc
@@ -56,6 +56,8 @@
 #include "content/public/browser/user_metrics.h"
 #include "content/public/common/content_features.h"

[hbos_chromium, 2016/02/25 15:20:16]

(I noticed content/public/common/features.h is a list of base::Features. I can
try to put kWebRtcH264WithOpenH264FFmpeg in it instead of having it in its own
header file and build target, but I don't want to do that in this CL.)

[jochen (gone - plz use gerrit), 2016/02/26 12:09:00]

those are runtime enabled features

[hbos_chromium, 2016/02/26 14:13:34]

*Meant to say content_features.h is a list of base::Features.
Which are runtime enabled features, just like extern const base::Feature
kWebRtcH264WithOpenH264FFmpeg.
(A bit confusing that both Feature and BUILDFLAG are referred to as features,
buildflag_features.h for features.h might have been a better name.)

 #include "content/public/common/content_switches.h"
+#include "content/public/common/feature_h264_with_openh264_ffmpeg.h"
+#include "content/public/common/features.h"
 #include "grit/components_strings.h"
 #include "media/base/media_switches.h"
 #include "media/midi/midi_switches.h"
@@ -1801,13 +1803,20 @@ const FeatureEntry kFeatureEntries[] = {
 #if defined(ENABLE_EXTENSIONS)
     {"enable-tab-for-desktop-share", IDS_FLAG_ENABLE_TAB_FOR_DESKTOP_SHARE,
      IDS_FLAG_ENABLE_TAB_FOR_DESKTOP_SHARE_DESCRIPTION, kOsAll,
-     SINGLE_VALUE_TYPE(extensions::switches::kEnableTabForDesktopShare)}
+     SINGLE_VALUE_TYPE(extensions::switches::kEnableTabForDesktopShare)},
 #endif
 #if defined(OS_ANDROID)
     {"enable-ntp-snippets", IDS_FLAGS_ENABLE_NTP_SNIPPETS_NAME,
      IDS_FLAGS_ENABLE_NTP_SNIPPETS_DESCRIPTION, kOsAndroid,
      FEATURE_VALUE_TYPE(chrome::android::kNTPSnippetsFeature)},
 #endif  // defined(OS_ANDROID)
+#if defined(ENABLE_WEBRTC) && BUILDFLAG(RTC_USE_H264)
+    {"enable-webrtc-h264-with-openh264-ffmpeg",
+     IDS_FLAGS_WEBRTC_H264_WITH_OPENH264_FFMPEG_NAME,
+     IDS_FLAGS_WEBRTC_H264_WITH_OPENH264_FFMPEG_DESCRIPTION,
+     kOsMac | kOsWin | kOsLinux | kOsCrOS,
+     FEATURE_VALUE_TYPE(content::kWebRtcH264WithOpenH264FFmpeg)},
+#endif  // defined(ENABLE_WEBRTC) && BUILDFLAG(RTC_USE_H264)
     // NOTE: Adding new command-line switches requires adding corresponding
     // entries to enum "LoginCustomFlags" in histograms.xml. See note in

[jochen (gone - plz use gerrit), 2016/02/26 12:09:00]

wow, such comment


    so little attention payed


 much important

[hbos_chromium, 2016/02/26 14:13:34]

Lol, thanks. Either missed it or thought it didn't apply to features, wow so
little attention payed regardless.

So, should this be done for base::Features (--enable-features=...) or only
something you do for old switches (base::CommandLine::HasSwitch)?

Because I looked at three FEATURE_VALUE_TYPE examples...
(1) "enable-webfonts-intervention" aka base::Feature
kWebFontsIntervention{"WebFontsIntervention",...}
(2) "enable-scroll-anchoring" aka base::Feature
kScrollAnchoring{"ScrollAnchoring",...}
(3) "enable-token-binding" aka base::Feature kTokenBinding{"token-binding",...}

And only (1) was listed in histograms.xml under the name
"enable-webfonts-intervention". (2) and (3) were not list under either name
(about_flags.cc name or base::Feature::name). Was this a mistake?

I added to histograms.xml with label="enable-webrtc-h264-with-openh264-ffmpeg"
because that's most like (1), but the command line switch is really
--enable-features=WebRTC-H264WithOpenH264FFmpeg.

Is this right?

[rkaplow, 2016/03/01 19:17:28]

Not entirely sure. I would guess 2/3 were omissions here. 
The name seems like it should be "enable-webrtc-h264-with-openh264-ffmpeg" since
that's how it is defined here.


Will loop in alexei who should be able to confirm. 

     // histograms.xml and don't forget to run AboutFlagsHistogramTest unit test.