Create //webrtc/api:libjingle_peerconnection_api + refactorings.

Create //webrtc/api:libjingle_peerconnection_api + refactorings.

Create a new target //webrtc/api:libjingle_peerconnection_api and start moving
things into it. Move remaining parts of //webrtc/api:libjingle_peerconnection
to //webrtc/pc:libjingle_peerconnection.

Moved the RTCStatsCollectorCallback into its own header file, so that
PeerConnectionInterface can include that instead of pulling in
RTCStatsCollector and PeerConnection and everything.

Separated cricket::MediaType into its own header/source set, so that it
can be used in the api.

BUG=webrtc:5883
Review-Url: https://codereview.webrtc.org/2514883002
Cr-Commit-Position: refs/heads/master@{#16210}
Committed: https://chromium.googlesource.com/external/webrtc/+/7bb87ee4e8b6620ebb647f2573d429b78b42440c

[Taylor Brandstetter, 2016-11-18 21:10:31]

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

[Taylor Brandstetter, 2016-11-18 21:10:32]

https://codereview.webrtc.org/2514883002/diff/1/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (left):

https://codereview.webrtc.org/2514883002/diff/1/webrtc/api/BUILD.gn#oldcode74
webrtc/api/BUILD.gn:74: "mediaconstraintsinterface.h",
mediaconstraintsinterface.h would go in the api target.

https://codereview.webrtc.org/2514883002/diff/1/webrtc/api/mediatype.h
File webrtc/api/mediatype.h (right):

https://codereview.webrtc.org/2514883002/diff/1/webrtc/api/mediatype.h#newcode28
webrtc/api/mediatype.h:28: #endif  // WEBRTC_API_MEDIATYPE_H_
We could call this file "mediatypes.h" (plural) and use it for various
media-related enums and value types. For example, cricket::MediaConfig could
also go here.

[the sun, 2016-11-22 12:59:49]

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

[the sun, 2016-11-22 12:59:50]

https://codereview.webrtc.org/2514883002/diff/1/webrtc/api/mediatype.h
File webrtc/api/mediatype.h (right):

https://codereview.webrtc.org/2514883002/diff/1/webrtc/api/mediatype.h#newcode28
webrtc/api/mediatype.h:28: #endif  // WEBRTC_API_MEDIATYPE_H_
On 2016/11/18 21:10:31, Taylor Brandstetter wrote:
> We could call this file "mediatypes.h" (plural) and use it for various
> media-related enums and value types. For example, cricket::MediaConfig could
> also go here.

Could we even call it media_types.h? I believe most files in the repo uses _
between words in file names - it would be nice if we could make it consistent,
at least when adding new files.

[the sun, 2016-11-22 13:08:22]

https://codereview.webrtc.org/2514883002/diff/1/webrtc/api/mediatype.cc
File webrtc/api/mediatype.cc (right):

https://codereview.webrtc.org/2514883002/diff/1/webrtc/api/mediatype.cc#newco...
webrtc/api/mediatype.cc:28: default:
I think it's preferred to not have the default clause nowadays, as we get
compiler warnings then.

[the sun, 2016-11-22 13:08:51]

Description was changed from

==========
Created a libjingle_peerconnection_api target and started separating
things into it.

Moved the RTCStatsCollectorCallback into its own header file, so that
PeerConnectionInterface can include that instead of pulling in
RTCStatsCollector and PeerConnection and everything.

Separated cricket::MediaType into its own header/source set, so that it
can be used in the api.

BUG=
==========

to

==========
Created a libjingle_peerconnection_api target and started separating
things into it.

Moved the RTCStatsCollectorCallback into its own header file, so that
PeerConnectionInterface can include that instead of pulling in
RTCStatsCollector and PeerConnection and everything.

Separated cricket::MediaType into its own header/source set, so that it
can be used in the api.

BUG=
==========

[the sun, 2016-11-22 13:08:51]

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

[ossu, 2016-11-22 15:36:16]

I've uploaded a second patch-set that moves a lot of the files and it's nearly
impossible to review, I'm afraid.

Some highlights:
- I've added the proxy classes into the api target (and retained them in api/),
since they only rely on the interfaces and seems to be used by other projects
directly.

- The same goes for notifier.h

- There seems to be projects using the jsep stuff directly. (more than just
jsep.h, that is). Perhaps the interface of jsepsessiondescription et al should
be cleaned up, so that it doesn't need to pull in other stuff but still be
usable from outside.

- There's only one peerconnection_unittest, although some of it covers the api
parts. Add another unittest target for the API or I might've kept too much in
api/. proxy.h is one example.

Clearly this cannot land as it is, since it changes the API, and would require a
deprecation period, etc. Any comments are appreciated, though!

https://codereview.webrtc.org/2514883002/diff/1/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (left):

https://codereview.webrtc.org/2514883002/diff/1/webrtc/api/BUILD.gn#oldcode74
webrtc/api/BUILD.gn:74: "mediaconstraintsinterface.h",
On 2016/11/18 21:10:31, Taylor Brandstetter wrote:
> mediaconstraintsinterface.h would go in the api target.

Done.

https://codereview.webrtc.org/2514883002/diff/1/webrtc/api/mediatype.cc
File webrtc/api/mediatype.cc (right):

https://codereview.webrtc.org/2514883002/diff/1/webrtc/api/mediatype.cc#newco...
webrtc/api/mediatype.cc:28: default:
On 2016/11/22 13:08:22, the sun wrote:
> I think it's preferred to not have the default clause nowadays, as we get
> compiler warnings then.

Sounds good, but I'll avoid doing any functional changes (if this can be called
that) in this CL.

https://codereview.webrtc.org/2514883002/diff/1/webrtc/api/mediatype.h
File webrtc/api/mediatype.h (right):

https://codereview.webrtc.org/2514883002/diff/1/webrtc/api/mediatype.h#newcode28
webrtc/api/mediatype.h:28: #endif  // WEBRTC_API_MEDIATYPE_H_
On 2016/11/22 12:59:50, the sun wrote:
> On 2016/11/18 21:10:31, Taylor Brandstetter wrote:
> > We could call this file "mediatypes.h" (plural) and use it for various
> > media-related enums and value types. For example, cricket::MediaConfig could
> > also go here.
> 
> Could we even call it media_types.h? I believe most files in the repo uses _
> between words in file names - it would be nice if we could make it consistent,
> at least when adding new files.

We could. I was about to say everything else in api/ has names that aren't
separated by underscores, but it seems to be a bit of a mixed bag: the
peerconnection stuff is like that, but stuff under call uses underscores. I
believe this should be normalized across api/ and I prefer underscores as well.

[ossu, 2016-11-29 13:59:17]

ossu@webrtc.org changed reviewers:
+ magjed@webrtc.org

[ossu, 2016-11-29 13:59:18]

+ magjed specifically for the changes to androidtests.

I'm looking at moving the peerconnection implementation from api/ to pc/ and
with that the unittests. This includes the android unittests. PTAL and sorry for
the humongous CL. Rietveld isn't great at visualizing moves.

[magjed_webrtc, 2016-11-29 16:27:31]

On 2016/11/29 13:59:18, ossu wrote:
> + magjed specifically for the changes to androidtests.
> 
> I'm looking at moving the peerconnection implementation from api/ to pc/ and
> with that the unittests. This includes the android unittests. PTAL and sorry
for
> the humongous CL. Rietveld isn't great at visualizing moves.

So there is no change at all to androidtests/, just moving it?

We want to move all android files under webrtc/sdk/android eventually. Maybe
it's best to create an androidtest folder there now so we can move it to the
right place directly.

[magjed_webrtc, 2016-11-30 12:47:51]

On 2016/11/29 16:27:31, magjed_webrtc wrote:
> On 2016/11/29 13:59:18, ossu wrote:
> > + magjed specifically for the changes to androidtests.
> > 
> > I'm looking at moving the peerconnection implementation from api/ to pc/ and
> > with that the unittests. This includes the android unittests. PTAL and sorry
> for
> > the humongous CL. Rietveld isn't great at visualizing moves.
> 
> So there is no change at all to androidtests/, just moving it?
> 
> We want to move all android files under webrtc/sdk/android eventually. Maybe
> it's best to create an androidtest folder there now so we can move it to the
> right place directly.

I discussed this with Sami. We want to move webrtc/api/androidtests/ to
webrtc/sdk/android/instrumentationtests/ and add a
webrtc/sdk/android/instrumentationtests/BUILD.gn.

[ossu, 2016-11-30 12:51:13]

On 2016/11/30 12:47:51, magjed_webrtc wrote:
> On 2016/11/29 16:27:31, magjed_webrtc wrote:
> > On 2016/11/29 13:59:18, ossu wrote:
> > > + magjed specifically for the changes to androidtests.
> > > 
> > > I'm looking at moving the peerconnection implementation from api/ to pc/
and
> > > with that the unittests. This includes the android unittests. PTAL and
sorry
> > for
> > > the humongous CL. Rietveld isn't great at visualizing moves.
> > 
> > So there is no change at all to androidtests/, just moving it?
> > 
> > We want to move all android files under webrtc/sdk/android eventually. Maybe
> > it's best to create an androidtest folder there now so we can move it to the
> > right place directly.
> 
> I discussed this with Sami. We want to move webrtc/api/androidtests/ to
> webrtc/sdk/android/instrumentationtests/ and add a
> webrtc/sdk/android/instrumentationtests/BUILD.gn.

Sounds good. Would you like to do this separately from this CL, perhaps? If so,
I'll remove any changes from androidtests here.
If you do so before I move the rest of the peerconnection stuff out, it would
still make sense. Otherwise we'll have dependencies going back and forth between
api/ and pc/ for a while, which gets real messy.

Oh, and yes, I'm only moving, not changing anything but possibly include paths.

[magjed_webrtc, 2016-11-30 12:54:59]

On 2016/11/30 12:51:13, ossu wrote:
> On 2016/11/30 12:47:51, magjed_webrtc wrote:
> > On 2016/11/29 16:27:31, magjed_webrtc wrote:
> > > On 2016/11/29 13:59:18, ossu wrote:
> > > > + magjed specifically for the changes to androidtests.
> > > > 
> > > > I'm looking at moving the peerconnection implementation from api/ to pc/
> and
> > > > with that the unittests. This includes the android unittests. PTAL and
> sorry
> > > for
> > > > the humongous CL. Rietveld isn't great at visualizing moves.
> > > 
> > > So there is no change at all to androidtests/, just moving it?
> > > 
> > > We want to move all android files under webrtc/sdk/android eventually.
Maybe
> > > it's best to create an androidtest folder there now so we can move it to
the
> > > right place directly.
> > 
> > I discussed this with Sami. We want to move webrtc/api/androidtests/ to
> > webrtc/sdk/android/instrumentationtests/ and add a
> > webrtc/sdk/android/instrumentationtests/BUILD.gn.
> 
> Sounds good. Would you like to do this separately from this CL, perhaps? If
so,
> I'll remove any changes from androidtests here.
> If you do so before I move the rest of the peerconnection stuff out, it would
> still make sense. Otherwise we'll have dependencies going back and forth
between
> api/ and pc/ for a while, which gets real messy.
> 
> Oh, and yes, I'm only moving, not changing anything but possibly include
paths.

Yeah I can upload a CL moving just webrtc/api/androidtests. Will do that right
now.

[magjed_webrtc, 2016-12-01 11:49:32]

webrtc/api/androidtests has been moved to webrtc/sdk/android in
https://codereview.webrtc.org/2541823002/. I'm removing myself as reviewer.

[ossu, 2016-12-01 11:52:51]

ossu@webrtc.org changed reviewers:
- magjed@webrtc.org

[ossu, 2016-12-01 11:52:52]

Great stuff! Thanks!
(also removed you as a reviewer for real)

[Taylor Brandstetter, 2016-12-02 04:15:29]

I'm not sure what folder the Android stuff is ultimately destined for. But
disregarding that, this CL is a big step in the right direction. Thanks again
for taking this task on.

https://codereview.webrtc.org/2514883002/diff/20001/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/2514883002/diff/20001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:46: rtc_static_library("libjingle_peerconnection_api") {
Do we need to keep around an api:libjingle_peerconnection target temporarily, so
that Chromium will still build?

https://codereview.webrtc.org/2514883002/diff/20001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:52: "fakemetricsobserver.h",
Does this need to be in the API target?

[Taylor Brandstetter, 2016-12-02 04:17:45]

On 2016/12/02 04:15:29, Taylor Brandstetter wrote:
> I'm not sure what folder the Android stuff is ultimately destined for.

Oh, I just saw magjed's comments; nevermind.

[ossu, 2016-12-07 14:29:16]

Patchset #3 (id:30001) has been deleted

[ossu, 2016-12-08 15:11:00]

Patchset #4 (id:50001) has been deleted

[ossu, 2016-12-08 19:16:40]

I've moved some things back into api/ and I've run through internal builds to
try and find out what needs redirecting .h-files in api/ to still build and run.
There are some files included by other projects that don't need to be. I'll
clean that up separately before trying to land this.
I'll go through chromium as well tomorrow, to make sure it will still build.

https://codereview.webrtc.org/2514883002/diff/20001/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/2514883002/diff/20001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:46: rtc_static_library("libjingle_peerconnection_api") {
On 2016/12/02 04:15:29, Taylor Brandstetter wrote:
> Do we need to keep around an api:libjingle_peerconnection target temporarily,
so
> that Chromium will still build?

Yes, you're right! I've added this back in just depending on the same target in
pc/.

https://codereview.webrtc.org/2514883002/diff/20001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:52: "fakemetricsobserver.h",
On 2016/12/02 04:15:29, Taylor Brandstetter wrote:
> Does this need to be in the API target?

I'm not sure. Having mocks and possibly fakes for classes and interfaces in api/
seems reasonable to me. That allows others to write their tests without having
to keep their own mocks up-to-date.

That said, in this case, it's more of a practical issue. It's used by both pc/
and p2p/base/ and I believe putting it in pc/ would get us a circular
dependency. I'm not sure I want to resolve that as part of this CL, but I guess
I could.

[Taylor Brandstetter, 2016-12-08 20:14:35]

https://codereview.webrtc.org/2514883002/diff/20001/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/2514883002/diff/20001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:52: "fakemetricsobserver.h",
On 2016/12/08 19:16:39, ossu wrote:
> On 2016/12/02 04:15:29, Taylor Brandstetter wrote:
> > Does this need to be in the API target?
> 
> I'm not sure. Having mocks and possibly fakes for classes and interfaces in
api/
> seems reasonable to me. That allows others to write their tests without having
> to keep their own mocks up-to-date.
> 
> That said, in this case, it's more of a practical issue. It's used by both pc/
> and p2p/base/ and I believe putting it in pc/ would get us a circular
> dependency. I'm not sure I want to resolve that as part of this CL, but I
guess
> I could.

Ah yes, I forgot about that. Though ideally it should be in a separate target
that only test code depends on.

[ossu, 2016-12-09 15:15:27]

ossu@webrtc.org changed reviewers:
+ miu@chromium.org, noahric@chromium.org, perkj@webrtc.org

[ossu, 2016-12-09 15:15:29]

I've added a few more header files that redirect into pc/ to get chromium to
build. I've also added perkj, miu and noahric as they all seem involved with
files including things that I've now started to move out of api/.

Since it's hard to get a good overview of this CL, I've put a note on each
header file in api/ that I added just as redirects into pc/.

I'm not 100% certain all of them should move into pc/. Specifically
test/fakeconstraints.h might actually belong in api/test, and in a specific
_test target. This might apply for all the fakes I've moved.

There are other things that are included from the outside that isn't clear to me
it should be a part of our public api/. We can either solve that as part of this
CL, or, if it requires larger redesigns, leave the redirecting headers as warts
to fix later. I'll be sure to add bugs for them in that case.

https://codereview.webrtc.org/2514883002/diff/70001/webrtc/api/datachannel.h
File webrtc/api/datachannel.h (right):

https://codereview.webrtc.org/2514883002/diff/70001/webrtc/api/datachannel.h#...
webrtc/api/datachannel.h:16: #include "webrtc/pc/datachannel.h"
Added for backwards-compatibility.

https://codereview.webrtc.org/2514883002/diff/70001/webrtc/api/jsepsessiondes...
File webrtc/api/jsepsessiondescription.h (right):

https://codereview.webrtc.org/2514883002/diff/70001/webrtc/api/jsepsessiondes...
webrtc/api/jsepsessiondescription.h:16: #include
"webrtc/pc/jsepsessiondescription.h"
Added for backwards-compatibility.

https://codereview.webrtc.org/2514883002/diff/70001/webrtc/api/mediacontroller.h
File webrtc/api/mediacontroller.h (right):

https://codereview.webrtc.org/2514883002/diff/70001/webrtc/api/mediacontrolle...
webrtc/api/mediacontroller.h:16: #include "webrtc/pc/mediacontroller.h"
Added for backwards-compatibility.

https://codereview.webrtc.org/2514883002/diff/70001/webrtc/api/mediastream.h
File webrtc/api/mediastream.h (right):

https://codereview.webrtc.org/2514883002/diff/70001/webrtc/api/mediastream.h#...
webrtc/api/mediastream.h:16: #include "webrtc/pc/mediastream.h"
Added for backwards-compatibility.

https://codereview.webrtc.org/2514883002/diff/70001/webrtc/api/mediastreamtra...
File webrtc/api/mediastreamtrack.h (right):

https://codereview.webrtc.org/2514883002/diff/70001/webrtc/api/mediastreamtra...
webrtc/api/mediastreamtrack.h:16: #include "webrtc/pc/mediastreamtrack.h"
Added for backwards-compatibility.

https://codereview.webrtc.org/2514883002/diff/70001/webrtc/api/rtpsender.h
File webrtc/api/rtpsender.h (right):

https://codereview.webrtc.org/2514883002/diff/70001/webrtc/api/rtpsender.h#ne...
webrtc/api/rtpsender.h:16: #include "webrtc/pc/rtpsender.h"
Added for backwards-compatibility.

https://codereview.webrtc.org/2514883002/diff/70001/webrtc/api/streamcollecti...
File webrtc/api/streamcollection.h (right):

https://codereview.webrtc.org/2514883002/diff/70001/webrtc/api/streamcollecti...
webrtc/api/streamcollection.h:16: #include "webrtc/pc/streamcollection.h"
Added for backwards-compatibility.

https://codereview.webrtc.org/2514883002/diff/70001/webrtc/api/test/fakeconst...
File webrtc/api/test/fakeconstraints.h (right):

https://codereview.webrtc.org/2514883002/diff/70001/webrtc/api/test/fakeconst...
webrtc/api/test/fakeconstraints.h:16: #include
"webrtc/pc/test/fakeconstraints.h"
Added for backwards-compatibility.

https://codereview.webrtc.org/2514883002/diff/70001/webrtc/api/videotracksour...
File webrtc/api/videotracksource.h (right):

https://codereview.webrtc.org/2514883002/diff/70001/webrtc/api/videotracksour...
webrtc/api/videotracksource.h:16: #include "webrtc/pc/videotracksource.h"
Added for backwards-compatibility.

[ossu, 2016-12-09 15:19:10]

Description was changed from

==========
Created a libjingle_peerconnection_api target and started separating
things into it.

Moved the RTCStatsCollectorCallback into its own header file, so that
PeerConnectionInterface can include that instead of pulling in
RTCStatsCollector and PeerConnection and everything.

Separated cricket::MediaType into its own header/source set, so that it
can be used in the api.

BUG=
==========

to

==========
Created a libjingle_peerconnection_api target and started separating
things into it.

Moved the RTCStatsCollectorCallback into its own header file, so that
PeerConnectionInterface can include that instead of pulling in
RTCStatsCollector and PeerConnection and everything.

Separated cricket::MediaType into its own header/source set, so that it
can be used in the api.

BUG=webrtc:5883
==========

[miu, 2016-12-13 21:28:55]

miu@chromium.org changed reviewers:
- miu@chromium.org

[miu, 2016-12-13 21:28:56]

I'm removing myself from this review, as I've only worked on the Chromium-side
of things for WebRTC. :)

As general guidance for the WebRTC project: We would like to see this and other
CLs move the code design/structure in this direction:

1. WebRTC/libjingle is a library shared among various browser vendors. As such,
its implementation should not attempt to own objects external to this library.
Instead, the embedder should own instances of objects created by the library.
(in reference to comments like this, which IMHO are erroneous:
https://cs.chromium.org/chromium/src/content/renderer/media/webrtc_audio_devi...)

2. Threading: Can WebRTC/libjingle use fewer threads? Can it share some of the
threads having similar purpose in the embedder (e.g., the "signaling thread" and
"browser main thread" could be the same)? (in reference to rather complex thread
start-up code seen here:
https://cs.chromium.org/chromium/src/content/renderer/media/webrtc/peer_conne...)

3. Eliminate the re-implementation of MediaStreams, if possible. In
Chromium+Blink, we already have a full implementation. The "shadow" hierarchy in
WebRTC isn't necessary (just more moving parts), and doesn't seem to add much
functionality.

4. Eliminate ref-counting: This makes the object graph impossible to reason
about in many circumstances. Ref-counting is something that should be used very
sparingly. There is a pretty good summary discussion on when/why to use
different smart pointers here:
https://www.chromium.org/developers/smart-pointer-guidelines

If you have questions, feel free to send me an e-mail. I believe perkj@ and
o1ka@chromium.org are aware of some of these issues and could provide more
direct guidance.

[ossu, 2016-12-15 12:23:02]

On 2016/12/13 21:28:56, miu wrote:
> I'm removing myself from this review, as I've only worked on the Chromium-side
> of things for WebRTC. :)
> 
> As general guidance for the WebRTC project: We would like to see this and
other
> CLs move the code design/structure in this direction:
> 
> 1. WebRTC/libjingle is a library shared among various browser vendors. As
such,
> its implementation should not attempt to own objects external to this library.
> Instead, the embedder should own instances of objects created by the library.
> (in reference to comments like this, which IMHO are erroneous:
>
https://cs.chromium.org/chromium/src/content/renderer/media/webrtc_audio_devi...)
> 
> 2. Threading: Can WebRTC/libjingle use fewer threads? Can it share some of the
> threads having similar purpose in the embedder (e.g., the "signaling thread"
and
> "browser main thread" could be the same)? (in reference to rather complex
thread
> start-up code seen here:
>
https://cs.chromium.org/chromium/src/content/renderer/media/webrtc/peer_conne...)
> 
> 3. Eliminate the re-implementation of MediaStreams, if possible. In
> Chromium+Blink, we already have a full implementation. The "shadow" hierarchy
in
> WebRTC isn't necessary (just more moving parts), and doesn't seem to add much
> functionality.
> 
> 4. Eliminate ref-counting: This makes the object graph impossible to reason
> about in many circumstances. Ref-counting is something that should be used
very
> sparingly. There is a pretty good summary discussion on when/why to use
> different smart pointers here:
> https://www.chromium.org/developers/smart-pointer-guidelines
> 
> If you have questions, feel free to send me an e-mail. I believe perkj@ and
> mailto:o1ka@chromium.org are aware of some of these issues and could provide
more
> direct guidance.

Alright, miu, thanks for the feedback!

I believe I agree with most of your points and will keep them in mind going
forward. This specific CL will just move some things around, though.

perkj, noahric have you had a look at what will be moved? Do you have any strong
objections against it?

Otherwise, I'd like to be able to land this as soon as possible, to minimize the
risk of having to do more, complicated rebasing.

[perkj_webrtc, 2016-12-15 12:33:24]

On 2016/12/15 12:23:02, ossu wrote:
> On 2016/12/13 21:28:56, miu wrote:
> > I'm removing myself from this review, as I've only worked on the
Chromium-side
> > of things for WebRTC. :)
> > 
> > As general guidance for the WebRTC project: We would like to see this and
> other
> > CLs move the code design/structure in this direction:
> > 
> > 1. WebRTC/libjingle is a library shared among various browser vendors. As
> such,
> > its implementation should not attempt to own objects external to this
library.
> > Instead, the embedder should own instances of objects created by the
library.
> > (in reference to comments like this, which IMHO are erroneous:
> >
>
https://cs.chromium.org/chromium/src/content/renderer/media/webrtc_audio_devi...)
> > 
> > 2. Threading: Can WebRTC/libjingle use fewer threads? Can it share some of
the
> > threads having similar purpose in the embedder (e.g., the "signaling thread"
> and
> > "browser main thread" could be the same)? (in reference to rather complex
> thread
> > start-up code seen here:
> >
>
https://cs.chromium.org/chromium/src/content/renderer/media/webrtc/peer_conne...)
> > 
> > 3. Eliminate the re-implementation of MediaStreams, if possible. In
> > Chromium+Blink, we already have a full implementation. The "shadow"
hierarchy
> in
> > WebRTC isn't necessary (just more moving parts), and doesn't seem to add
much
> > functionality.
> > 
> > 4. Eliminate ref-counting: This makes the object graph impossible to reason
> > about in many circumstances. Ref-counting is something that should be used
> very
> > sparingly. There is a pretty good summary discussion on when/why to use
> > different smart pointers here:
> > https://www.chromium.org/developers/smart-pointer-guidelines
> > 
> > If you have questions, feel free to send me an e-mail. I believe perkj@ and
> > mailto:o1ka@chromium.org are aware of some of these issues and could provide
> more
> > direct guidance.
> 
> Alright, miu, thanks for the feedback!
> 
> I believe I agree with most of your points and will keep them in mind going
> forward. This specific CL will just move some things around, though.
> 
> perkj, noahric have you had a look at what will be moved? Do you have any
strong
> objections against it?
> 
> Otherwise, I'd like to be able to land this as soon as possible, to minimize
the
> risk of having to do more, complicated rebasing.

I think Taylor is the right reviewer. I have no strong opionions.

[ossu, 2016-12-15 13:14:21]

On 2016/12/15 12:33:24, perkj_webrtc wrote:
> On 2016/12/15 12:23:02, ossu wrote:
> > On 2016/12/13 21:28:56, miu wrote:
> > > I'm removing myself from this review, as I've only worked on the
> Chromium-side
> > > of things for WebRTC. :)
> > > 
> > > As general guidance for the WebRTC project: We would like to see this and
> > other
> > > CLs move the code design/structure in this direction:
> > > 
> > > 1. WebRTC/libjingle is a library shared among various browser vendors. As
> > such,
> > > its implementation should not attempt to own objects external to this
> library.
> > > Instead, the embedder should own instances of objects created by the
> library.
> > > (in reference to comments like this, which IMHO are erroneous:
> > >
> >
>
https://cs.chromium.org/chromium/src/content/renderer/media/webrtc_audio_devi...)
> > > 
> > > 2. Threading: Can WebRTC/libjingle use fewer threads? Can it share some of
> the
> > > threads having similar purpose in the embedder (e.g., the "signaling
thread"
> > and
> > > "browser main thread" could be the same)? (in reference to rather complex
> > thread
> > > start-up code seen here:
> > >
> >
>
https://cs.chromium.org/chromium/src/content/renderer/media/webrtc/peer_conne...)
> > > 
> > > 3. Eliminate the re-implementation of MediaStreams, if possible. In
> > > Chromium+Blink, we already have a full implementation. The "shadow"
> hierarchy
> > in
> > > WebRTC isn't necessary (just more moving parts), and doesn't seem to add
> much
> > > functionality.
> > > 
> > > 4. Eliminate ref-counting: This makes the object graph impossible to
reason
> > > about in many circumstances. Ref-counting is something that should be used
> > very
> > > sparingly. There is a pretty good summary discussion on when/why to use
> > > different smart pointers here:
> > > https://www.chromium.org/developers/smart-pointer-guidelines
> > > 
> > > If you have questions, feel free to send me an e-mail. I believe perkj@
and
> > > mailto:o1ka@chromium.org are aware of some of these issues and could
provide
> > more
> > > direct guidance.
> > 
> > Alright, miu, thanks for the feedback!
> > 
> > I believe I agree with most of your points and will keep them in mind going
> > forward. This specific CL will just move some things around, though.
> > 
> > perkj, noahric have you had a look at what will be moved? Do you have any
> strong
> > objections against it?
> > 
> > Otherwise, I'd like to be able to land this as soon as possible, to minimize
> the
> > risk of having to do more, complicated rebasing.
> 
> I think Taylor is the right reviewer. I have no strong opionions.

Great, thanks!

[Taylor Brandstetter, 2016-12-15 21:31:55]

lgtm from my perspective. It may be good to have someone else more familiar with
the build system review to see if there's anything I missed.

https://codereview.webrtc.org/2514883002/diff/70001/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/2514883002/diff/70001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:45: "fakemetricsobserver.cc",
I'd still add a "TODO: Put this in a separate target for api mock/fake classes",
in case someone else encounters this later and wonders why it's here.

[ossu, 2016-12-16 09:45:33]

https://codereview.webrtc.org/2514883002/diff/70001/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/2514883002/diff/70001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:45: "fakemetricsobserver.cc",
On 2016/12/15 21:31:54, Taylor Brandstetter wrote:
> I'd still add a "TODO: Put this in a separate target for api mock/fake
classes",
> in case someone else encounters this later and wonders why it's here.

Will do!

[ossu, 2016-12-16 13:55:16]

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

[ossu, 2016-12-16 13:55:17]

Tested the change on chromium and internal projects and hit no errors.

Adding kjellander to review build related changes. 
TL;DR: I'm moving the whole peerconnection implementation from api/ to pc/,
leaving the public interfaces in api/. I've had to add a few header files only
as forwards for the time being. I've also had to keep the
api:libjingle_peerconnection target, with it only depending on the new
pc:libjingle_peerconnection target, to avoid breaking projects downstream.

[noahric, 2016-12-19 21:09:21]

On 2016/12/16 13:55:17, ossu wrote:
> Tested the change on chromium and internal projects and hit no errors.
> 
> Adding kjellander to review build related changes. 
> TL;DR: I'm moving the whole peerconnection implementation from api/ to pc/,
> leaving the public interfaces in api/. I've had to add a few header files only
> as forwards for the time being. I've also had to keep the
> api:libjingle_peerconnection target, with it only depending on the new
> pc:libjingle_peerconnection target, to avoid breaking projects downstream.

Added two comments, but the general comment is: if we're going to remove support
for cricket::* types for configuration, that requires a separate conversation.
In the short term, can you just put the Jsep* types back? I didn't catch if
there were others, but definitely the session description and candidates.

[noahric, 2016-12-19 21:09:36]

https://codereview.webrtc.org/2514883002/diff/90001/webrtc/api/jsepicecandida...
File webrtc/api/jsepicecandidate.h (left):

https://codereview.webrtc.org/2514883002/diff/90001/webrtc/api/jsepicecandida...
webrtc/api/jsepicecandidate.h:24: class JsepIceCandidate : public
IceCandidateInterface {
This one as well, unless it just moved somewhere else (or there's another way to
easily add candidates from a cricket::Candidate).

https://codereview.webrtc.org/2514883002/diff/90001/webrtc/api/jsepsessiondes...
File webrtc/api/jsepsessiondescription.h (left):

https://codereview.webrtc.org/2514883002/diff/90001/webrtc/api/jsepsessiondes...
webrtc/api/jsepsessiondescription.h:40: bool
Initialize(cricket::SessionDescription* description,
This is going to be problematic in the short term, unless there's another part
of the API that accepts cricket::SessionDescription objects.

Can you start an email thread with me, pthatcher, deadbeef, and tommi?

[kjellander_webrtc, 2016-12-20 07:17:06]

https://codereview.webrtc.org/2514883002/diff/90001/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/2514883002/diff/90001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:74: "test/fakeconstraints.h",
This wasn't here before and seems to be only included by tests +
webrtc/api/test/peerconnectiontestwrapper.h.

It used to be in the peerconnection_unittests target so why add in the api
target? I understand it makes sense keeping it in api as it's a fake, but I'm
wondering if we should have *_test_api targets for those or something similar. 
mock_audio_mixer is such an example:
https://cs.chromium.org/chromium/src/third_party/webrtc/api/BUILD.gn?rcl=0&l=325

https://codereview.webrtc.org/2514883002/diff/90001/webrtc/pc/DEPS
File webrtc/pc/DEPS (right):

https://codereview.webrtc.org/2514883002/diff/90001/webrtc/pc/DEPS#newcode13
webrtc/pc/DEPS:13: "+webrtc/modules/video_coding",
sort alphabetically.

https://codereview.webrtc.org/2514883002/diff/90001/webrtc/sdk/DEPS
File webrtc/sdk/DEPS (right):

https://codereview.webrtc.org/2514883002/diff/90001/webrtc/sdk/DEPS#newcode4
webrtc/sdk/DEPS:4: "+webrtc/pc",
sort alphabetically

https://codereview.webrtc.org/2514883002/diff/90001/webrtc/sdk/android/src/jn...
File webrtc/sdk/android/src/jni/DEPS (right):

https://codereview.webrtc.org/2514883002/diff/90001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/DEPS:2: "+webrtc/pc",
sort

[ossu, 2016-12-20 14:06:43]

On 2016/12/19 21:09:21, noahric wrote:
> Added two comments, but the general comment is: if we're going to remove
support
> for cricket::* types for configuration, that requires a separate conversation.
> In the short term, can you just put the Jsep* types back? I didn't catch if
> there were others, but definitely the session description and candidates.

This change doesn't strictly remove support for them, but moves their
implementation into pc/. It should be fine (from a practical standpoint) to
continue relying on them. I'd even say it's fine to include them from their old
(api/) locations, since the redirection files being there indicates there are
still things that need doing. It'd be great if this CL wouldn't need to wait for
what looks like some redesign to be done.

On 2016/12/19 21:09:36, noahric wrote:
>
https://codereview.webrtc.org/2514883002/diff/90001/webrtc/api/jsepsessiondes...
> File webrtc/api/jsepsessiondescription.h (left):
> 
>
https://codereview.webrtc.org/2514883002/diff/90001/webrtc/api/jsepsessiondes...
> webrtc/api/jsepsessiondescription.h:40: bool
> Initialize(cricket::SessionDescription* description,
> This is going to be problematic in the short term, unless there's another part
> of the API that accepts cricket::SessionDescription objects.
> 
> Can you start an email thread with me, pthatcher, deadbeef, and tommi?

Sure thing!

[ossu, 2016-12-20 16:22:25]

https://codereview.webrtc.org/2514883002/diff/90001/webrtc/api/jsepsessiondes...
File webrtc/api/jsepsessiondescription.h (left):

https://codereview.webrtc.org/2514883002/diff/90001/webrtc/api/jsepsessiondes...
webrtc/api/jsepsessiondescription.h:40: bool
Initialize(cricket::SessionDescription* description,
On 2016/12/19 21:09:36, noahric wrote:
> This is going to be problematic in the short term, unless there's another part
> of the API that accepts cricket::SessionDescription objects.
> 
> Can you start an email thread with me, pthatcher, deadbeef, and tommi?

I'll pick up the thread anyways, but looking at this now, it seems this just
relies on p2p/base/candidate.h and (transitively) p2p/base/p2pconstants.h, apart
from stuff in base/ that is. So I think it would be fine to keep these in api/
and move the requisite p2p/base/ interfaces into api/ as well. Leaving them
where they are will likely cause odd dependency loops (p2p <-> api).

[ossu, 2016-12-20 16:29:10]

https://codereview.webrtc.org/2514883002/diff/90001/webrtc/api/jsepsessiondes...
File webrtc/api/jsepsessiondescription.h (left):

https://codereview.webrtc.org/2514883002/diff/90001/webrtc/api/jsepsessiondes...
webrtc/api/jsepsessiondescription.h:40: bool
Initialize(cricket::SessionDescription* description,
On 2016/12/20 16:22:25, ossu wrote:
> On 2016/12/19 21:09:36, noahric wrote:
> > This is going to be problematic in the short term, unless there's another
part
> > of the API that accepts cricket::SessionDescription objects.
> > 
> > Can you start an email thread with me, pthatcher, deadbeef, and tommi?
> 
> I'll pick up the thread anyways, but looking at this now, it seems this just
> relies on p2p/base/candidate.h and (transitively) p2p/base/p2pconstants.h,
apart
> from stuff in base/ that is. So I think it would be fine to keep these in api/
> and move the requisite p2p/base/ interfaces into api/ as well. Leaving them
> where they are will likely cause odd dependency loops (p2p <-> api).

Actually, no, I'll just move the jsep headers back. They clearly belong here.
I'll see if I can retain the dependency on p2p/base for now, or if I have to
move some other files around. Preferably the first, and do the second in another
CL. Thanks for highlighting this!

[Taylor Brandstetter, 2016-12-20 19:19:10]

https://codereview.webrtc.org/2514883002/diff/90001/webrtc/api/jsepsessiondes...
File webrtc/api/jsepsessiondescription.h (left):

https://codereview.webrtc.org/2514883002/diff/90001/webrtc/api/jsepsessiondes...
webrtc/api/jsepsessiondescription.h:40: bool
Initialize(cricket::SessionDescription* description,
On 2016/12/20 16:29:09, ossu wrote:
> On 2016/12/20 16:22:25, ossu wrote:
> > On 2016/12/19 21:09:36, noahric wrote:
> > > This is going to be problematic in the short term, unless there's another
> part
> > > of the API that accepts cricket::SessionDescription objects.
> > > 
> > > Can you start an email thread with me, pthatcher, deadbeef, and tommi?
> > 
> > I'll pick up the thread anyways, but looking at this now, it seems this just
> > relies on p2p/base/candidate.h and (transitively) p2p/base/p2pconstants.h,
> apart
> > from stuff in base/ that is. So I think it would be fine to keep these in
api/
> > and move the requisite p2p/base/ interfaces into api/ as well. Leaving them
> > where they are will likely cause odd dependency loops (p2p <-> api).
> 
> Actually, no, I'll just move the jsep headers back. They clearly belong here.
> I'll see if I can retain the dependency on p2p/base for now, or if I have to
> move some other files around. Preferably the first, and do the second in
another
> CL. Thanks for highlighting this!

If this is a matter of "what API do we commit to supporting long-term", I think
the Jsep classes and what comes along with them should be left out. I wouldn't
want people writing new code that depends on cricket::SessionDescription.

Of course, nothing will break in the short term if you still use these classes
directly. But they shouldn't be part of the API.

Also: I think it's an oversight that "RemoveCandidates" uses
"cricket::Candidate" instead of "IceCandidateInterface". Not sure what to do
about that. Maybe it was added recently enough that it will be easy to change.

[noahric, 2016-12-20 22:06:01]

On 2016/12/20 19:19:10, Taylor Brandstetter wrote:
>
https://codereview.webrtc.org/2514883002/diff/90001/webrtc/api/jsepsessiondes...
> File webrtc/api/jsepsessiondescription.h (left):
> 
>
https://codereview.webrtc.org/2514883002/diff/90001/webrtc/api/jsepsessiondes...
> webrtc/api/jsepsessiondescription.h:40: bool
> Initialize(cricket::SessionDescription* description,
> On 2016/12/20 16:29:09, ossu wrote:
> > On 2016/12/20 16:22:25, ossu wrote:
> > > On 2016/12/19 21:09:36, noahric wrote:
> > > > This is going to be problematic in the short term, unless there's
another
> > part
> > > > of the API that accepts cricket::SessionDescription objects.
> > > > 
> > > > Can you start an email thread with me, pthatcher, deadbeef, and tommi?
> > > 
> > > I'll pick up the thread anyways, but looking at this now, it seems this
just
> > > relies on p2p/base/candidate.h and (transitively) p2p/base/p2pconstants.h,
> > apart
> > > from stuff in base/ that is. So I think it would be fine to keep these in
> api/
> > > and move the requisite p2p/base/ interfaces into api/ as well. Leaving
them
> > > where they are will likely cause odd dependency loops (p2p <-> api).
> > 
> > Actually, no, I'll just move the jsep headers back. They clearly belong
here.
> > I'll see if I can retain the dependency on p2p/base for now, or if I have to
> > move some other files around. Preferably the first, and do the second in
> another
> > CL. Thanks for highlighting this!
> 
> If this is a matter of "what API do we commit to supporting long-term", I
think
> the Jsep classes and what comes along with them should be left out. I wouldn't
> want people writing new code that depends on cricket::SessionDescription.
But you can't do that without changing SessionDescriptionInterface as well,
which exposes cricket::SessionDescription (for PeerConnection to consume, so PC
also transitively depends on cricket::SessionDescription).

Whether or not it's cricket::SessionDescription, I think you do want some type
of structured representation (ideally: a proto-like object, so
java/objc/swift/c++/whatever could all interact with it), with a WebRTC provided
conversion to/from SDP for clients (e.g. browsers or existing users) that use
it. That's roughly what you have today with JsepSessionDescription, and that's
the way that PeerConnection is implemented. It'd be kinda awkward to require SDP
outside of PeerConnection (even in C++), only to avoid it wherever possible
internally :)

> Of course, nothing will break in the short term if you still use these classes
> directly.

Practically speaking, I don't think that's necessarily true :) Once it moves to
a different directory with different support expectations, it will get broken
(temporarily at least). And then I'll get complaints asking me why my client is
using an unsupported interface.

Plus, there are other ramifications for the transitive header include; it may
violate strict layering/dependency management in some build systems to have this
directly include a header that isn't otherwise visible, depending on how that's
enforced (e.g. something like bazel would get cranky if it enforced header
visibility strictly).

In any event, an email thread here with the people I mentioned previously is the
right way forward.

> Also: I think it's an oversight that "RemoveCandidates" uses
> "cricket::Candidate" instead of "IceCandidateInterface". Not sure what to do
> about that. Maybe it was added recently enough that it will be easy to change.

[ossu, 2017-01-11 16:46:30]

Patchset #9 (id:110001) has been deleted

[ossu, 2017-01-11 17:27:55]

Patchset #10 (id:130001) has been deleted

[ossu, 2017-01-12 14:00:21]

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

[commit-bot: I haz the power, 2017-01-12 14:00:27]

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

[ossu, 2017-01-12 14:03:07]

I've moved the JSEP headers back into api/ but I've had to leave the .cc files
in pc/ to avoid compilation errors. It's not ideal but also not without
precedent: e.g. CreatePeerConnectionFactory() is declared in
api/peerconnectioninterface.h but implemented in pc/peerconnectionfactory.cc.
We'll have to look at untangling things like these going forward.

https://codereview.webrtc.org/2514883002/diff/90001/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/2514883002/diff/90001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:74: "test/fakeconstraints.h",
On 2016/12/20 07:17:06, kjellander_webrtc wrote:
> This wasn't here before and seems to be only included by tests +
> webrtc/api/test/peerconnectiontestwrapper.h.
> 
> It used to be in the peerconnection_unittests target so why add in the api
> target? I understand it makes sense keeping it in api as it's a fake, but I'm
> wondering if we should have *_test_api targets for those or something similar.

> mock_audio_mixer is such an example:
>
https://cs.chromium.org/chromium/src/third_party/webrtc/api/BUILD.gn?rcl=0&l=325

Yeah, putting testing-specific things into a separate target (i.e. fakes and
mocks) makes sense. I've created a libjingle_peerconnection_test_api target and
put this fake in there for now; or do you prefer separate target for each fake
or mock, such as the mock_audio_mixer target? Regardless of target, no
downstream project will properly depend on it to get at fakeconstraints.h but
AFAICT, they didn't before either, so we should be fine to move forward.

https://codereview.webrtc.org/2514883002/diff/90001/webrtc/pc/DEPS
File webrtc/pc/DEPS (right):

https://codereview.webrtc.org/2514883002/diff/90001/webrtc/pc/DEPS#newcode13
webrtc/pc/DEPS:13: "+webrtc/modules/video_coding",
On 2016/12/20 07:17:06, kjellander_webrtc wrote:
> sort alphabetically.

Done.

https://codereview.webrtc.org/2514883002/diff/90001/webrtc/sdk/DEPS
File webrtc/sdk/DEPS (right):

https://codereview.webrtc.org/2514883002/diff/90001/webrtc/sdk/DEPS#newcode4
webrtc/sdk/DEPS:4: "+webrtc/pc",
On 2016/12/20 07:17:06, kjellander_webrtc wrote:
> sort alphabetically

Done.

https://codereview.webrtc.org/2514883002/diff/90001/webrtc/sdk/android/src/jn...
File webrtc/sdk/android/src/jni/DEPS (right):

https://codereview.webrtc.org/2514883002/diff/90001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/DEPS:2: "+webrtc/pc",
On 2016/12/20 07:17:06, kjellander_webrtc wrote:
> sort

Done.

[commit-bot: I haz the power, 2017-01-12 14:05:37]

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

[commit-bot: I haz the power, 2017-01-12 14:05:39]

Dry run: Try jobs failed on following builders:
  mac_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/21674)

[ossu, 2017-01-12 15:22:48]

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

[commit-bot: I haz the power, 2017-01-12 15:22:57]

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, 2017-01-12 17:39:12]

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

[commit-bot: I haz the power, 2017-01-12 17:39:14]

Dry run: This issue passed the CQ dry run.

[ossu, 2017-01-17 15:03:14]

ossu@webrtc.org changed reviewers:
+ magjed@webrtc.org

[ossu, 2017-01-17 15:03:16]

kjellander@ could you have a look and possibly approve the build changes?

magjed@ could you do the same but for the changes in webrtc/sdk/android and
webrtc/sdk/objc?

[magjed_webrtc, 2017-01-18 10:22:49]

On 2017/01/17 15:03:16, ossu wrote:
> kjellander@ could you have a look and possibly approve the build changes?
> 
> magjed@ could you do the same but for the changes in webrtc/sdk/android and
> webrtc/sdk/objc?

webrtc/sdk/ changes lgtm!

[kjellander_webrtc, 2017-01-20 09:55:06]

Description was changed from

==========
Created a libjingle_peerconnection_api target and started separating
things into it.

Moved the RTCStatsCollectorCallback into its own header file, so that
PeerConnectionInterface can include that instead of pulling in
RTCStatsCollector and PeerConnection and everything.

Separated cricket::MediaType into its own header/source set, so that it
can be used in the api.

BUG=webrtc:5883
==========

to

==========
Create libjingle_peerconnection_api target

Create a new target in webrtc/api and moving things into it.
Move remaining parts of //webrtc/api:libjingle_peerconnection to
//webrtc/pc:libjingle_peerconnection.

Moved the RTCStatsCollectorCallback into its own header file, so that
PeerConnectionInterface can include that instead of pulling in
RTCStatsCollector and PeerConnection and everything.

Separated cricket::MediaType into its own header/source set, so that it
can be used in the api.

BUG=webrtc:5883
==========

[kjellander_webrtc, 2017-01-20 09:55:23]

Description was changed from

==========
Create libjingle_peerconnection_api target

Create a new target in webrtc/api and moving things into it.
Move remaining parts of //webrtc/api:libjingle_peerconnection to
//webrtc/pc:libjingle_peerconnection.

Moved the RTCStatsCollectorCallback into its own header file, so that
PeerConnectionInterface can include that instead of pulling in
RTCStatsCollector and PeerConnection and everything.

Separated cricket::MediaType into its own header/source set, so that it
can be used in the api.

BUG=webrtc:5883
==========

to

==========
Create libjingle_peerconnection_api target

Create a new target in webrtc/api and start moving things into it.
Move remaining parts of //webrtc/api:libjingle_peerconnection to
//webrtc/pc:libjingle_peerconnection.

Moved the RTCStatsCollectorCallback into its own header file, so that
PeerConnectionInterface can include that instead of pulling in
RTCStatsCollector and PeerConnection and everything.

Separated cricket::MediaType into its own header/source set, so that it
can be used in the api.

BUG=webrtc:5883
==========

[kjellander_webrtc, 2017-01-20 09:56:35]

Description was changed from

==========
Create libjingle_peerconnection_api target

Create a new target in webrtc/api and start moving things into it.
Move remaining parts of //webrtc/api:libjingle_peerconnection to
//webrtc/pc:libjingle_peerconnection.

Moved the RTCStatsCollectorCallback into its own header file, so that
PeerConnectionInterface can include that instead of pulling in
RTCStatsCollector and PeerConnection and everything.

Separated cricket::MediaType into its own header/source set, so that it
can be used in the api.

BUG=webrtc:5883
==========

to

==========
Create //webrtc/api:libjingle_peerconnection_api + refactorings.

Create a new target //webrtc/api:libjingle_peerconnection_api and start moving
things into it. Move remaining parts of //webrtc/api:libjingle_peerconnection
to //webrtc/pc:libjingle_peerconnection.

Moved the RTCStatsCollectorCallback into its own header file, so that
PeerConnectionInterface can include that instead of pulling in
RTCStatsCollector and PeerConnection and everything.

Separated cricket::MediaType into its own header/source set, so that it
can be used in the api.

BUG=webrtc:5883
==========

[kjellander_webrtc, 2017-01-20 10:10:53]

This is awesome work. lgtm if the internal trybot is happy.

I updated the beginning of the CL description + title slightly.

[ossu, 2017-01-20 13:00:53]

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

[commit-bot: I haz the power, 2017-01-20 13:01:02]

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, 2017-01-20 13:12:05]

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

[commit-bot: I haz the power, 2017-01-20 13:12:07]

Dry run: Try jobs failed on following builders:
  android_compile_x64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...)

[ossu, 2017-01-20 13:53:30]

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

[commit-bot: I haz the power, 2017-01-20 13:53:42]

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, 2017-01-20 14:51:06]

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

[commit-bot: I haz the power, 2017-01-20 14:51:08]

Dry run: This issue passed the CQ dry run.

[ossu, 2017-01-20 15:39:52]

Thanks!

deadbeef@: I've moved the OrtcFactory code into pc/. I suspect it should go into
its own ortc/ subdirectory or something, eventually, but this is what I could do
for now. Speak up if this is a problem.

I've also come across a problem spotted by the internal bot. Seems to be a
simple IWYU violation that came to light. I'll address this separately.

[Taylor Brandstetter, 2017-01-20 17:42:10]

On 2017/01/20 15:39:52, ossu wrote:
> Thanks!
> 
> deadbeef@: I've moved the OrtcFactory code into pc/. I suspect it should go
into
> its own ortc/ subdirectory or something, eventually, but this is what I could
do
> for now. Speak up if this is a problem.

Sounds good; we'll take care of adding an ortc subdirectory after things are
further along.

[ossu, 2017-01-23 09:48:29]

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

[commit-bot: I haz the power, 2017-01-23 09:48:35]

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, 2017-01-23 10:56:05]

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

[commit-bot: I haz the power, 2017-01-23 10:56:07]

Dry run: This issue passed the CQ dry run.

[ossu, 2017-01-23 12:52:27]

The CQ bit was checked by ossu@webrtc.org

[ossu, 2017-01-23 12:52:28]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org,
magjed@webrtc.org, kjellander@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2514883002/#ps200001 (title:
"Rebase")

[commit-bot: I haz the power, 2017-01-23 12:52:40]

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, 2017-01-23 12:56:27]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1485175947076050,
"parent_rev": "f49ff260d1843975f134416d694bd77d829e4201", "commit_rev":
"7bb87ee4e8b6620ebb647f2573d429b78b42440c"}

[commit-bot: I haz the power, 2017-01-23 12:56:31]

Description was changed from

==========
Create //webrtc/api:libjingle_peerconnection_api + refactorings.

Create a new target //webrtc/api:libjingle_peerconnection_api and start moving
things into it. Move remaining parts of //webrtc/api:libjingle_peerconnection
to //webrtc/pc:libjingle_peerconnection.

Moved the RTCStatsCollectorCallback into its own header file, so that
PeerConnectionInterface can include that instead of pulling in
RTCStatsCollector and PeerConnection and everything.

Separated cricket::MediaType into its own header/source set, so that it
can be used in the api.

BUG=webrtc:5883
==========

to

==========
Create //webrtc/api:libjingle_peerconnection_api + refactorings.

Create a new target //webrtc/api:libjingle_peerconnection_api and start moving
things into it. Move remaining parts of //webrtc/api:libjingle_peerconnection
to //webrtc/pc:libjingle_peerconnection.

Moved the RTCStatsCollectorCallback into its own header file, so that
PeerConnectionInterface can include that instead of pulling in
RTCStatsCollector and PeerConnection and everything.

Separated cricket::MediaType into its own header/source set, so that it
can be used in the api.

BUG=webrtc:5883

Review-Url: https://codereview.webrtc.org/2514883002
Cr-Commit-Position: refs/heads/master@{#16210}
Committed:
https://chromium.googlesource.com/external/webrtc/+/7bb87ee4e8b6620ebb647f257...
==========

[commit-bot: I haz the power, 2017-01-23 12:56:34]

Committed patchset #16 (id:200001) as
https://chromium.googlesource.com/external/webrtc/+/7bb87ee4e8b6620ebb647f257...