|
|
DescriptionAdd a use counter for RtcpMuxPolicy of "negotiate".
When parsing the RTCConfiguration, if the RtcpMuxPolicy is "negotiate",
it will be counted. The number will be used to estimate the risk of
removing this feature.
Intent to deprecate thread: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/OP2SGSWF5lo
BUG=chromium:685727
Review-Url: https://codereview.chromium.org/2679093006
Cr-Commit-Position: refs/heads/master@{#451162}
Committed: https://chromium.googlesource.com/chromium/src/+/887a0d5ac980c21eebecb133a75e08dc2fe048bf
Patch Set 1 #
Total comments: 2
Patch Set 2 : Modified the deprecate message. #
Total comments: 1
Patch Set 3 : CR comments. #
Total comments: 1
Patch Set 4 : Remove the deprecation message. #Patch Set 5 : Replace the countDeprecate with UseCounter::count #Patch Set 6 : Merge #
Messages
Total messages: 35 (20 generated)
Description was changed from ========== Add a deprecate message and a counter for RtcpMuxPolicy of "negotiate". When paring the RTCConfiguration, if the RtcpMuxPolicy is "negotiate", a deprecate message will be returned. BUG=chromium:685727 ========== to ========== Add a deprecate message and a counter for RtcpMuxPolicy of "negotiate". When parsing the RTCConfiguration, if the RtcpMuxPolicy is "negotiate", a deprecate message will be returned. BUG=chromium:685727 ==========
deadbeef@chromium.org changed reviewers: + deadbeef@chromium.org
https://codereview.chromium.org/2679093006/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/Deprecation.cpp (right): https://codereview.chromium.org/2679093006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/Deprecation.cpp:421: return "The rtcpMuxPolicy \"negotiate\" will be deprecated. Please use " Should change to "is deprecated"? I believe saying something "is deprecated" is equivalent to saying "is discouraged, may be removed in the future".
Patchset #2 (id:20001) has been deleted
zhihuang@chromium.org changed reviewers: + dcheng@chromium.org, guidou@chromium.org, holte@chromium.org
Hi, Please take a look. Thanks. https://codereview.chromium.org/2679093006/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/Deprecation.cpp (right): https://codereview.chromium.org/2679093006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/Deprecation.cpp:421: return "The rtcpMuxPolicy \"negotiate\" will be deprecated. Please use " On 2017/02/09 00:29:43, Taylor_Brandstetter wrote: > Should change to "is deprecated"? I believe saying something "is deprecated" is > equivalent to saying "is discouraged, may be removed in the future". By applying the template, the message is now " The rtcpMuxPolicy "negotiate" is deprecated. Please use "require" instead. "
histograms lgtm
Can you include a link to the intent-to-deprecate thread in the description?
On 2017/02/09 12:30:19, Guido Urdaneta wrote: > Can you include a link to the intent-to-deprecate thread in the description? We haven't started that thread yet. Should it be in the form of "Intent to Deprecate and Remove"?
rs lgtm https://codereview.chromium.org/2679093006/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/Deprecation.cpp (right): https://codereview.chromium.org/2679093006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/Deprecation.cpp:421: return replacedBy("The rtcpMuxPolicy \"negotiate\"", "\"require\""); Nit: remove "the", it reads a bit awkwardly when interpolated into the format string: %s is deprecated. Please use %s instead.
On 2017/02/09 18:26:47, Taylor_Brandstetter wrote: > On 2017/02/09 12:30:19, Guido Urdaneta wrote: > > Can you include a link to the intent-to-deprecate thread in the description? > > We haven't started that thread yet. Should it be in the form of "Intent to > Deprecate and Remove"? There are "Intent to Deprecate" and "Intent to Deprecate and Remove". The former requires approval from one API owner. The latter requires approval from three API owners. More details here: https://www.chromium.org/blink/removing-features
Description was changed from ========== Add a deprecate message and a counter for RtcpMuxPolicy of "negotiate". When parsing the RTCConfiguration, if the RtcpMuxPolicy is "negotiate", a deprecate message will be returned. BUG=chromium:685727 ========== to ========== Add a deprecate message and a counter for RtcpMuxPolicy of "negotiate". When parsing the RTCConfiguration, if the RtcpMuxPolicy is "negotiate", a deprecate message will be returned. Intent to deprecate thread: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/OP2SGSWF5lo BUG=chromium:685727 ==========
foolip@chromium.org changed reviewers: + foolip@chromium.org
Can you add just the use counter in this CL? For deprecation we would like to also include in the message when the actual removal is going to be, so that developers know it's really going to happen, and can plan their work around the dates given. https://codereview.chromium.org/2679093006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/Deprecation.cpp (right): https://codereview.chromium.org/2679093006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/Deprecation.cpp:421: return replacedBy("rtcpMuxPolicy \"negotiate\"", "\"require\""); If the "negotiate" value is removed, I think we'd end up removing the rtcpMuxPolicy stuff entirely, rather than keeping it around with just one allowed value.
Description was changed from ========== Add a deprecate message and a counter for RtcpMuxPolicy of "negotiate". When parsing the RTCConfiguration, if the RtcpMuxPolicy is "negotiate", a deprecate message will be returned. Intent to deprecate thread: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/OP2SGSWF5lo BUG=chromium:685727 ========== to ========== Add a use counter for RtcpMuxPolicy of "negotiate". When parsing the RTCConfiguration, if the RtcpMuxPolicy is "negotiate", it will be counted. The count will be used to estimate the risk of removing this feature. Intent to deprecate thread: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/OP2SGSWF5lo BUG=chromium:685727 ==========
Description was changed from ========== Add a use counter for RtcpMuxPolicy of "negotiate". When parsing the RTCConfiguration, if the RtcpMuxPolicy is "negotiate", it will be counted. The count will be used to estimate the risk of removing this feature. Intent to deprecate thread: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/OP2SGSWF5lo BUG=chromium:685727 ========== to ========== Add a use counter for RtcpMuxPolicy of "negotiate". When parsing the RTCConfiguration, if the RtcpMuxPolicy is "negotiate", it will be counted. The number will be used to estimate the risk of removing this feature. Intent to deprecate thread: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/OP2SGSWF5lo BUG=chromium:685727 ==========
Description was changed from ========== Add a use counter for RtcpMuxPolicy of "negotiate". When parsing the RTCConfiguration, if the RtcpMuxPolicy is "negotiate", it will be counted. The number will be used to estimate the risk of removing this feature. Intent to deprecate thread: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/OP2SGSWF5lo BUG=chromium:685727 ========== to ========== Add a use counter for RtcpMuxPolicy of "negotiate". When parsing the RTCConfiguration, if the RtcpMuxPolicy is "negotiate", it will be counted. The number will be used to estimate the risk of removing this feature. Intent to deprecate thread: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/OP2SGSWF5lo BUG=chromium:685727 ==========
The CQ bit was checked by zhihuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Please take another look. Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/02/15 06:46:36, foolip wrote: > Can you add just the use counter in this CL? For deprecation we would like to > also include in the message when the actual removal is going to be, so that > developers know it's really going to happen, and can plan their work around the > dates given. > > https://codereview.chromium.org/2679093006/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/frame/Deprecation.cpp (right): > > https://codereview.chromium.org/2679093006/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/Deprecation.cpp:421: return > replacedBy("rtcpMuxPolicy \"negotiate\"", "\"require\""); > If the "negotiate" value is removed, I think we'd end up removing the > rtcpMuxPolicy stuff entirely, rather than keeping it around with just one > allowed value. It seems that I cannot use the "countDeprecation()" without a deprecation message. If we only want to get a number, I can close this issue and create a new UMA histogram for this. Or creating a new one is what you meant?
On 2017/02/15 21:26:02, zhihuang1 wrote: > On 2017/02/15 06:46:36, foolip wrote: > > Can you add just the use counter in this CL? For deprecation we would like to > > also include in the message when the actual removal is going to be, so that > > developers know it's really going to happen, and can plan their work around > the > > dates given. > > > > > https://codereview.chromium.org/2679093006/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/frame/Deprecation.cpp (right): > > > > > https://codereview.chromium.org/2679093006/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/frame/Deprecation.cpp:421: return > > replacedBy("rtcpMuxPolicy \"negotiate\"", "\"require\""); > > If the "negotiate" value is removed, I think we'd end up removing the > > rtcpMuxPolicy stuff entirely, rather than keeping it around with just one > > allowed value. > > It seems that I cannot use the "countDeprecation()" without a deprecation > message. > If we only want to get a number, I can close this issue and create a new UMA > histogram for this. Or creating a new one is what you meant? This lgtm, most use counters are just for measurement and aren't deprecated. For blink-dev intents, use counters are nicer than UMA, because they show up on https://www.chromestatus.com/ (public) and we are kind of calibrated to the way those percentages are calculated.
The CQ bit was checked by zhihuang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org, holte@chromium.org Link to the patchset: https://codereview.chromium.org/2679093006/#ps100001 (title: "Replace the countDeprecate with UseCounter::count")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by zhihuang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org, holte@chromium.org, foolip@chromium.org Link to the patchset: https://codereview.chromium.org/2679093006/#ps120001 (title: "Merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 120001, "attempt_start_ts": 1487277940159290, "parent_rev": "9508e457da7231dd37343e999b81a4260243a4a2", "commit_rev": "887a0d5ac980c21eebecb133a75e08dc2fe048bf"}
Message was sent while issue was closed.
Description was changed from ========== Add a use counter for RtcpMuxPolicy of "negotiate". When parsing the RTCConfiguration, if the RtcpMuxPolicy is "negotiate", it will be counted. The number will be used to estimate the risk of removing this feature. Intent to deprecate thread: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/OP2SGSWF5lo BUG=chromium:685727 ========== to ========== Add a use counter for RtcpMuxPolicy of "negotiate". When parsing the RTCConfiguration, if the RtcpMuxPolicy is "negotiate", it will be counted. The number will be used to estimate the risk of removing this feature. Intent to deprecate thread: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/OP2SGSWF5lo BUG=chromium:685727 Review-Url: https://codereview.chromium.org/2679093006 Cr-Commit-Position: refs/heads/master@{#451162} Committed: https://chromium.googlesource.com/chromium/src/+/887a0d5ac980c21eebecb133a75e... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/chromium/src/+/887a0d5ac980c21eebecb133a75e... |