|
|
Created:
4 years, 10 months ago by hbos_chromium Modified:
4 years, 9 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionabout_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}
Patch Set 1 : #Patch Set 2 : nit, moved flag to bottom of about_flags list #
Total comments: 9
Patch Set 3 : wow, histograms.xml enum updated to include feature #Patch Set 4 : Not updating histograms.xml #
Total comments: 4
Patch Set 5 : Rebase with master #Patch Set 6 : .grd if expr matching about_flags.cc #if. Use of kOsDesktop #
Total comments: 2
Messages
Total messages: 35 (13 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
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.org changed reviewers: + jochen@chromium.org, rkaplow@chromium.org
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.)
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
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?
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@chromium.org changed reviewers: + asvitkine@google.com
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@chromium.org changed reviewers: + asvitkine@chromium.org - asvitkine@google.com
er, switching to chromium account
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.
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.
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?
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.
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?
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.org changed reviewers: + flackr@chromium.org
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)
lgtm
grit_rule.gni lgtm
lgtm
PTAL jochen :)
lgtm
The CQ bit was checked by hbos@chromium.org
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
Message was sent while issue was closed.
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 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:200001)
Message was sent while issue was closed.
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} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/8018fbdf7bd32858d8926772e54c9ee20227d403 Cr-Commit-Position: refs/heads/master@{#379284}
Message was sent while issue was closed.
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). |