Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2471573003:
Split out target rtc_media_base from rtc_media (Closed)
|
Created:
4 years, 1 month ago by magjed_webrtc Modified:
4 years, 1 month ago Reviewers:
kjellander_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionSplit out target rtc_media_base from rtc_media
The purpose with this CL is to be able to depend on
cricket::VideoCodec (webrtc/media/base/codec.h) from other targets
without getting cyclic dependencies.
BUG=webrtc:6402, webrtc:6337
NOTRY=True
Committed: https://crrev.com/aae7e7cf35a5bb43ebbaf75396aa7ccc544e920a
Cr-Commit-Position: refs/heads/master@{#15137}
Patch Set 1 #Patch Set 2 : Fix size_t to int conversion. #
Total comments: 2
Patch Set 3 : Rebase #
Messages
Total messages: 34 (17 generated)
Description was changed from ========== Split out target rtc_media_base from rtc_media The purpose with this CL is to be able to depend on cricket::VideoCodec (webrtc/media/base/codec.h) from other targets without getting cyclic dependencies. BUG=webrtc:6337 ========== to ========== Split out target rtc_media_base from rtc_media The purpose with this CL is to be able to depend on cricket::VideoCodec (webrtc/media/base/codec.h) from other targets without getting cyclic dependencies. BUG=webrtc:6402,webrtc:6337 ==========
magjed@webrtc.org changed reviewers: + kjellander@webrtc.org
kjellander - please take a look.
looks good, but you'll have to update GYP a few more days unfortunately.
On 2016/11/07 21:12:33, kjellander_webrtc wrote: > looks good, but you'll have to update GYP a few more days unfortunately. Ping, can I ignore updating GYP now and land this CL?
On 2016/11/14 16:06:07, magjed_webrtc wrote: > On 2016/11/07 21:12:33, kjellander_webrtc wrote: > > looks good, but you'll have to update GYP a few more days unfortunately. > > Ping, can I ignore updating GYP now and land this CL? Give us a few more days, then we'll rip GYP out, it's been less than a week still.
On 2016/11/14 21:12:58, kjellander_webrtc wrote: > On 2016/11/14 16:06:07, magjed_webrtc wrote: > > On 2016/11/07 21:12:33, kjellander_webrtc wrote: > > > looks good, but you'll have to update GYP a few more days unfortunately. > > > > Ping, can I ignore updating GYP now and land this CL? > > Give us a few more days, then we'll rip GYP out, it's been less than a week > still. GYP is now gone so please proceed with this. I'm seeing Windows errors (I re-ran those two trybots just now, so it's valid) so you have to fix those first.
The CQ bit was checked by magjed@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by magjed@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-webrtc-committers". Note that this has nothing to do with OWNERS files.
On 2016/11/16 21:33:45, kjellander_webrtc wrote: > On 2016/11/14 21:12:58, kjellander_webrtc wrote: > > On 2016/11/14 16:06:07, magjed_webrtc wrote: > > > On 2016/11/07 21:12:33, kjellander_webrtc wrote: > > > > looks good, but you'll have to update GYP a few more days unfortunately. > > > > > > Ping, can I ignore updating GYP now and land this CL? > > > > Give us a few more days, then we'll rip GYP out, it's been less than a week > > still. > > GYP is now gone so please proceed with this. I'm seeing Windows errors (I re-ran > those two trybots just now, so it's valid) so you have to fix those first. I fixed the Windows errors. Can you stamp it now?
lgtm although I'm not owner here. https://codereview.webrtc.org/2471573003/diff/20001/webrtc/media/BUILD.gn File webrtc/media/BUILD.gn (left): https://codereview.webrtc.org/2471573003/diff/20001/webrtc/media/BUILD.gn#old... webrtc/media/BUILD.gn:174: "../video", So video wasn't really needed?
The CQ bit was checked by magjed@webrtc.org
The CQ bit was unchecked by magjed@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2471573003/diff/20001/webrtc/media/BUILD.gn File webrtc/media/BUILD.gn (left): https://codereview.webrtc.org/2471573003/diff/20001/webrtc/media/BUILD.gn#old... webrtc/media/BUILD.gn:174: "../video", On 2016/11/17 14:58:42, kjellander_webrtc wrote: > So video wasn't really needed? No. There is no includes of webrtc/video/ at all in the webrtc/media folder, so I removed it. p2p is only used from webrtc/media/base so I have it there.
The CQ bit was checked by magjed@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10374)
Description was changed from ========== Split out target rtc_media_base from rtc_media The purpose with this CL is to be able to depend on cricket::VideoCodec (webrtc/media/base/codec.h) from other targets without getting cyclic dependencies. BUG=webrtc:6402,webrtc:6337 ========== to ========== Split out target rtc_media_base from rtc_media The purpose with this CL is to be able to depend on cricket::VideoCodec (webrtc/media/base/codec.h) from other targets without getting cyclic dependencies. BUG=webrtc:6402,webrtc:6337 NOTRY=True ==========
The CQ bit was checked by magjed@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/2471573003/#ps40001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Split out target rtc_media_base from rtc_media The purpose with this CL is to be able to depend on cricket::VideoCodec (webrtc/media/base/codec.h) from other targets without getting cyclic dependencies. BUG=webrtc:6402,webrtc:6337 NOTRY=True ========== to ========== Split out target rtc_media_base from rtc_media The purpose with this CL is to be able to depend on cricket::VideoCodec (webrtc/media/base/codec.h) from other targets without getting cyclic dependencies. BUG=webrtc:6402,webrtc:6337 NOTRY=True ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Split out target rtc_media_base from rtc_media The purpose with this CL is to be able to depend on cricket::VideoCodec (webrtc/media/base/codec.h) from other targets without getting cyclic dependencies. BUG=webrtc:6402,webrtc:6337 NOTRY=True ========== to ========== Split out target rtc_media_base from rtc_media The purpose with this CL is to be able to depend on cricket::VideoCodec (webrtc/media/base/codec.h) from other targets without getting cyclic dependencies. BUG=webrtc:6402,webrtc:6337 NOTRY=True Committed: https://crrev.com/aae7e7cf35a5bb43ebbaf75396aa7ccc544e920a Cr-Commit-Position: refs/heads/master@{#15137} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/aae7e7cf35a5bb43ebbaf75396aa7ccc544e920a Cr-Commit-Position: refs/heads/master@{#15137}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.webrtc.org/2508163002/ by magjed@webrtc.org. The reason for reverting is: Breaks downstream import.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
