Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1678763002:
Re-add target in webrtc/libjingle that was dead. (Closed)
|
Created:
4 years, 10 months ago by kjellander_webrtc Modified:
4 years, 10 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, qiang.lu, niklas.enbom, yujie_mao (webrtc), peah-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRe-add target in webrtc/libjingle that was dead.
In https://webrtc-codereview.appspot.com/36439004 the
GYP processing of webrtc/libjingle/libjingle.gyp was removed
and became dead code. This moves the target into webrtc/libjingle/xmpp/xmpp.gyp
and adds it to processing.
Patch Set 1 #Patch Set 2 : Disable -Wnon-virtual-dtor #
Total comments: 8
Messages
Total messages: 12 (4 generated)
Description was changed from ========== Readd target in webrtc/libjingle that was dead. In https://webrtc-codereview.appspot.com/36439004 the GYP processing of webrtc/libjingle/libjingle.gyp was removed and became dead code. This moves the target into webrtc/libjingle/xmpp/xmpp.gyp and adds it to processing. ========== to ========== Re-add target in webrtc/libjingle that was dead. In https://webrtc-codereview.appspot.com/36439004 the GYP processing of webrtc/libjingle/libjingle.gyp was removed and became dead code. This moves the target into webrtc/libjingle/xmpp/xmpp.gyp and adds it to processing. ==========
kjellander@webrtc.org changed reviewers: + pbos@webrtc.org, pthatcher@webrtc.org
I thought it was best to re-add this, even if it's only two files. Or should the files be dropped instead?
I'll leave it to pthatcher@, don't have enough context here. I think we want webrtc/libjingle to be removed, but I don't think that's ready yet.
https://codereview.webrtc.org/1678763002/diff/20001/webrtc/libjingle/xmpp/xmp... File webrtc/libjingle/xmpp/xmpp.gyp (right): https://codereview.webrtc.org/1678763002/diff/20001/webrtc/libjingle/xmpp/xmp... webrtc/libjingle/xmpp/xmpp.gyp:165: ], We can just delete this (and jinglinfotask.*). It's never used by anything. https://codereview.webrtc.org/1678763002/diff/20001/webrtc/webrtc.gyp File webrtc/webrtc.gyp (right): https://codereview.webrtc.org/1678763002/diff/20001/webrtc/webrtc.gyp#newcode65 webrtc/webrtc.gyp:65: 'libjingle/xmpp/xmpp.gyp:*', We shouldn't compile these into webrtc. They are only used by parts of Chrome, and only left in our repository as a convenience to them. We should leave them out our main build. For reference, they are used by: //src/jingle/glue/ //src/jingle/notifier/ //src/remoting/ If those three stop using it, we can remove them altogether.
https://codereview.webrtc.org/1678763002/diff/20001/webrtc/libjingle/xmpp/xmp... File webrtc/libjingle/xmpp/xmpp.gyp (right): https://codereview.webrtc.org/1678763002/diff/20001/webrtc/libjingle/xmpp/xmp... webrtc/libjingle/xmpp/xmpp.gyp:165: ], On 2016/02/08 19:11:47, pthatcher1 wrote: > We can just delete this (and jinglinfotask.*). It's never used by anything. I'll be happy to delete. Found this though https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... but it seems webrtc/p2p/client/autoportallocator.h is also dead code that could be deleted. https://codereview.webrtc.org/1678763002/diff/20001/webrtc/webrtc.gyp File webrtc/webrtc.gyp (right): https://codereview.webrtc.org/1678763002/diff/20001/webrtc/webrtc.gyp#newcode65 webrtc/webrtc.gyp:65: 'libjingle/xmpp/xmpp.gyp:*', On 2016/02/08 19:11:47, pthatcher1 wrote: > We shouldn't compile these into webrtc. They are only used by parts of Chrome, > and only left in our repository as a convenience to them. We should leave them > out our main build. > > For reference, they are used by: > > //src/jingle/glue/ > //src/jingle/notifier/ > //src/remoting/ > > If those three stop using it, we can remove them altogether. If they're only used by Chromium, I think we should move the source into Chromium's src repo instead. If we have them in our repo but don't build them, it's possible a future Clang update introduces a new warning which makes them start fail to compile in Chromium. Then that'll require someone to create a WebRTC CL and land (with our trybots being useless since they don't even build the source) + wait for a roll to go in and so on. Quite not how it should work. I'd say we either build the code or move the source files into Chromium. Sounds reasonable?
pthatcher@google.com changed reviewers: + pthatcher@google.com
https://codereview.webrtc.org/1678763002/diff/20001/webrtc/libjingle/xmpp/xmp... File webrtc/libjingle/xmpp/xmpp.gyp (right): https://codereview.webrtc.org/1678763002/diff/20001/webrtc/libjingle/xmpp/xmp... webrtc/libjingle/xmpp/xmpp.gyp:165: ], On 2016/02/08 19:43:20, kjellander (webrtc) wrote: > On 2016/02/08 19:11:47, pthatcher1 wrote: > > We can just delete this (and jinglinfotask.*). It's never used by anything. > > I'll be happy to delete. Found this though > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > but it seems webrtc/p2p/client/autoportallocator.h is also dead code that could > be deleted. Oh wow, yes. Please delete that, too. https://codereview.webrtc.org/1678763002/diff/20001/webrtc/webrtc.gyp File webrtc/webrtc.gyp (right): https://codereview.webrtc.org/1678763002/diff/20001/webrtc/webrtc.gyp#newcode65 webrtc/webrtc.gyp:65: 'libjingle/xmpp/xmpp.gyp:*', On 2016/02/08 19:43:21, kjellander (webrtc) wrote: > On 2016/02/08 19:11:47, pthatcher1 wrote: > > We shouldn't compile these into webrtc. They are only used by parts of > Chrome, > > and only left in our repository as a convenience to them. We should leave > them > > out our main build. > > > > For reference, they are used by: > > > > //src/jingle/glue/ > > //src/jingle/notifier/ > > //src/remoting/ > > > > If those three stop using it, we can remove them altogether. > > If they're only used by Chromium, I think we should move the source into > Chromium's src repo instead. > If we have them in our repo but don't build them, it's possible a future Clang > update introduces a new warning which makes them start fail to compile in > Chromium. Then that'll require someone to create a WebRTC CL and land (with our > trybots being useless since they don't even build the source) + wait for a roll > to go in and so on. Quite not how it should work. > > I'd say we either build the code or move the source files into Chromium. Sounds > reasonable? I'd love it if you moved them into Chromium. We talked to the remoting team about doing that (I believe the volunteered to own it), but it's never been a high priority for either one of us. If you can talk to sergeyu@ and figure out a better home, that would be great.
kjellander@webrtc.org changed reviewers: + sergeyu@chromium.org
+sergeyu https://codereview.webrtc.org/1678763002/diff/20001/webrtc/webrtc.gyp File webrtc/webrtc.gyp (right): https://codereview.webrtc.org/1678763002/diff/20001/webrtc/webrtc.gyp#newcode65 webrtc/webrtc.gyp:65: 'libjingle/xmpp/xmpp.gyp:*', On 2016/02/08 19:50:04, pthatcher wrote: > On 2016/02/08 19:43:21, kjellander (webrtc) wrote: > > On 2016/02/08 19:11:47, pthatcher1 wrote: > > > We shouldn't compile these into webrtc. They are only used by parts of > > Chrome, > > > and only left in our repository as a convenience to them. We should leave > > them > > > out our main build. > > > > > > For reference, they are used by: > > > > > > //src/jingle/glue/ > > > //src/jingle/notifier/ > > > //src/remoting/ > > > > > > If those three stop using it, we can remove them altogether. > > > > If they're only used by Chromium, I think we should move the source into > > Chromium's src repo instead. > > If we have them in our repo but don't build them, it's possible a future Clang > > update introduces a new warning which makes them start fail to compile in > > Chromium. Then that'll require someone to create a WebRTC CL and land (with > our > > trybots being useless since they don't even build the source) + wait for a > roll > > to go in and so on. Quite not how it should work. > > > > I'd say we either build the code or move the source files into Chromium. > Sounds > > reasonable? > > I'd love it if you moved them into Chromium. We talked to the remoting team > about doing that (I believe the volunteered to own it), but it's never been a > high priority for either one of us. If you can talk to sergeyu@ and figure out > a better home, that would be great. sergeyu: if I take care of moving this to Chromium, can remoting commit to own the code? I really don't like the fact of having code in WebRTC that isn't built as part of our CI.
https://codereview.webrtc.org/1678763002/diff/20001/webrtc/webrtc.gyp File webrtc/webrtc.gyp (right): https://codereview.webrtc.org/1678763002/diff/20001/webrtc/webrtc.gyp#newcode65 webrtc/webrtc.gyp:65: 'libjingle/xmpp/xmpp.gyp:*', On 2016/02/15 18:27:54, kjellander (webrtc) wrote: > On 2016/02/08 19:50:04, pthatcher wrote: > > On 2016/02/08 19:43:21, kjellander (webrtc) wrote: > > > On 2016/02/08 19:11:47, pthatcher1 wrote: > > > > We shouldn't compile these into webrtc. They are only used by parts of > > > Chrome, > > > > and only left in our repository as a convenience to them. We should leave > > > them > > > > out our main build. > > > > > > > > For reference, they are used by: > > > > > > > > //src/jingle/glue/ > > > > //src/jingle/notifier/ > > > > //src/remoting/ > > > > > > > > If those three stop using it, we can remove them altogether. > > > > > > If they're only used by Chromium, I think we should move the source into > > > Chromium's src repo instead. > > > If we have them in our repo but don't build them, it's possible a future > Clang > > > update introduces a new warning which makes them start fail to compile in > > > Chromium. Then that'll require someone to create a WebRTC CL and land (with > > our > > > trybots being useless since they don't even build the source) + wait for a > > roll > > > to go in and so on. Quite not how it should work. > > > > > > I'd say we either build the code or move the source files into Chromium. > > Sounds > > > reasonable? > > > > I'd love it if you moved them into Chromium. We talked to the remoting team > > about doing that (I believe the volunteered to own it), but it's never been a > > high priority for either one of us. If you can talk to sergeyu@ and figure > out > > a better home, that would be great. > > sergeyu: if I take care of moving this to Chromium, can remoting commit to own > the code? I really don't like the fact of having code in WebRTC that isn't built > as part of our CI. Sent a separate e-mail about it with more details. Yes, please move xmpp and xmllite to jingle/xmpp and jingle/xmllite.
Message was sent while issue was closed.
On 2016/02/16 20:24:35, Sergey Ulanov wrote: > https://codereview.webrtc.org/1678763002/diff/20001/webrtc/webrtc.gyp > File webrtc/webrtc.gyp (right): > > https://codereview.webrtc.org/1678763002/diff/20001/webrtc/webrtc.gyp#newcode65 > webrtc/webrtc.gyp:65: 'libjingle/xmpp/xmpp.gyp:*', > On 2016/02/15 18:27:54, kjellander (webrtc) wrote: > > On 2016/02/08 19:50:04, pthatcher wrote: > > > On 2016/02/08 19:43:21, kjellander (webrtc) wrote: > > > > On 2016/02/08 19:11:47, pthatcher1 wrote: > > > > > We shouldn't compile these into webrtc. They are only used by parts of > > > > Chrome, > > > > > and only left in our repository as a convenience to them. We should > leave > > > > them > > > > > out our main build. > > > > > > > > > > For reference, they are used by: > > > > > > > > > > //src/jingle/glue/ > > > > > //src/jingle/notifier/ > > > > > //src/remoting/ > > > > > > > > > > If those three stop using it, we can remove them altogether. > > > > > > > > If they're only used by Chromium, I think we should move the source into > > > > Chromium's src repo instead. > > > > If we have them in our repo but don't build them, it's possible a future > > Clang > > > > update introduces a new warning which makes them start fail to compile in > > > > Chromium. Then that'll require someone to create a WebRTC CL and land > (with > > > our > > > > trybots being useless since they don't even build the source) + wait for a > > > roll > > > > to go in and so on. Quite not how it should work. > > > > > > > > I'd say we either build the code or move the source files into Chromium. > > > Sounds > > > > reasonable? > > > > > > I'd love it if you moved them into Chromium. We talked to the remoting team > > > about doing that (I believe the volunteered to own it), but it's never been > a > > > high priority for either one of us. If you can talk to sergeyu@ and figure > > out > > > a better home, that would be great. > > > > sergeyu: if I take care of moving this to Chromium, can remoting commit to own > > the code? I really don't like the fact of having code in WebRTC that isn't > built > > as part of our CI. > > Sent a separate e-mail about it with more details. Yes, please move xmpp and > xmllite to jingle/xmpp and jingle/xmllite. Closing this issue as I we don't want this. I hope to be able to migrate the code to Chromium, but it was harder than I expected (https://codereview.chromium.org/1703053003) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
