about_flags.cc entry for WebRTC-H264WithOpenH264FFmpeg

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>
@@ skipping 36 matching lines @@
 #include "components/password_manager/core/common/password_manager_features.h"
 #include "components/password_manager/core/common/password_manager_switches.h"
 #include "components/proximity_auth/switches.h"
 #include "components/search/search_switches.h"
 #include "components/security_state/switches.h"
 #include "components/signin/core/common/signin_switches.h"
 #include "components/sync_driver/sync_driver_switches.h"
 #include "components/tracing/tracing_switches.h"
 #include "components/version_info/version_info.h"
 #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"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/ui_base_switches.h"
 #include "ui/compositor/compositor_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"
@@ skipping 1725 matching lines @@
                      switches::kUIDisableCompositorAnimationTimelines)},
 #endif  // !defined(OS_ANDROID)
     {"enable-audio-support-for-desktop-share",
      IDS_FLAG_ENABLE_AUDIO_FOR_DESKTOP_SHARE,
      IDS_FLAG_ENABLE_AUDIO_FOR_DESKTOP_SHARE_DESCRIPTION,
      kOsAll,
      SINGLE_VALUE_TYPE(switches::kEnableAudioSupportForDesktopShare)},
 #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.
 };
 
 class FlagsStateSingleton {
  public:
   FlagsStateSingleton()
       : flags_state_(kFeatureEntries, arraysize(kFeatureEntries)) {}
   ~FlagsStateSingleton() {}
 
   static FlagsStateSingleton* GetInstance() {
@@ skipping 199 matching lines @@
 const base::HistogramBase::Sample kBadSwitchFormatHistogramId = 0;
 
 const FeatureEntry* GetFeatureEntries(size_t* count) {
   *count = arraysize(kFeatureEntries);
   return kFeatureEntries;
 }
 
 }  // namespace testing
 
 }  // namespace about_flags