Moved transport.h from webrtc/ to webrtc/api, created build target and updated dependencies.

Moved transport.h from webrtc/ to webrtc/api, created build target and updated WebRTC dependencies.

transport.h defines an interface for sending rtp and rtcp packets,
which is used by MediaChannel in webrtc/media/engine,
{Audio|Video}{Send|Receive}Stream and in a few other
places. It was part of the build target //webrtc:webrtc, which is a monolithic target with
all webrtc production code. This CL moves the header to its own target in webrtc/api
and deprecates the old location.

Targets in webrtc/api should in general only depend on other
targets in webrtc/api. The target webrtc/api:call_api depends on
transport.h. This change also makes webrtc/voice_engine pass GN's header
include checker and is needed in order for webrtc/api:call_api to pass
it.

transport.h will be completely removed in a follow-up CL in a few weeks
after clients have updated their includes.

NOTRY=True

BUG=webrtc:5589, webrtc:5878, webrtc:6785
Committed: https://crrev.com/a8eb756a346682a4af66f9a9c0b87cfe1ed5b114
Cr-Commit-Position: refs/heads/master@{#15267}

[aleloi, 2016-10-17 13:52:38]

Patchset #1 (id:1) has been deleted

[aleloi, 2016-10-17 13:52:49]

Patchset #1 (id:20001) has been deleted

[aleloi, 2016-10-17 13:53:53]

aleloi@webrtc.org changed reviewers:
+ kjellander@webrtc.org, ossu@webrtc.org

[aleloi, 2016-10-17 13:53:54]

Please take a look!

https://codereview.webrtc.org/2426563003/diff/40001/webrtc/api/call/transport.h
File webrtc/api/call/transport.h (right):

https://codereview.webrtc.org/2426563003/diff/40001/webrtc/api/call/transport...
webrtc/api/call/transport.h:14: #include "webrtc/base/basictypes.h"
Not in any build target, but included in
webrtc/api/{android/jni/androidnetworkmonitor_jni.h,call/transport.h,datachannelinterface.h,jsep.h,mediastreaminterface.h,rtpreceiver.h,rtpsender.h,statstypes.h,test/fakeaudiocapturemodule.h,webrtcsession.cc}

[aleloi, 2016-10-17 13:55:19]

aleloi@webrtc.org changed reviewers:
+ kwiberg@webrtc.org

[aleloi, 2016-10-17 13:55:19]

Added kwiberg@ as top level owner. Please take a look, kwiberg@.

[aleloi, 2016-10-17 14:00:08]

Patchset #1 (id:40001) has been deleted

[aleloi, 2016-10-17 14:00:37]

Description was changed from

==========
Moved transport.h from webrtc/ to webrtc/api, created build target and updated
dependencies.

transport.h defines an interface for sending rtp and rtcp packets,
which is used by MediaChannel in webrtc/media/engine,
Audio{Send|Receive}Stream in webrtc/api/call and a few other
places. Targets in webrtc/api should in general only depend on other
targets in webrtc/api. The target webrtc/api:call_api depends on this
target. This change also makes webrtc/voice_engine pass GN's header
include checker and is needed in order for webrtc/api:call_api to pass
it.

BUG=webrc:5589
==========

to

==========
Moved transport.h from webrtc/ to webrtc/api, created build target and updated
dependencies.

transport.h defines an interface for sending rtp and rtcp packets,
which is used by MediaChannel in webrtc/media/engine,
Audio{Send|Receive}Stream in webrtc/api/call and a few other
places. Targets in webrtc/api should in general only depend on other
targets in webrtc/api. The target webrtc/api:call_api depends on this
target. This change also makes webrtc/voice_engine pass GN's header
include checker and is needed in order for webrtc/api:call_api to pass
it.

NOTRY=True

BUG=webrc:5589
==========

[aleloi, 2016-10-17 14:01:33]

https://codereview.webrtc.org/2426563003/diff/60001/webrtc/api/call/transport.h
File webrtc/api/call/transport.h (right):

https://codereview.webrtc.org/2426563003/diff/60001/webrtc/api/call/transport...
webrtc/api/call/transport.h:14: #include "webrtc/base/basictypes.h"
This file is not in any build target, but is included in
webrtc/api/{android/jni/androidnetworkmonitor_jni.h,call/transport.h,datachannelinterface.h,jsep.h,mediastreaminterface.h,rtpreceiver.h,rtpsender.h,statstypes.h,test/fakeaudiocapturemodule.h,webrtcsession.cc}

(So it should probably be OK to include here as well).

[ossu, 2016-10-17 14:05:44]

Two quick comments. Tried to post this once but someone removed the patch set I
was commenting on ... Alex! :)

https://codereview.webrtc.org/2426563003/diff/60001/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/2426563003/diff/60001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:321: rtc_source_set("transport_api") {
If transport.h is in api/call, shouldn't it be part of the call_api target
instead of being its own target?

https://codereview.webrtc.org/2426563003/diff/60001/webrtc/api/call/transport.h
File webrtc/api/call/transport.h (right):

https://codereview.webrtc.org/2426563003/diff/60001/webrtc/api/call/transport...
webrtc/api/call/transport.h:14: #include "webrtc/base/basictypes.h"
On 2016/10/17 14:01:33, aleloi wrote:
> This file is not in any build target, but is included in
>
webrtc/api/{android/jni/androidnetworkmonitor_jni.h,call/transport.h,datachannelinterface.h,jsep.h,mediastreaminterface.h,rtpreceiver.h,rtpsender.h,statstypes.h,test/fakeaudiocapturemodule.h,webrtcsession.cc}
> 
> (So it should probably be OK to include here as well).

Are you sure you wouldn't be better off including <stddef.h> and <stdint.h>
directly?

[aleloi, 2016-10-17 14:21:56]

Oops, sorry for that, ossu@

https://codereview.webrtc.org/2426563003/diff/60001/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/2426563003/diff/60001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:321: rtc_source_set("transport_api") {
On 2016/10/17 14:05:44, ossu wrote:
> If transport.h is in api/call, shouldn't it be part of the call_api target
> instead of being its own target?

I'm not sure, but I think it shouldn't be merged with call_api.
webrtc::Transport is not tightly coupled with the other things in call. It's
used in quite a lot of other places, which do include anything else from call.
Also, I think that splitting up large build targets in small independent
submodules is generally a good thing: it generates less dependencies in the
future and simplifies modularization. WDYT?

https://codereview.webrtc.org/2426563003/diff/60001/webrtc/api/call/transport.h
File webrtc/api/call/transport.h (right):

https://codereview.webrtc.org/2426563003/diff/60001/webrtc/api/call/transport...
webrtc/api/call/transport.h:14: #include "webrtc/base/basictypes.h"
On 2016/10/17 14:05:44, ossu wrote:
> On 2016/10/17 14:01:33, aleloi wrote:
> > This file is not in any build target, but is included in
> >
>
webrtc/api/{android/jni/androidnetworkmonitor_jni.h,call/transport.h,datachannelinterface.h,jsep.h,mediastreaminterface.h,rtpreceiver.h,rtpsender.h,statstypes.h,test/fakeaudiocapturemodule.h,webrtcsession.cc}
> > 
> > (So it should probably be OK to include here as well).
> 
> Are you sure you wouldn't be better off including <stddef.h> and <stdint.h>
> directly?

I think including the std headers is more simple and readable. Considering the
amount of macros and defines in basictypes.h. Changed in next PS.

[the sun, 2016-10-17 14:38:35]

solenberg@webrtc.org changed reviewers:
+ solenberg@webrtc.org

[the sun, 2016-10-17 14:38:36]

The CL description should mention Video[Send|Receive]Stream in addition to
audio.

https://codereview.webrtc.org/2426563003/diff/60001/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/2426563003/diff/60001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:321: rtc_source_set("transport_api") {
On 2016/10/17 14:21:56, aleloi wrote:
> On 2016/10/17 14:05:44, ossu wrote:
> > If transport.h is in api/call, shouldn't it be part of the call_api target
> > instead of being its own target?
> 
> I'm not sure, but I think it shouldn't be merged with call_api.
> webrtc::Transport is not tightly coupled with the other things in call. It's
> used in quite a lot of other places, which do include anything else from call.
> Also, I think that splitting up large build targets in small independent
> submodules is generally a good thing: it generates less dependencies in the
> future and simplifies modularization. WDYT?

I was about to agree with Oskar first, but looking through the rest of your CL I
think it should be a separate target - there's no reason for the rtp_rtcp module
to know about ALL of Call, when it just needs Transport.

https://codereview.webrtc.org/2426563003/diff/60001/webrtc/api/call/transport.h
File webrtc/api/call/transport.h (right):

https://codereview.webrtc.org/2426563003/diff/60001/webrtc/api/call/transport...
webrtc/api/call/transport.h:14: #include "webrtc/base/basictypes.h"
On 2016/10/17 14:05:44, ossu wrote:
> On 2016/10/17 14:01:33, aleloi wrote:
> > This file is not in any build target, but is included in
> >
>
webrtc/api/{android/jni/androidnetworkmonitor_jni.h,call/transport.h,datachannelinterface.h,jsep.h,mediastreaminterface.h,rtpreceiver.h,rtpsender.h,statstypes.h,test/fakeaudiocapturemodule.h,webrtcsession.cc}
> > 
> > (So it should probably be OK to include here as well).

No, the files in api/ should only include files in api/.

> Are you sure you wouldn't be better off including <stddef.h> and <stdint.h>
> directly?

Discussed offline.

[the sun, 2016-10-17 14:38:46]

solenberg@webrtc.org changed reviewers:
- solenberg@webrtc.org

[aleloi, 2016-10-17 14:41:40]

Description was changed from

==========
Moved transport.h from webrtc/ to webrtc/api, created build target and updated
dependencies.

transport.h defines an interface for sending rtp and rtcp packets,
which is used by MediaChannel in webrtc/media/engine,
Audio{Send|Receive}Stream in webrtc/api/call and a few other
places. Targets in webrtc/api should in general only depend on other
targets in webrtc/api. The target webrtc/api:call_api depends on this
target. This change also makes webrtc/voice_engine pass GN's header
include checker and is needed in order for webrtc/api:call_api to pass
it.

NOTRY=True

BUG=webrc:5589
==========

to

==========
Moved transport.h from webrtc/ to webrtc/api, created build target and updated
dependencies.

transport.h defines an interface for sending rtp and rtcp packets,
which is used by MediaChannel in webrtc/media/engine,
{Audio|Video}{Send|Receive}Stream and a few other
places. Targets in webrtc/api should in general only depend on other
targets in webrtc/api. The target webrtc/api:call_api depends on this
target. This change also makes webrtc/voice_engine pass GN's header
include checker and is needed in order for webrtc/api:call_api to pass
it.

NOTRY=True

BUG=webrtc:5589
==========

[ossu, 2016-10-17 14:54:58]

https://codereview.webrtc.org/2426563003/diff/60001/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/2426563003/diff/60001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:321: rtc_source_set("transport_api") {
On 2016/10/17 14:38:35, the sun wrote:
> On 2016/10/17 14:21:56, aleloi wrote:
> > On 2016/10/17 14:05:44, ossu wrote:
> > > If transport.h is in api/call, shouldn't it be part of the call_api target
> > > instead of being its own target?
> > 
> > I'm not sure, but I think it shouldn't be merged with call_api.
> > webrtc::Transport is not tightly coupled with the other things in call. It's
> > used in quite a lot of other places, which do include anything else from
call.
> > Also, I think that splitting up large build targets in small independent
> > submodules is generally a good thing: it generates less dependencies in the
> > future and simplifies modularization. WDYT?
> 
> I was about to agree with Oskar first, but looking through the rest of your CL
I
> think it should be a separate target - there's no reason for the rtp_rtcp
module
> to know about ALL of Call, when it just needs Transport.

Alright. Should the file still be in call/ then? Or directly under api?

[the sun, 2016-10-17 15:00:46]

On 2016/10/17 14:54:58, ossu wrote:
> https://codereview.webrtc.org/2426563003/diff/60001/webrtc/api/BUILD.gn
> File webrtc/api/BUILD.gn (right):
> 
>
https://codereview.webrtc.org/2426563003/diff/60001/webrtc/api/BUILD.gn#newco...
> webrtc/api/BUILD.gn:321: rtc_source_set("transport_api") {
> On 2016/10/17 14:38:35, the sun wrote:
> > On 2016/10/17 14:21:56, aleloi wrote:
> > > On 2016/10/17 14:05:44, ossu wrote:
> > > > If transport.h is in api/call, shouldn't it be part of the call_api
target
> > > > instead of being its own target?
> > > 
> > > I'm not sure, but I think it shouldn't be merged with call_api.
> > > webrtc::Transport is not tightly coupled with the other things in call.
It's
> > > used in quite a lot of other places, which do include anything else from
> call.
> > > Also, I think that splitting up large build targets in small independent
> > > submodules is generally a good thing: it generates less dependencies in
the
> > > future and simplifies modularization. WDYT?
> > 
> > I was about to agree with Oskar first, but looking through the rest of your
CL
> I
> > think it should be a separate target - there's no reason for the rtp_rtcp
> module
> > to know about ALL of Call, when it just needs Transport.
> 
> Alright. Should the file still be in call/ then? Or directly under api?

I think it's ok if it lives under call/ - WDYT?

[ossu, 2016-10-17 17:05:19]

On 2016/10/17 15:00:46, the sun wrote:
> On 2016/10/17 14:54:58, ossu wrote:
> > https://codereview.webrtc.org/2426563003/diff/60001/webrtc/api/BUILD.gn
> > File webrtc/api/BUILD.gn (right):
> > 
> >
>
https://codereview.webrtc.org/2426563003/diff/60001/webrtc/api/BUILD.gn#newco...
> > webrtc/api/BUILD.gn:321: rtc_source_set("transport_api") {
> > On 2016/10/17 14:38:35, the sun wrote:
> > > On 2016/10/17 14:21:56, aleloi wrote:
> > > > On 2016/10/17 14:05:44, ossu wrote:
> > > > > If transport.h is in api/call, shouldn't it be part of the call_api
> target
> > > > > instead of being its own target?
> > > > 
> > > > I'm not sure, but I think it shouldn't be merged with call_api.
> > > > webrtc::Transport is not tightly coupled with the other things in call.
> It's
> > > > used in quite a lot of other places, which do include anything else from
> > call.
> > > > Also, I think that splitting up large build targets in small independent
> > > > submodules is generally a good thing: it generates less dependencies in
> the
> > > > future and simplifies modularization. WDYT?
> > > 
> > > I was about to agree with Oskar first, but looking through the rest of
your
> CL
> > I
> > > think it should be a separate target - there's no reason for the rtp_rtcp
> > module
> > > to know about ALL of Call, when it just needs Transport.
> > 
> > Alright. Should the file still be in call/ then? Or directly under api?
> 
> I think it's ok if it lives under call/ - WDYT?

I don't know. It probably is. It's a bit weird that there's a call target for
the stuff in call/, but that doesn't include that thing in call/. I'll take
another look at it tomorrow and try to figure it out.

[the sun, 2016-10-26 08:34:47]

On 2016/10/17 17:05:19, ossu wrote:
> On 2016/10/17 15:00:46, the sun wrote:
> > On 2016/10/17 14:54:58, ossu wrote:
> > > https://codereview.webrtc.org/2426563003/diff/60001/webrtc/api/BUILD.gn
> > > File webrtc/api/BUILD.gn (right):
> > > 
> > >
> >
>
https://codereview.webrtc.org/2426563003/diff/60001/webrtc/api/BUILD.gn#newco...
> > > webrtc/api/BUILD.gn:321: rtc_source_set("transport_api") {
> > > On 2016/10/17 14:38:35, the sun wrote:
> > > > On 2016/10/17 14:21:56, aleloi wrote:
> > > > > On 2016/10/17 14:05:44, ossu wrote:
> > > > > > If transport.h is in api/call, shouldn't it be part of the call_api
> > target
> > > > > > instead of being its own target?
> > > > > 
> > > > > I'm not sure, but I think it shouldn't be merged with call_api.
> > > > > webrtc::Transport is not tightly coupled with the other things in
call.
> > It's
> > > > > used in quite a lot of other places, which do include anything else
from
> > > call.
> > > > > Also, I think that splitting up large build targets in small
independent
> > > > > submodules is generally a good thing: it generates less dependencies
in
> > the
> > > > > future and simplifies modularization. WDYT?
> > > > 
> > > > I was about to agree with Oskar first, but looking through the rest of
> your
> > CL
> > > I
> > > > think it should be a separate target - there's no reason for the
rtp_rtcp
> > > module
> > > > to know about ALL of Call, when it just needs Transport.
> > > 
> > > Alright. Should the file still be in call/ then? Or directly under api?
> > 
> > I think it's ok if it lives under call/ - WDYT?
> 
> I don't know. It probably is. It's a bit weird that there's a call target for
> the stuff in call/, but that doesn't include that thing in call/. I'll take
> another look at it tomorrow and try to figure it out.

The more I think about it the more it makes sense that Transport is in a
separate target, but in the longer term it should likely live in "api/network/"
(or if there's a better name). Layers of the onion - transport is used by Call
but defined by a lower-layer module (e.g. RtpRtcp, or similar). We don't want to
put RtpRtcp in api/ though, so either we create api/network/ or just leave it in
api/call/ for the time being.

[the sun, 2016-10-26 08:36:19]

On 2016/10/26 08:34:47, the sun wrote:
> On 2016/10/17 17:05:19, ossu wrote:
> > On 2016/10/17 15:00:46, the sun wrote:
> > > On 2016/10/17 14:54:58, ossu wrote:
> > > > https://codereview.webrtc.org/2426563003/diff/60001/webrtc/api/BUILD.gn
> > > > File webrtc/api/BUILD.gn (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.webrtc.org/2426563003/diff/60001/webrtc/api/BUILD.gn#newco...
> > > > webrtc/api/BUILD.gn:321: rtc_source_set("transport_api") {
> > > > On 2016/10/17 14:38:35, the sun wrote:
> > > > > On 2016/10/17 14:21:56, aleloi wrote:
> > > > > > On 2016/10/17 14:05:44, ossu wrote:
> > > > > > > If transport.h is in api/call, shouldn't it be part of the
call_api
> > > target
> > > > > > > instead of being its own target?
> > > > > > 
> > > > > > I'm not sure, but I think it shouldn't be merged with call_api.
> > > > > > webrtc::Transport is not tightly coupled with the other things in
> call.
> > > It's
> > > > > > used in quite a lot of other places, which do include anything else
> from
> > > > call.
> > > > > > Also, I think that splitting up large build targets in small
> independent
> > > > > > submodules is generally a good thing: it generates less dependencies
> in
> > > the
> > > > > > future and simplifies modularization. WDYT?
> > > > > 
> > > > > I was about to agree with Oskar first, but looking through the rest of
> > your
> > > CL
> > > > I
> > > > > think it should be a separate target - there's no reason for the
> rtp_rtcp
> > > > module
> > > > > to know about ALL of Call, when it just needs Transport.
> > > > 
> > > > Alright. Should the file still be in call/ then? Or directly under api?
> > > 
> > > I think it's ok if it lives under call/ - WDYT?
> > 
> > I don't know. It probably is. It's a bit weird that there's a call target
for
> > the stuff in call/, but that doesn't include that thing in call/. I'll take
> > another look at it tomorrow and try to figure it out.
> 
> The more I think about it the more it makes sense that Transport is in a
> separate target, but in the longer term it should likely live in
"api/network/"
> (or if there's a better name). Layers of the onion - transport is used by Call
> but defined by a lower-layer module (e.g. RtpRtcp, or similar). We don't want
to
> put RtpRtcp in api/ though, so either we create api/network/ or just leave it
in
> api/call/ for the time being.

(The reason I'm hesitant about "network" is that it's a very broad term and may
be confusing.)

[kjellander_webrtc, 2016-10-26 08:59:29]

On 2016/10/26 08:36:19, the sun wrote:
> On 2016/10/26 08:34:47, the sun wrote:
> > On 2016/10/17 17:05:19, ossu wrote:
> > > On 2016/10/17 15:00:46, the sun wrote:
> > > > On 2016/10/17 14:54:58, ossu wrote:
> > > > >
https://codereview.webrtc.org/2426563003/diff/60001/webrtc/api/BUILD.gn
> > > > > File webrtc/api/BUILD.gn (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.webrtc.org/2426563003/diff/60001/webrtc/api/BUILD.gn#newco...
> > > > > webrtc/api/BUILD.gn:321: rtc_source_set("transport_api") {
> > > > > On 2016/10/17 14:38:35, the sun wrote:
> > > > > > On 2016/10/17 14:21:56, aleloi wrote:
> > > > > > > On 2016/10/17 14:05:44, ossu wrote:
> > > > > > > > If transport.h is in api/call, shouldn't it be part of the
> call_api
> > > > target
> > > > > > > > instead of being its own target?
> > > > > > > 
> > > > > > > I'm not sure, but I think it shouldn't be merged with call_api.
> > > > > > > webrtc::Transport is not tightly coupled with the other things in
> > call.
> > > > It's
> > > > > > > used in quite a lot of other places, which do include anything
else
> > from
> > > > > call.
> > > > > > > Also, I think that splitting up large build targets in small
> > independent
> > > > > > > submodules is generally a good thing: it generates less
dependencies
> > in
> > > > the
> > > > > > > future and simplifies modularization. WDYT?
> > > > > > 
> > > > > > I was about to agree with Oskar first, but looking through the rest
of
> > > your
> > > > CL
> > > > > I
> > > > > > think it should be a separate target - there's no reason for the
> > rtp_rtcp
> > > > > module
> > > > > > to know about ALL of Call, when it just needs Transport.
> > > > > 
> > > > > Alright. Should the file still be in call/ then? Or directly under
api?
> > > > 
> > > > I think it's ok if it lives under call/ - WDYT?
> > > 
> > > I don't know. It probably is. It's a bit weird that there's a call target
> for
> > > the stuff in call/, but that doesn't include that thing in call/. I'll
take
> > > another look at it tomorrow and try to figure it out.

The target named similar to the directory doesn't have to contain everything in
it, it should just be "the most common case", kind of. 
See https://groups.google.com/a/chromium.org/forum/#!topic/gn-dev/5iuYFo2b35k
for more discussion about this (I was confused myself).

> > The more I think about it the more it makes sense that Transport is in a
> > separate target, but in the longer term it should likely live in
> "api/network/"
> > (or if there's a better name). Layers of the onion - transport is used by
Call
> > but defined by a lower-layer module (e.g. RtpRtcp, or similar). We don't
want
> to
> > put RtpRtcp in api/ though, so either we create api/network/ or just leave
it
> in
> > api/call/ for the time being.
> 
> (The reason I'm hesitant about "network" is that it's a very broad term and
may
> be confusing.)

I'm fine with this as-is, we can always move more later - the important thing is
to get stuff away from the root into api/, so thanks for doing that!
lgtm

[ossu, 2016-10-26 09:15:13]

On 2016/10/26 08:59:29, kjellander_webrtc wrote:
> On 2016/10/26 08:36:19, the sun wrote:
> > On 2016/10/26 08:34:47, the sun wrote:
> > > On 2016/10/17 17:05:19, ossu wrote:
> > > > On 2016/10/17 15:00:46, the sun wrote:
> > > > > On 2016/10/17 14:54:58, ossu wrote:
> > > > > >
> https://codereview.webrtc.org/2426563003/diff/60001/webrtc/api/BUILD.gn
> > > > > > File webrtc/api/BUILD.gn (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.webrtc.org/2426563003/diff/60001/webrtc/api/BUILD.gn#newco...
> > > > > > webrtc/api/BUILD.gn:321: rtc_source_set("transport_api") {
> > > > > > On 2016/10/17 14:38:35, the sun wrote:
> > > > > > > On 2016/10/17 14:21:56, aleloi wrote:
> > > > > > > > On 2016/10/17 14:05:44, ossu wrote:
> > > > > > > > > If transport.h is in api/call, shouldn't it be part of the
> > call_api
> > > > > target
> > > > > > > > > instead of being its own target?
> > > > > > > > 
> > > > > > > > I'm not sure, but I think it shouldn't be merged with call_api.
> > > > > > > > webrtc::Transport is not tightly coupled with the other things
in
> > > call.
> > > > > It's
> > > > > > > > used in quite a lot of other places, which do include anything
> else
> > > from
> > > > > > call.
> > > > > > > > Also, I think that splitting up large build targets in small
> > > independent
> > > > > > > > submodules is generally a good thing: it generates less
> dependencies
> > > in
> > > > > the
> > > > > > > > future and simplifies modularization. WDYT?
> > > > > > > 
> > > > > > > I was about to agree with Oskar first, but looking through the
rest
> of
> > > > your
> > > > > CL
> > > > > > I
> > > > > > > think it should be a separate target - there's no reason for the
> > > rtp_rtcp
> > > > > > module
> > > > > > > to know about ALL of Call, when it just needs Transport.
> > > > > > 
> > > > > > Alright. Should the file still be in call/ then? Or directly under
> api?
> > > > > 
> > > > > I think it's ok if it lives under call/ - WDYT?
> > > > 
> > > > I don't know. It probably is. It's a bit weird that there's a call
target
> > for
> > > > the stuff in call/, but that doesn't include that thing in call/. I'll
> take
> > > > another look at it tomorrow and try to figure it out.
> 
> The target named similar to the directory doesn't have to contain everything
in
> it, it should just be "the most common case", kind of. 
> See https://groups.google.com/a/chromium.org/forum/#!topic/gn-dev/5iuYFo2b35k
> for more discussion about this (I was confused myself).
> 
> > > The more I think about it the more it makes sense that Transport is in a
> > > separate target, but in the longer term it should likely live in
> > "api/network/"
> > > (or if there's a better name). Layers of the onion - transport is used by
> Call
> > > but defined by a lower-layer module (e.g. RtpRtcp, or similar). We don't
> want
> > to
> > > put RtpRtcp in api/ though, so either we create api/network/ or just leave
> it
> > in
> > > api/call/ for the time being.
> > 
> > (The reason I'm hesitant about "network" is that it's a very broad term and
> may
> > be confusing.)
> 
> I'm fine with this as-is, we can always move more later - the important thing
is
> to get stuff away from the root into api/, so thanks for doing that!
> lgtm

Yeah, alright. I've not come to any great insight on where I'd rather have it.
LGTM as well.

[danilchap, 2016-10-26 09:41:18]

Please postpone deleting transport.h in root since that will break dependent
project that include it there, deprecate it there instead.

[kjellander_webrtc, 2016-11-16 10:44:22]

On 2016/10/26 09:41:18, danilchap wrote:
> Please postpone deleting transport.h in root since that will break dependent
> project that include it there, deprecate it there instead.

Agreed, please leave the root transport.h in place and just include the new
header in that file until all downstream projects have been updated.
I'd love to see this land soon.

[kjellander_webrtc, 2016-11-16 10:45:13]

Description was changed from

==========
Moved transport.h from webrtc/ to webrtc/api, created build target and updated
dependencies.

transport.h defines an interface for sending rtp and rtcp packets,
which is used by MediaChannel in webrtc/media/engine,
{Audio|Video}{Send|Receive}Stream and a few other
places. Targets in webrtc/api should in general only depend on other
targets in webrtc/api. The target webrtc/api:call_api depends on this
target. This change also makes webrtc/voice_engine pass GN's header
include checker and is needed in order for webrtc/api:call_api to pass
it.

NOTRY=True

BUG=webrtc:5589
==========

to

==========
Moved transport.h from webrtc/ to webrtc/api, created build target and updated
dependencies.

transport.h defines an interface for sending rtp and rtcp packets,
which is used by MediaChannel in webrtc/media/engine,
{Audio|Video}{Send|Receive}Stream and a few other
places. Targets in webrtc/api should in general only depend on other
targets in webrtc/api. The target webrtc/api:call_api depends on this
target. This change also makes webrtc/voice_engine pass GN's header
include checker and is needed in order for webrtc/api:call_api to pass
it.

NOTRY=True

BUG=webrtc:5589, webrtc:5878
==========

[kjellander_webrtc, 2016-11-16 10:45:34]

On 2016/11/16 10:44:22, kjellander_webrtc wrote:
> On 2016/10/26 09:41:18, danilchap wrote:
> > Please postpone deleting transport.h in root since that will break dependent
> > project that include it there, deprecate it there instead.
> 
> Agreed, please leave the root transport.h in place and just include the new
> header in that file until all downstream projects have been updated.
> I'd love to see this land soon.

I added webrtc:5878 to the BUG= field, since this CL addresses exactly that.

[aleloi, 2016-11-21 12:38:19]

Patchset #2 (id:80001) has been deleted

[aleloi, 2016-11-21 12:52:49]

Patchset #2 (id:100001) has been deleted

[aleloi, 2016-11-21 12:57:52]

Description was changed from

==========
Moved transport.h from webrtc/ to webrtc/api, created build target and updated
dependencies.

transport.h defines an interface for sending rtp and rtcp packets,
which is used by MediaChannel in webrtc/media/engine,
{Audio|Video}{Send|Receive}Stream and a few other
places. Targets in webrtc/api should in general only depend on other
targets in webrtc/api. The target webrtc/api:call_api depends on this
target. This change also makes webrtc/voice_engine pass GN's header
include checker and is needed in order for webrtc/api:call_api to pass
it.

NOTRY=True

BUG=webrtc:5589, webrtc:5878
==========

to

==========
Moved transport.h from webrtc/ to webrtc/api, created build target and updated
WebRTC dependencies.

transport.h defines an interface for sending rtp and rtcp packets,
which is used by MediaChannel in webrtc/media/engine,
{Audio|Video}{Send|Receive}Stream and in a few other
places. It was part of the build target //webrtc:webrtc, which is a monolithic
target with 
all webrtc production code. This CL moves the header to its own target in
webrtc/api
and deprecates the old location.

Targets in webrtc/api should in general only depend on other
targets in webrtc/api. The target webrtc/api:call_api depends on
transport.h. This change also makes webrtc/voice_engine pass GN's header
include checker and is needed in order for webrtc/api:call_api to pass
it.

transport.h will be completely removed in a follow-up CL in a few weeks
after clients have updated their includes.

NOTRY=True

BUG=webrtc:5589, webrtc:5878
==========

[aleloi, 2016-11-21 13:07:41]

Hi again!

Sorry for waiting this long... This change breaks a single header out of
//webrtc:webrtc.

The problem last time was that this change broke both internal and probably
external projects. Now I've included the new location from the old and added a
deprecation notice. I've also removed mentions of GYP and updated GN
dependencies that have changed since last time.

I'll check if it breaks internal projects before committing. 

Could you take a quick look on the contents of //webrtc/transport.h?

kwiberg@: can you take a quick look at this change in general?

[kjellander_webrtc, 2016-11-21 13:16:51]

On 2016/11/21 13:07:41, aleloi wrote:
> Hi again!
> 
> Sorry for waiting this long... This change breaks a single header out of
> //webrtc:webrtc.
> 
> The problem last time was that this change broke both internal and probably
> external projects. Now I've included the new location from the old and added a
> deprecation notice. I've also removed mentions of GYP and updated GN
> dependencies that have changed since last time.
> 
> I'll check if it breaks internal projects before committing. 
> 
> Could you take a quick look on the contents of //webrtc/transport.h?
> 
> kwiberg@: can you take a quick look at this change in general?

//webrtc/transport.h lgtm
Thanks for picking this up again. Please run chromium trybots on the patch + the
internal one.

[kwiberg-webrtc, 2016-11-25 12:16:26]

lgtm, but see comments

https://codereview.webrtc.org/2426563003/diff/140001/webrtc/BUILD.gn
File webrtc/BUILD.gn (right):

https://codereview.webrtc.org/2426563003/diff/140001/webrtc/BUILD.gn#newcode237
webrtc/BUILD.gn:237: # location in webrtc/api/call/transport.h
Link to the bug from here too.

https://codereview.webrtc.org/2426563003/diff/140001/webrtc/transport.h
File webrtc/transport.h (right):

https://codereview.webrtc.org/2426563003/diff/140001/webrtc/transport.h#newco...
webrtc/transport.h:16: // TODO(aleloi): delete this file when all dependencies
are updated.
It's probably best to have a bug to track this (and to point to it from here).

[aleloi, 2016-11-28 13:13:14]

Description was changed from

==========
Moved transport.h from webrtc/ to webrtc/api, created build target and updated
WebRTC dependencies.

transport.h defines an interface for sending rtp and rtcp packets,
which is used by MediaChannel in webrtc/media/engine,
{Audio|Video}{Send|Receive}Stream and in a few other
places. It was part of the build target //webrtc:webrtc, which is a monolithic
target with 
all webrtc production code. This CL moves the header to its own target in
webrtc/api
and deprecates the old location.

Targets in webrtc/api should in general only depend on other
targets in webrtc/api. The target webrtc/api:call_api depends on
transport.h. This change also makes webrtc/voice_engine pass GN's header
include checker and is needed in order for webrtc/api:call_api to pass
it.

transport.h will be completely removed in a follow-up CL in a few weeks
after clients have updated their includes.

NOTRY=True

BUG=webrtc:5589, webrtc:5878
==========

to

==========
Moved transport.h from webrtc/ to webrtc/api, created build target and updated
WebRTC dependencies.

transport.h defines an interface for sending rtp and rtcp packets,
which is used by MediaChannel in webrtc/media/engine,
{Audio|Video}{Send|Receive}Stream and in a few other
places. It was part of the build target //webrtc:webrtc, which is a monolithic
target with 
all webrtc production code. This CL moves the header to its own target in
webrtc/api
and deprecates the old location.

Targets in webrtc/api should in general only depend on other
targets in webrtc/api. The target webrtc/api:call_api depends on
transport.h. This change also makes webrtc/voice_engine pass GN's header
include checker and is needed in order for webrtc/api:call_api to pass
it.

transport.h will be completely removed in a follow-up CL in a few weeks
after clients have updated their includes.

NOTRY=True

BUG=webrtc:5589, webrtc:5878, webrt:6785
==========

[aleloi, 2016-11-28 13:13:27]

Description was changed from

==========
Moved transport.h from webrtc/ to webrtc/api, created build target and updated
WebRTC dependencies.

transport.h defines an interface for sending rtp and rtcp packets,
which is used by MediaChannel in webrtc/media/engine,
{Audio|Video}{Send|Receive}Stream and in a few other
places. It was part of the build target //webrtc:webrtc, which is a monolithic
target with 
all webrtc production code. This CL moves the header to its own target in
webrtc/api
and deprecates the old location.

Targets in webrtc/api should in general only depend on other
targets in webrtc/api. The target webrtc/api:call_api depends on
transport.h. This change also makes webrtc/voice_engine pass GN's header
include checker and is needed in order for webrtc/api:call_api to pass
it.

transport.h will be completely removed in a follow-up CL in a few weeks
after clients have updated their includes.

NOTRY=True

BUG=webrtc:5589, webrtc:5878, webrt:6785
==========

to

==========
Moved transport.h from webrtc/ to webrtc/api, created build target and updated
WebRTC dependencies.

transport.h defines an interface for sending rtp and rtcp packets,
which is used by MediaChannel in webrtc/media/engine,
{Audio|Video}{Send|Receive}Stream and in a few other
places. It was part of the build target //webrtc:webrtc, which is a monolithic
target with 
all webrtc production code. This CL moves the header to its own target in
webrtc/api
and deprecates the old location.

Targets in webrtc/api should in general only depend on other
targets in webrtc/api. The target webrtc/api:call_api depends on
transport.h. This change also makes webrtc/voice_engine pass GN's header
include checker and is needed in order for webrtc/api:call_api to pass
it.

transport.h will be completely removed in a follow-up CL in a few weeks
after clients have updated their includes.

NOTRY=True

BUG=webrtc:5589, webrtc:5878, webrtc:6785
==========

[aleloi, 2016-11-28 13:16:48]

The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 13:16:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-11-28 13:18:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-28 13:18:45]

Dry run: This issue passed the CQ dry run.

[aleloi, 2016-11-28 14:49:43]

The CQ bit was checked by aleloi@webrtc.org

[aleloi, 2016-11-28 14:49:44]

The patchset sent to the CQ was uploaded after l-g-t-m from ossu@webrtc.org,
kjellander@webrtc.org, kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2426563003/#ps160001 (title:
"Created issue webrtc:6785 as reminder and linked to it in comments.")

[commit-bot: I haz the power, 2016-11-28 14:49:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-11-28 15:02:13]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1480344583724670,
"parent_rev": "4bbc99abc42a1277cb04f48403b8da0809f5ebb0", "commit_rev":
"40d695d6a3a095e8bb691f1a2aadd9a4b9195716"}

[commit-bot: I haz the power, 2016-11-28 15:02:16]

Description was changed from

==========
Moved transport.h from webrtc/ to webrtc/api, created build target and updated
WebRTC dependencies.

transport.h defines an interface for sending rtp and rtcp packets,
which is used by MediaChannel in webrtc/media/engine,
{Audio|Video}{Send|Receive}Stream and in a few other
places. It was part of the build target //webrtc:webrtc, which is a monolithic
target with 
all webrtc production code. This CL moves the header to its own target in
webrtc/api
and deprecates the old location.

Targets in webrtc/api should in general only depend on other
targets in webrtc/api. The target webrtc/api:call_api depends on
transport.h. This change also makes webrtc/voice_engine pass GN's header
include checker and is needed in order for webrtc/api:call_api to pass
it.

transport.h will be completely removed in a follow-up CL in a few weeks
after clients have updated their includes.

NOTRY=True

BUG=webrtc:5589, webrtc:5878, webrtc:6785
==========

to

==========
Moved transport.h from webrtc/ to webrtc/api, created build target and updated
WebRTC dependencies.

transport.h defines an interface for sending rtp and rtcp packets,
which is used by MediaChannel in webrtc/media/engine,
{Audio|Video}{Send|Receive}Stream and in a few other
places. It was part of the build target //webrtc:webrtc, which is a monolithic
target with 
all webrtc production code. This CL moves the header to its own target in
webrtc/api
and deprecates the old location.

Targets in webrtc/api should in general only depend on other
targets in webrtc/api. The target webrtc/api:call_api depends on
transport.h. This change also makes webrtc/voice_engine pass GN's header
include checker and is needed in order for webrtc/api:call_api to pass
it.

transport.h will be completely removed in a follow-up CL in a few weeks
after clients have updated their includes.

NOTRY=True

BUG=webrtc:5589, webrtc:5878, webrtc:6785
==========

[commit-bot: I haz the power, 2016-11-28 15:02:18]

Committed patchset #4 (id:160001)

[commit-bot: I haz the power, 2016-11-28 15:02:22]

Description was changed from

==========
Moved transport.h from webrtc/ to webrtc/api, created build target and updated
WebRTC dependencies.

transport.h defines an interface for sending rtp and rtcp packets,
which is used by MediaChannel in webrtc/media/engine,
{Audio|Video}{Send|Receive}Stream and in a few other
places. It was part of the build target //webrtc:webrtc, which is a monolithic
target with 
all webrtc production code. This CL moves the header to its own target in
webrtc/api
and deprecates the old location.

Targets in webrtc/api should in general only depend on other
targets in webrtc/api. The target webrtc/api:call_api depends on
transport.h. This change also makes webrtc/voice_engine pass GN's header
include checker and is needed in order for webrtc/api:call_api to pass
it.

transport.h will be completely removed in a follow-up CL in a few weeks
after clients have updated their includes.

NOTRY=True

BUG=webrtc:5589, webrtc:5878, webrtc:6785
==========

to

==========
Moved transport.h from webrtc/ to webrtc/api, created build target and updated
WebRTC dependencies.

transport.h defines an interface for sending rtp and rtcp packets,
which is used by MediaChannel in webrtc/media/engine,
{Audio|Video}{Send|Receive}Stream and in a few other
places. It was part of the build target //webrtc:webrtc, which is a monolithic
target with
all webrtc production code. This CL moves the header to its own target in
webrtc/api
and deprecates the old location.

Targets in webrtc/api should in general only depend on other
targets in webrtc/api. The target webrtc/api:call_api depends on
transport.h. This change also makes webrtc/voice_engine pass GN's header
include checker and is needed in order for webrtc/api:call_api to pass
it.

transport.h will be completely removed in a follow-up CL in a few weeks
after clients have updated their includes.

NOTRY=True

BUG=webrtc:5589, webrtc:5878, webrtc:6785

Committed: https://crrev.com/a8eb756a346682a4af66f9a9c0b87cfe1ed5b114
Cr-Commit-Position: refs/heads/master@{#15267}
==========

[commit-bot: I haz the power, 2016-11-28 15:02:24]

Patchset 4 (id:??) landed as
https://crrev.com/a8eb756a346682a4af66f9a9c0b87cfe1ed5b114
Cr-Commit-Position: refs/heads/master@{#15267}