Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1513483006:
PRESUBMIT: Split NATIVE_API_DIRS into two lists. (Closed)
|
Created:
5 years ago by kjellander_webrtc Modified:
4 years, 6 months ago CC:
webrtc-reviews_webrtc.org, qiang.lu, niklas.enbom, yujie_mao (webrtc), peah-webrtc, Andrew MacDonald Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionPRESUBMIT: Split NATIVE_API_DIRS into two lists.
This should make it more clear what's supported and what's
only checked due to legacy code. No longer uses the word deprecated
since it may be confusing.
BUG=webrtc:5095
NOTRY=True
Committed: https://crrev.com/dd70547adb4e30a5ed9e70cad30f13793b11683d
Cr-Commit-Position: refs/heads/master@{#13094}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Rebased, addressed comments and added webrtc/api, webrtc/media and webrtc/pc #
Total comments: 2
Messages
Total messages: 27 (9 generated)
kjellander@webrtc.org changed reviewers: + pbos@webrtc.org, solenberg@webrtc.org
solenberg@webrtc.org changed reviewers: + holmer@chromium.org
https://codereview.webrtc.org/1513483006/diff/1/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.webrtc.org/1513483006/diff/1/PRESUBMIT.py#newcode35 PRESUBMIT.py:35: 'webrtc/modules/audio_coding/include', Move to LEGACY: webrtc/modules/audio_coding/include webrtc/modules/audio_processing/include webrtc/voice_engine/include Also, I'd like to move: webrtc/modules/bitrate_controller/include webrtc/modules/remote_bitrate_estimator/include webrtc/modules/rtp_rtcp/include webrtc/modules/utility/include as well, but Stefan may have thoughts on that?
stefan@webrtc.org changed reviewers: + stefan@webrtc.org
https://codereview.webrtc.org/1513483006/diff/1/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.webrtc.org/1513483006/diff/1/PRESUBMIT.py#newcode35 PRESUBMIT.py:35: 'webrtc/modules/audio_coding/include', On 2015/12/10 10:02:20, the sun wrote: > Move to LEGACY: > webrtc/modules/audio_coding/include > webrtc/modules/audio_processing/include > webrtc/voice_engine/include > > Also, I'd like to move: > webrtc/modules/bitrate_controller/include > webrtc/modules/remote_bitrate_estimator/include > webrtc/modules/rtp_rtcp/include > webrtc/modules/utility/include > as well, but Stefan may have thoughts on that? No, I'm OK with that. We don't want new projects to start using these modules directly.
https://codereview.webrtc.org/1513483006/diff/1/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.webrtc.org/1513483006/diff/1/PRESUBMIT.py#newcode35 PRESUBMIT.py:35: 'webrtc/modules/audio_coding/include', On 2015/12/10 10:14:32, stefan-webrtc (holmer) wrote: > On 2015/12/10 10:02:20, the sun wrote: > > Move to LEGACY: > > webrtc/modules/audio_coding/include > > webrtc/modules/audio_processing/include > > webrtc/voice_engine/include > > > > Also, I'd like to move: > > webrtc/modules/bitrate_controller/include > > webrtc/modules/remote_bitrate_estimator/include > > webrtc/modules/rtp_rtcp/include > > webrtc/modules/utility/include > > as well, but Stefan may have thoughts on that? > > No, I'm OK with that. We don't want new projects to start using these modules > directly. I think all of video_coding should also go under legacy.
https://codereview.webrtc.org/1513483006/diff/1/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.webrtc.org/1513483006/diff/1/PRESUBMIT.py#newcode33 PRESUBMIT.py:33: 'talk/app/webrtc', Note that until there is an include/ dir under talk/app/webrtc it would probably be best to keep in under LEGACY as well. https://codereview.webrtc.org/1513483006/diff/1/PRESUBMIT.py#newcode35 PRESUBMIT.py:35: 'webrtc/modules/audio_coding/include', On 2015/12/10 10:20:01, pbos-webrtc wrote: > On 2015/12/10 10:14:32, stefan-webrtc (holmer) wrote: > > On 2015/12/10 10:02:20, the sun wrote: > > > Move to LEGACY: > > > webrtc/modules/audio_coding/include > > > webrtc/modules/audio_processing/include > > > webrtc/voice_engine/include > > > > > > Also, I'd like to move: > > > webrtc/modules/bitrate_controller/include > > > webrtc/modules/remote_bitrate_estimator/include > > > webrtc/modules/rtp_rtcp/include > > > webrtc/modules/utility/include > > > as well, but Stefan may have thoughts on that? > > > > No, I'm OK with that. We don't want new projects to start using these modules > > directly. > > I think all of video_coding should also go under legacy. Ah, I thought that was required to feed codec instances to the video streams. Would that mean webrtc/modules/include can go under legacy too?
On 2015/12/10 10:56:57, the sun wrote: > https://codereview.webrtc.org/1513483006/diff/1/PRESUBMIT.py > File PRESUBMIT.py (right): > > https://codereview.webrtc.org/1513483006/diff/1/PRESUBMIT.py#newcode33 > PRESUBMIT.py:33: 'talk/app/webrtc', > Note that until there is an include/ dir under talk/app/webrtc it would probably > be best to keep in under LEGACY as well. > > https://codereview.webrtc.org/1513483006/diff/1/PRESUBMIT.py#newcode35 > PRESUBMIT.py:35: 'webrtc/modules/audio_coding/include', > On 2015/12/10 10:20:01, pbos-webrtc wrote: > > On 2015/12/10 10:14:32, stefan-webrtc (holmer) wrote: > > > On 2015/12/10 10:02:20, the sun wrote: > > > > Move to LEGACY: > > > > webrtc/modules/audio_coding/include > > > > webrtc/modules/audio_processing/include > > > > webrtc/voice_engine/include > > > > > > > > Also, I'd like to move: > > > > webrtc/modules/bitrate_controller/include > > > > webrtc/modules/remote_bitrate_estimator/include > > > > webrtc/modules/rtp_rtcp/include > > > > webrtc/modules/utility/include > > > > as well, but Stefan may have thoughts on that? > > > > > > No, I'm OK with that. We don't want new projects to start using these > modules > > > directly. > > > > I think all of video_coding should also go under legacy. > > Ah, I thought that was required to feed codec instances to the video streams. > > Would that mean webrtc/modules/include can go under legacy too? I really like this work; are we held back by something?
Description was changed from ========== PRESUBMIT: Split NATIVE_API_DIRS into two lists. This should make it more clear what's supported and what's only checked due to legacy code. No longer uses the word deprecated since it may be confusing. BUG=webrtc:5095 ========== to ========== PRESUBMIT: Split NATIVE_API_DIRS into two lists. This should make it more clear what's supported and what's only checked due to legacy code. No longer uses the word deprecated since it may be confusing. BUG=webrtc:5095 ==========
kjellander@webrtc.org changed reviewers: + pthatcher@webrtc.org
On 2016/02/09 14:49:05, the sun wrote: > On 2015/12/10 10:56:57, the sun wrote: > > https://codereview.webrtc.org/1513483006/diff/1/PRESUBMIT.py > > File PRESUBMIT.py (right): > > > > https://codereview.webrtc.org/1513483006/diff/1/PRESUBMIT.py#newcode33 > > PRESUBMIT.py:33: 'talk/app/webrtc', > > Note that until there is an include/ dir under talk/app/webrtc it would > probably > > be best to keep in under LEGACY as well. > > > > https://codereview.webrtc.org/1513483006/diff/1/PRESUBMIT.py#newcode35 > > PRESUBMIT.py:35: 'webrtc/modules/audio_coding/include', > > On 2015/12/10 10:20:01, pbos-webrtc wrote: > > > On 2015/12/10 10:14:32, stefan-webrtc (holmer) wrote: > > > > On 2015/12/10 10:02:20, the sun wrote: > > > > > Move to LEGACY: > > > > > webrtc/modules/audio_coding/include > > > > > webrtc/modules/audio_processing/include > > > > > webrtc/voice_engine/include > > > > > > > > > > Also, I'd like to move: > > > > > webrtc/modules/bitrate_controller/include > > > > > webrtc/modules/remote_bitrate_estimator/include > > > > > webrtc/modules/rtp_rtcp/include > > > > > webrtc/modules/utility/include > > > > > as well, but Stefan may have thoughts on that? > > > > > > > > No, I'm OK with that. We don't want new projects to start using these > > modules > > > > directly. > > > > > > I think all of video_coding should also go under legacy. > > > > Ah, I thought that was required to feed codec instances to the video streams. > > > > Would that mean webrtc/modules/include can go under legacy too? > > I really like this work; are we held back by something? Sorry, it fell under the radar for a while. I've now moved a lot more to the legacy section and added newly moved directories recently moved in from talk/ as well. +pthatcher for input on how to categorize them (if we should care about headers at all in those locations).
What's the status of this? Should we close it?
On 2016/05/06 11:01:44, stefan-webrtc (holmer) wrote: > What's the status of this? Should we close it? I actually waited for more review feedback but have forgotten to ping owners. I'll send a separate e-mail to pthatcher@ to remind him. Even if we plan to make webrtc/api the single directory containing all the public and supported APIs as part of the work in https://bugs.chromium.org/p/webrtc/issues/detail?id=5716 I think this is an improvement. pbos/stefan/solenberg: feel free to give input/lg on this CL while we wait for pthatcher.
lgtm
https://codereview.webrtc.org/1513483006/diff/20001/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.webrtc.org/1513483006/diff/20001/PRESUBMIT.py#newcode49 PRESUBMIT.py:49: # some legacy downstream code. We should point out that if something in the NATIVE_API_DIRS folders exposes something below through its methods, then those things exposed can be used. For example, scoped_ptr or sigslots, which are contained in webrtc/base.
lgtm
lgtm https://codereview.webrtc.org/1513483006/diff/20001/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.webrtc.org/1513483006/diff/20001/PRESUBMIT.py#newcode49 PRESUBMIT.py:49: # some legacy downstream code. On 2016/05/12 03:43:13, pthatcher1 wrote: > We should point out that if something in the NATIVE_API_DIRS folders exposes > something below through its methods, then those things exposed can be used. For > example, scoped_ptr or sigslots, which are contained in webrtc/base. I don't think we need to point that out at this time, it will be a natural consequence of using the thing in NATIVE_API_DIRS, and will be sorted out once we've move all de-facto external APIs into api/.
lgtm
Description was changed from ========== PRESUBMIT: Split NATIVE_API_DIRS into two lists. This should make it more clear what's supported and what's only checked due to legacy code. No longer uses the word deprecated since it may be confusing. BUG=webrtc:5095 ========== to ========== PRESUBMIT: Split NATIVE_API_DIRS into two lists. This should make it more clear what's supported and what's only checked due to legacy code. No longer uses the word deprecated since it may be confusing. BUG=webrtc:5095 NOTRY=True ==========
The CQ bit was checked by kjellander@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1513483006/20001
Message was sent while issue was closed.
Description was changed from ========== PRESUBMIT: Split NATIVE_API_DIRS into two lists. This should make it more clear what's supported and what's only checked due to legacy code. No longer uses the word deprecated since it may be confusing. BUG=webrtc:5095 NOTRY=True ========== to ========== PRESUBMIT: Split NATIVE_API_DIRS into two lists. This should make it more clear what's supported and what's only checked due to legacy code. No longer uses the word deprecated since it may be confusing. BUG=webrtc:5095 NOTRY=True ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== PRESUBMIT: Split NATIVE_API_DIRS into two lists. This should make it more clear what's supported and what's only checked due to legacy code. No longer uses the word deprecated since it may be confusing. BUG=webrtc:5095 NOTRY=True ========== to ========== PRESUBMIT: Split NATIVE_API_DIRS into two lists. This should make it more clear what's supported and what's only checked due to legacy code. No longer uses the word deprecated since it may be confusing. BUG=webrtc:5095 NOTRY=True Committed: https://crrev.com/dd70547adb4e30a5ed9e70cad30f13793b11683d Cr-Commit-Position: refs/heads/master@{#13094} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/dd70547adb4e30a5ed9e70cad30f13793b11683d Cr-Commit-Position: refs/heads/master@{#13094} |
