about_flags.cc entry for WebRTC-H264WithOpenH264FFmpeg

about_flags.cc entry for WebRTC-H264WithOpenH264FFmpeg
(base::Feature kWebRtcH264WithOpenH264FFmpeg).

This is a feature that enables a H.264 encoder/decoder in
WebRTC. The feature is off by default.

BUG=chromium:500605, chromium:468365
Committed: https://crrev.com/8018fbdf7bd32858d8926772e54c9ee20227d403
Cr-Commit-Position: refs/heads/master@{#379284}

[hbos_chromium, 2016-02-24 13:01:06]

Patchset #1 (id:1) has been deleted

[hbos_chromium, 2016-02-24 14:14:56]

Patchset #1 (id:20001) has been deleted

[hbos_chromium, 2016-02-25 14:33:59]

Patchset #1 (id:40001) has been deleted

[hbos_chromium, 2016-02-25 14:40:00]

Patchset #2 (id:80001) has been deleted

[hbos_chromium, 2016-02-25 14:42:08]

Patchset #2 (id:100001) has been deleted

[hbos_chromium, 2016-02-25 14:51:30]

Description was changed from

==========
about_flags.cc entry for kWebRtcH264WithOpenH264FFmpeg

WIP

COMMIT=False
BUG=
==========

to

==========
about_flags.cc entry for WebRTC-H264WithOpenH264FFmpeg
(base::Feature kWebRtcH264WithOpenH264FFmpeg).

This is a feature that enables a H.264 encoder/decoder in
WebRTC. The feature is off by default.

BUG=chromium:500605, chromium:468365
==========

[hbos_chromium, 2016-02-25 15:20:15]

hbos@chromium.org changed reviewers:
+ jochen@chromium.org, rkaplow@chromium.org

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

Please take a look rkaplow and jochen.

https://codereview.chromium.org/1722283002/diff/120001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1722283002/diff/120001/chrome/app/generated_r...
chrome/app/generated_resources.grd:5528: +      <if expr="not is_android and not
is_ios">
Is it right or wrong to have these ifs here? The feature is not supported on
android/ios, but unsupported experiments are still listed as unsupported, so
these strings would still be included, yeah?

https://codereview.chromium.org/1722283002/diff/120001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/1722283002/diff/120001/chrome/browser/BUILD.g...
chrome/browser/BUILD.gn:191:
"//content/public/common:feature_h264_with_openh264_ffmpeg",
(Target for base::Feature kWebRtcH264WithOpenH264FFmpeg.)

https://codereview.chromium.org/1722283002/diff/120001/chrome/browser/BUILD.g...
chrome/browser/BUILD.gn:192: "//content/public/common:features",
(Target for BUILDFLAG(RTC_USE_H264).)

https://codereview.chromium.org/1722283002/diff/120001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1722283002/diff/120001/chrome/browser/about_f...
chrome/browser/about_flags.cc:57: #include
"content/public/common/content_features.h"
(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:01]

https://codereview.chromium.org/1722283002/diff/120001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1722283002/diff/120001/chrome/browser/about_f...
chrome/browser/about_flags.cc:57: #include
"content/public/common/content_features.h"
On 2016/02/25 at 15:20:16, hbos_chromium wrote:
> (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.)

those are runtime enabled features

https://codereview.chromium.org/1722283002/diff/120001/chrome/browser/about_f...
chrome/browser/about_flags.cc:1821: // entries to enum "LoginCustomFlags" in
histograms.xml. See note in
wow, such comment


    so little attention payed


 much important

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

https://codereview.chromium.org/1722283002/diff/120001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1722283002/diff/120001/chrome/browser/about_f...
chrome/browser/about_flags.cc:57: #include
"content/public/common/content_features.h"
On 2016/02/26 12:09:00, jochen wrote:
> On 2016/02/25 at 15:20:16, hbos_chromium wrote:
> > (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.)
> 
> those are runtime enabled features

*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.)

https://codereview.chromium.org/1722283002/diff/120001/chrome/browser/about_f...
chrome/browser/about_flags.cc:1821: // entries to enum "LoginCustomFlags" in
histograms.xml. See note in
On 2016/02/26 12:09:00, jochen wrote:
> wow, such comment
> 
> 
>     so little attention payed
> 
> 
>  much important

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?

[jochen (gone - plz use gerrit), 2016-03-01 14:14:55]

On 2016/02/26 at 14:13:34, hbos wrote:
>
https://codereview.chromium.org/1722283002/diff/120001/chrome/browser/about_f...
> File chrome/browser/about_flags.cc (right):
> 
>
https://codereview.chromium.org/1722283002/diff/120001/chrome/browser/about_f...
> chrome/browser/about_flags.cc:57: #include
"content/public/common/content_features.h"
> On 2016/02/26 12:09:00, jochen wrote:
> > On 2016/02/25 at 15:20:16, hbos_chromium wrote:
> > > (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.)
> > 
> > those are runtime enabled features
> 
> *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.)
> 
>
https://codereview.chromium.org/1722283002/diff/120001/chrome/browser/about_f...
> chrome/browser/about_flags.cc:1821: // entries to enum "LoginCustomFlags" in
histograms.xml. See note in
> On 2016/02/26 12:09:00, jochen wrote:
> > wow, such comment
> > 
> > 
> >     so little attention payed
> > 
> > 
> >  much important
> 
> 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?

I actually don't know. Maybe you can git blame a base::Features person to shed
some light on this?

[rkaplow, 2016-03-01 19:17:27]

rkaplow@chromium.org changed reviewers:
+ asvitkine@google.com

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

https://codereview.chromium.org/1722283002/diff/120001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1722283002/diff/120001/chrome/browser/about_f...
chrome/browser/about_flags.cc:1821: // entries to enum "LoginCustomFlags" in
histograms.xml. See note in
On 2016/02/26 14:13:34, hbos_chromium wrote:
> On 2016/02/26 12:09:00, jochen wrote:
> > wow, such comment
> > 
> > 
> >     so little attention payed
> > 
> > 
> >  much important
> 
> 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?

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.

[rkaplow, 2016-03-01 19:17:42]

rkaplow@chromium.org changed reviewers:
+ asvitkine@chromium.org
- asvitkine@google.com

[rkaplow, 2016-03-01 19:17:43]

er, switching to chromium account

[Alexei Svitkine (slow), 2016-03-01 19:37:21]

I agree that it would be better to have it in content_features.h.

In terms of the about:flags histogram, it currently doesn't exist for
base::Feature entries - so no need to update the histograms.xml when adding a
Feature entry to about_flags.cc.

[hbos_chromium, 2016-03-02 09:54:42]

PTAL guys

On 2016/03/01 19:37:21, Alexei Svitkine (slow) wrote:
> I agree that it would be better to have it in content_features.h.
> 
> In terms of the about:flags histogram, it currently doesn't exist for
> base::Feature entries - so no need to update the histograms.xml when adding a
> Feature entry to about_flags.cc.

In that case, removed the histograms.xml change. The content_features.h move
I'll do in a separate CL.

[jochen (gone - plz use gerrit), 2016-03-02 11:03:42]

https://codereview.chromium.org/1722283002/diff/160001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1722283002/diff/160001/chrome/browser/about_f...
chrome/browser/about_flags.cc:1813: #if defined(ENABLE_WEBRTC) &&
BUILDFLAG(RTC_USE_H264)
how bad is it that the generated resources if block does not match these
defines?

[hbos_chromium, 2016-03-02 12:54:56]

https://codereview.chromium.org/1722283002/diff/160001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1722283002/diff/160001/chrome/browser/about_f...
chrome/browser/about_flags.cc:1813: #if defined(ENABLE_WEBRTC) &&
BUILDFLAG(RTC_USE_H264)
On 2016/03/02 11:03:42, jochen wrote:
> how bad is it that the generated resources if block does not match these
> defines?

Not sure, but the other WebRTC features which are also behind #if
defined(ENABLE_WEBRTC) have no corresponding if in the generated_resources.grd
file.

The generated_resources.grd if's are mostly about the OS. I don't know the
significance of this considering even if its not supported on the current OS the
feature is still listed as a an unsupported experiment in chrome://flags,
including the generated resources texts that are if'd.

[hbos_chromium, 2016-03-02 15:37:53]

On 2016/03/02 12:54:56, hbos_chromium wrote:
>
https://codereview.chromium.org/1722283002/diff/160001/chrome/browser/about_f...
> File chrome/browser/about_flags.cc (right):
> 
>
https://codereview.chromium.org/1722283002/diff/160001/chrome/browser/about_f...
> chrome/browser/about_flags.cc:1813: #if defined(ENABLE_WEBRTC) &&
> BUILDFLAG(RTC_USE_H264)
> On 2016/03/02 11:03:42, jochen wrote:
> > how bad is it that the generated resources if block does not match these
> > defines?
> 
> Not sure, but the other WebRTC features which are also behind #if
> defined(ENABLE_WEBRTC) have no corresponding if in the generated_resources.grd
> file.
> 
> The generated_resources.grd if's are mostly about the OS. I don't know the
> significance of this considering even if its not supported on the current OS
the
> feature is still listed as a an unsupported experiment in chrome://flags,
> including the generated resources texts that are if'd.

Do any of you know, rkaplow / asvitknie?

[Alexei Svitkine (slow), 2016-03-02 17:29:47]

https://codereview.chromium.org/1722283002/diff/160001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1722283002/diff/160001/chrome/browser/about_f...
chrome/browser/about_flags.cc:1813: #if defined(ENABLE_WEBRTC) &&
BUILDFLAG(RTC_USE_H264)
On 2016/03/02 12:54:56, hbos_chromium wrote:
> On 2016/03/02 11:03:42, jochen wrote:
> > how bad is it that the generated resources if block does not match these
> > defines?
> 
> Not sure, but the other WebRTC features which are also behind #if
> defined(ENABLE_WEBRTC) have no corresponding if in the generated_resources.grd
> file.
> 
> The generated_resources.grd if's are mostly about the OS. I don't know the
> significance of this considering even if its not supported on the current OS
the
> feature is still listed as a an unsupported experiment in chrome://flags,
> including the generated resources texts that are if'd.

I think if the generated resources if doesn't match, the end result is these
strings being part of the build for platforms that don't support them - which
does incur bloat. So if it's easy, better to make it match, but not the end of
the world if it's tricky.

[hbos_chromium, 2016-03-03 16:51:41]

hbos@chromium.org changed reviewers:
+ flackr@chromium.org

[hbos_chromium, 2016-03-03 16:51:42]

PTAL rkaplow, asvitkine, jochen and +flackr for grit_rule.gni.

https://codereview.chromium.org/1722283002/diff/160001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1722283002/diff/160001/chrome/browser/about_f...
chrome/browser/about_flags.cc:1813: #if defined(ENABLE_WEBRTC) &&
BUILDFLAG(RTC_USE_H264)
On 2016/03/02 17:29:46, Alexei Svitkine (slow) wrote:
> On 2016/03/02 12:54:56, hbos_chromium wrote:
> > On 2016/03/02 11:03:42, jochen wrote:
> > > how bad is it that the generated resources if block does not match these
> > > defines?
> > 
> > Not sure, but the other WebRTC features which are also behind #if
> > defined(ENABLE_WEBRTC) have no corresponding if in the
generated_resources.grd
> > file.
> > 
> > The generated_resources.grd if's are mostly about the OS. I don't know the
> > significance of this considering even if its not supported on the current OS
> the
> > feature is still listed as a an unsupported experiment in chrome://flags,
> > including the generated resources texts that are if'd.
> 
> I think if the generated resources if doesn't match, the end result is these
> strings being part of the build for platforms that don't support them - which
> does incur bloat. So if it's easy, better to make it match, but not the end of
> the world if it's tricky.

Done. It was doable. Matching .grd with the #if, replacing the previous if-expr
matching the kOs.. stuff which was incorrect.

https://codereview.chromium.org/1722283002/diff/200001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1722283002/diff/200001/build/common.gypi#newc...
build/common.gypi:2184: '../third_party/webrtc/build/common.gypi',
(For gyp variable rtc_use_h264)

https://codereview.chromium.org/1722283002/diff/200001/tools/grit/grit_rule.gni
File tools/grit/grit_rule.gni (right):

https://codereview.chromium.org/1722283002/diff/200001/tools/grit/grit_rule.g...
tools/grit/grit_rule.gni:252: import("//third_party/webrtc/build/webrtc.gni")
(For GN variable rtc_use_h264)

[Alexei Svitkine (slow), 2016-03-03 18:14:40]

lgtm

[flackr, 2016-03-03 18:23:02]

grit_rule.gni lgtm

[rkaplow, 2016-03-03 20:35:50]

lgtm

[hbos_chromium, 2016-03-04 09:01:04]

PTAL jochen :)

[jochen (gone - plz use gerrit), 2016-03-04 13:01:25]

lgtm

[hbos_chromium, 2016-03-04 13:01:52]

The CQ bit was checked by hbos@chromium.org

[commit-bot: I haz the power, 2016-03-04 13:01:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1722283002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1722283002/200001

[commit-bot: I haz the power, 2016-03-04 14:38:58]

Description was changed from

==========
about_flags.cc entry for WebRTC-H264WithOpenH264FFmpeg
(base::Feature kWebRtcH264WithOpenH264FFmpeg).

This is a feature that enables a H.264 encoder/decoder in
WebRTC. The feature is off by default.

BUG=chromium:500605, chromium:468365
==========

to

==========
about_flags.cc entry for WebRTC-H264WithOpenH264FFmpeg
(base::Feature kWebRtcH264WithOpenH264FFmpeg).

This is a feature that enables a H.264 encoder/decoder in
WebRTC. The feature is off by default.

BUG=chromium:500605, chromium:468365
==========

[commit-bot: I haz the power, 2016-03-04 14:38:59]

Committed patchset #6 (id:200001)

[commit-bot: I haz the power, 2016-03-04 14:40:18]

Description was changed from

==========
about_flags.cc entry for WebRTC-H264WithOpenH264FFmpeg
(base::Feature kWebRtcH264WithOpenH264FFmpeg).

This is a feature that enables a H.264 encoder/decoder in
WebRTC. The feature is off by default.

BUG=chromium:500605, chromium:468365
==========

to

==========
about_flags.cc entry for WebRTC-H264WithOpenH264FFmpeg
(base::Feature kWebRtcH264WithOpenH264FFmpeg).

This is a feature that enables a H.264 encoder/decoder in
WebRTC. The feature is off by default.

BUG=chromium:500605, chromium:468365

Committed: https://crrev.com/8018fbdf7bd32858d8926772e54c9ee20227d403
Cr-Commit-Position: refs/heads/master@{#379284}
==========

[commit-bot: I haz the power, 2016-03-04 14:40:19]

Patchset 6 (id:??) landed as
https://crrev.com/8018fbdf7bd32858d8926772e54c9ee20227d403
Cr-Commit-Position: refs/heads/master@{#379284}

[kjellander_chromium, 2016-03-06 09:47:07]

On 2016/03/04 14:40:19, commit-bot: I haz the power wrote:
> Patchset 6 (id:??) landed as
> https://crrev.com/8018fbdf7bd32858d8926772e54c9ee20227d403
> Cr-Commit-Position: refs/heads/master@{#379284}

A revert of this was created in https://codereview.chromium.org/1770683002/
(had to do it manually since Rietveld cannot handle the large
chrome/app/generated_resources.grd file).