Move RtpExtension to api/ directory and config.h/.cc to call/.

Move RtpExtension to api/ directory and config.h/.cc to call/.

BUG=webrtc:5876
R=deadbeef@webrtc.org, solenberg@webrtc.org

Review-Url: https://codereview.webrtc.org/3004723002 .
Cr-Commit-Position: refs/heads/master@{#19639}
Committed: https://chromium.googlesource.com/external/webrtc/+/1acbd68718476b2754a7872fb72e3a8a74166eb9

[stefan-webrtc, 2017-08-28 14:15:36]

stefan@webrtc.org changed reviewers:
+ mbonadei@webrtc.org, solenberg@webrtc.org

[stefan-webrtc, 2017-08-28 14:17:17]

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

[commit-bot: I haz the power, 2017-08-28 14:17:19]

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-08-28 14:21:30]

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

[commit-bot: I haz the power, 2017-08-28 14:21:31]

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

[stefan-webrtc, 2017-08-28 14:36:05]

Fix build issues.

[stefan-webrtc, 2017-08-28 14:36:22]

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

[commit-bot: I haz the power, 2017-08-28 14:36:29]

Dry run: CQ is trying da patch.

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

[stefan-webrtc, 2017-08-28 14:38:42]

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

[commit-bot: I haz the power, 2017-08-28 14:38:49]

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-08-28 14:42:59]

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

[commit-bot: I haz the power, 2017-08-28 14:43:00]

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

[the sun, 2017-08-28 18:52:56]

Brilliant! Just one thing that concerns me, about leaving too much in
RtpExtension when moving it into api/

https://codereview.webrtc.org/3004723002/diff/40001/webrtc/BUILD.gn
File webrtc/BUILD.gn (right):

https://codereview.webrtc.org/3004723002/diff/40001/webrtc/BUILD.gn#newcode383
webrtc/BUILD.gn:383: rtc_test("rtc_unittests") {
Is this target not needed anymore?

https://codereview.webrtc.org/3004723002/diff/40001/webrtc/api/rtpparameters.h
File webrtc/api/rtpparameters.h (right):

https://codereview.webrtc.org/3004723002/diff/40001/webrtc/api/rtpparameters....
webrtc/api/rtpparameters.h:225: static bool IsSupportedForAudio(const
std::string& uri);
Is it necessary to support these utility functions for clients? The API should
be kept as small as possible. By putting a method or class in webrtc/api/ we are
promising certain functionality.

https://codereview.webrtc.org/3004723002/diff/40001/webrtc/call/config.h
File webrtc/call/config.h (right):

https://codereview.webrtc.org/3004723002/diff/40001/webrtc/call/config.h#newc...
webrtc/call/config.h:11: // TODO(pbos): Move Config from common.h to here.
obsolete comment

https://codereview.webrtc.org/3004723002/diff/40001/webrtc/call/config.h#newc...
webrtc/call/config.h:12: 
Add another comment here, or file an issue, that the contents of this file
should be split out.

[mbonadei, 2017-08-29 06:45:29]

https://codereview.webrtc.org/3004723002/diff/40001/webrtc/BUILD.gn
File webrtc/BUILD.gn (right):

https://codereview.webrtc.org/3004723002/diff/40001/webrtc/BUILD.gn#newcode383
webrtc/BUILD.gn:383: rtc_test("rtc_unittests") {
On 2017/08/28 18:52:56, the sun wrote:
> Is this target not needed anymore?

I think it is used as an "umbrella target" to link all the tests in the deps
list into a single executable.

But (not related to this CL) I see that there is also webrtc_tests and it
contains some unit tests, so maybe we can merge them together or keep unit tests
separated from webrtc_tests.

I will follow up on this.

[mbonadei, 2017-08-29 06:49:44]

Trybots are just complaining because some dependencies are not declared
in GN.

If you run "gn check out/Debug" it should be easy to spot them before
uploading the patch set.

You can also use:

$ python tools_webrtc/gn_check_autofix.py -m tryserver.webrtc \
  -b <BOT_NAME(e.g.: linux_libfuzzer_rel)>

to try to auto-fix them.

[stefan-webrtc, 2017-08-29 07:34:42]

Fix build dependencies.

[stefan-webrtc, 2017-08-29 07:38:22]

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

[commit-bot: I haz the power, 2017-08-29 07:38:31]

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-08-29 07:43:20]

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

[commit-bot: I haz the power, 2017-08-29 07:43:21]

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

[stefan-webrtc, 2017-08-29 08:36:32]

Rename config.h->video_config.h

[stefan-webrtc, 2017-08-29 08:37:07]

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

[commit-bot: I haz the power, 2017-08-29 08:37:10]

Dry run: CQ is trying da patch.

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

[stefan-webrtc, 2017-08-29 08:37:18]

https://codereview.webrtc.org/3004723002/diff/40001/webrtc/BUILD.gn
File webrtc/BUILD.gn (right):

https://codereview.webrtc.org/3004723002/diff/40001/webrtc/BUILD.gn#newcode383
webrtc/BUILD.gn:383: rtc_test("rtc_unittests") {
On 2017/08/29 06:45:29, mbonadei wrote:
> On 2017/08/28 18:52:56, the sun wrote:
> > Is this target not needed anymore?
> 
> I think it is used as an "umbrella target" to link all the tests in the deps
> list into a single executable.
> 
> But (not related to this CL) I see that there is also webrtc_tests and it
> contains some unit tests, so maybe we can merge them together or keep unit
tests
> separated from webrtc_tests.
> 
> I will follow up on this.

I'll keep it for now then.

https://codereview.webrtc.org/3004723002/diff/40001/webrtc/api/rtpparameters.h
File webrtc/api/rtpparameters.h (right):

https://codereview.webrtc.org/3004723002/diff/40001/webrtc/api/rtpparameters....
webrtc/api/rtpparameters.h:225: static bool IsSupportedForAudio(const
std::string& uri);
On 2017/08/28 18:52:56, the sun wrote:
> Is it necessary to support these utility functions for clients? The API should
> be kept as small as possible. By putting a method or class in webrtc/api/ we
are
> promising certain functionality.

It could probably be useful for a user of webrtc to be able to determine what
extensions are supported. We could of course move it somewhere else too, but I
don't see any obvious place right now as they're used in many different files.

https://codereview.webrtc.org/3004723002/diff/40001/webrtc/call/config.h
File webrtc/call/config.h (right):

https://codereview.webrtc.org/3004723002/diff/40001/webrtc/call/config.h#newc...
webrtc/call/config.h:11: // TODO(pbos): Move Config from common.h to here.
On 2017/08/28 18:52:56, the sun wrote:
> obsolete comment

I don't know if it's obsolete to be honest, not clear what Config it refers to.
:)

I'll delete it since it's hard to understand anyway.

https://codereview.webrtc.org/3004723002/diff/40001/webrtc/call/config.h#newc...
webrtc/call/config.h:12: 
On 2017/08/28 18:52:56, the sun wrote:
> Add another comment here, or file an issue, that the contents of this file
> should be split out.

I had to split them already because of dependency issues.

[commit-bot: I haz the power, 2017-08-29 09:02:01]

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

[commit-bot: I haz the power, 2017-08-29 09:02:02]

Dry run: This issue passed the CQ dry run.

[stefan-webrtc, 2017-08-30 07:28:48]

May I request another look at this? Would be good to get it in to avoid merge
conflicts. :)

[mbonadei, 2017-08-30 08:17:38]

On 2017/08/30 07:28:48, stefan-webrtc wrote:
> May I request another look at this? Would be good to get it in to avoid merge
> conflicts. :)

It is good from my point of view. I am only worried about backward
compatibility.
Some downstream projects are including webrtc/config.h and they will break.

How can we avoid this? Maybe keeping a stub webrtc/config.h header around with:
#include "webrtc/api/rtpparameters.h"
#include "webrtc/call/rtp_config.h"

So we can update all the downstream projects and remove webrtc/config.h in
another
CL. What do you think?

[stefan-webrtc, 2017-08-30 15:28:35]

On 2017/08/30 08:17:38, mbonadei wrote:
> On 2017/08/30 07:28:48, stefan-webrtc wrote:
> > May I request another look at this? Would be good to get it in to avoid
merge
> > conflicts. :)
> 
> It is good from my point of view. I am only worried about backward
> compatibility.
> Some downstream projects are including webrtc/config.h and they will break.
> 
> How can we avoid this? Maybe keeping a stub webrtc/config.h header around
with:
> #include "webrtc/api/rtpparameters.h"
> #include "webrtc/call/rtp_config.h"
> 
> So we can update all the downstream projects and remove webrtc/config.h in
> another
> CL. What do you think?

Sounds good to me. I'll make that change.

[stefan-webrtc, 2017-08-30 15:40:52]

Actually add file.

[stefan-webrtc, 2017-08-30 15:45:42]

Rebase

[the sun, 2017-08-31 06:51:46]

On 2017/08/30 15:28:35, stefan-webrtc wrote:
> On 2017/08/30 08:17:38, mbonadei wrote:
> > On 2017/08/30 07:28:48, stefan-webrtc wrote:
> > > May I request another look at this? Would be good to get it in to avoid
> merge
> > > conflicts. :)
> > 
> > It is good from my point of view. I am only worried about backward
> > compatibility.
> > Some downstream projects are including webrtc/config.h and they will break.
> > 
> > How can we avoid this? Maybe keeping a stub webrtc/config.h header around
> with:
> > #include "webrtc/api/rtpparameters.h"
> > #include "webrtc/call/rtp_config.h"
> > 
> > So we can update all the downstream projects and remove webrtc/config.h in
> > another
> > CL. What do you think?
> 
> Sounds good to me. I'll make that change.

sg - don't forget to PSA and remove this stub in 1-2 weeks

[stefan-webrtc, 2017-08-31 06:54:44]

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

[commit-bot: I haz the power, 2017-08-31 06:54:54]

Dry run: CQ is trying da patch.

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

[the sun, 2017-08-31 07:08:05]

LG except there are a lot of deps on call/:call added which I'm not fully
convinced they are needed.

https://codereview.webrtc.org/3004723002/diff/140001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/3004723002/diff/140001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/BUILD.gn:1392: "../../call:call",
why? I don't see any direct use of things defined in call/ or the former
config.h

https://codereview.webrtc.org/3004723002/diff/140001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/BUILD.gn:1714:
"../../api:libjingle_peerconnection_api",
...uhm, really?

[commit-bot: I haz the power, 2017-08-31 07:28:44]

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

[commit-bot: I haz the power, 2017-08-31 07:28:45]

Dry run: This issue passed the CQ dry run.

[stefan-webrtc, 2017-08-31 13:51:08]

Rebase

[stefan-webrtc, 2017-08-31 14:12:41]

Removed a lot of call dependencies.

[stefan-webrtc, 2017-08-31 14:13:09]

Removed all call dependencies that I had previously added, since it was an
artifact of some earlier patch-set. Thanks for catching that.

https://codereview.webrtc.org/3004723002/diff/140001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/3004723002/diff/140001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/BUILD.gn:1392: "../../call:call",
On 2017/08/31 07:08:04, the sun wrote:
> why? I don't see any direct use of things defined in call/ or the former
> config.h

Thanks for catching.

https://codereview.webrtc.org/3004723002/diff/140001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/BUILD.gn:1714:
"../../api:libjingle_peerconnection_api",
On 2017/08/31 07:08:04, the sun wrote:
> ...uhm, really?

I thought it was, but seems like it isn't any longer. :) Probably a mistake from
my side.

[stefan-webrtc, 2017-08-31 14:13:22]

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

[commit-bot: I haz the power, 2017-08-31 14:13:34]

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-08-31 14:39:45]

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

[commit-bot: I haz the power, 2017-08-31 14:39:47]

Dry run: This issue passed the CQ dry run.

[stefan-webrtc, 2017-08-31 17:53:13]

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

[stefan-webrtc, 2017-08-31 17:53:14]

+deadbeef who also may want to comment on some of these changes.

[Taylor Brandstetter, 2017-08-31 19:42:35]

lgtm

https://codereview.webrtc.org/3004723002/diff/180001/webrtc/api/rtpparameters.h
File webrtc/api/rtpparameters.h (right):

https://codereview.webrtc.org/3004723002/diff/180001/webrtc/api/rtpparameters...
webrtc/api/rtpparameters.h:201: RtpHeaderExtensionCapability();
Since these are all POD structs, doesn't the chromium style guide say it's ok to
have inlined constructors and no explicit destructor? I'm fine with this change,
I'm just wondering whether it's recommended by a style guide or is just a
personal preference.

[the sun, 2017-09-01 11:15:03]

lgtm

https://codereview.webrtc.org/3004723002/diff/180001/webrtc/DEPS
File webrtc/DEPS (right):

https://codereview.webrtc.org/3004723002/diff/180001/webrtc/DEPS#newcode13
webrtc/DEPS:13: "+webrtc/call/rtp_config.h",
nit: order

[stefan-webrtc, 2017-09-01 12:26:50]

https://codereview.webrtc.org/3004723002/diff/180001/webrtc/DEPS
File webrtc/DEPS (right):

https://codereview.webrtc.org/3004723002/diff/180001/webrtc/DEPS#newcode13
webrtc/DEPS:13: "+webrtc/call/rtp_config.h",
On 2017/09/01 11:15:03, the sun wrote:
> nit: order

Done.

https://codereview.webrtc.org/3004723002/diff/180001/webrtc/api/rtpparameters.h
File webrtc/api/rtpparameters.h (right):

https://codereview.webrtc.org/3004723002/diff/180001/webrtc/api/rtpparameters...
webrtc/api/rtpparameters.h:201: RtpHeaderExtensionCapability();
On 2017/08/31 19:42:35, Taylor Brandstetter wrote:
> Since these are all POD structs, doesn't the chromium style guide say it's ok
to
> have inlined constructors and no explicit destructor? I'm fine with this
change,
> I'm just wondering whether it's recommended by a style guide or is just a
> personal preference.

TBH, I did this since that's the only way I got it to build. For instance, if I
remove ~RtpRtxParameters(), the chromium style complains:
../../webrtc/api/rtpparameters.h:315:1: error: [chromium-style] Complex
class/struct needs an explicit out-of-line destructor.

Probably RtpRtxParameters isn't considered POD because of the use of Optional.
Why this wasn't a problem before I don't know. Maybe one of my changes somehow
has enabled these chromium style checks, can't tell how though?

[stefan-webrtc, 2017-09-01 12:30:55]

Fix nit.

[stefan-webrtc, 2017-09-01 12:31:13]

The CQ bit was checked by stefan@webrtc.org

[stefan-webrtc, 2017-09-01 12:31:14]

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

[commit-bot: I haz the power, 2017-09-01 12:31:26]

CQ is trying da patch.

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

[kwiberg-webrtc, 2017-09-01 12:37:51]

https://codereview.webrtc.org/3004723002/diff/180001/webrtc/api/rtpparameters.h
File webrtc/api/rtpparameters.h (right):

https://codereview.webrtc.org/3004723002/diff/180001/webrtc/api/rtpparameters...
webrtc/api/rtpparameters.h:201: RtpHeaderExtensionCapability();
On 2017/09/01 12:26:50, stefan-webrtc wrote:
> On 2017/08/31 19:42:35, Taylor Brandstetter wrote:
> > Since these are all POD structs, doesn't the chromium style guide say it's
ok
> to
> > have inlined constructors and no explicit destructor? I'm fine with this
> change,
> > I'm just wondering whether it's recommended by a style guide or is just a
> > personal preference.
> 
> TBH, I did this since that's the only way I got it to build. For instance, if
I
> remove ~RtpRtxParameters(), the chromium style complains:
> ../../webrtc/api/rtpparameters.h:315:1: error: [chromium-style] Complex
> class/struct needs an explicit out-of-line destructor.
> 
> Probably RtpRtxParameters isn't considered POD because of the use of Optional.

Yes, Optional<T> isn't a trivial type even for trivial Ts, since it isn't
trivially default constructible.

> Why this wasn't a problem before I don't know. Maybe one of my changes somehow
> has enabled these chromium style checks, can't tell how though?

A large number of build targets explicitly disable the Chromium style checks
because we haven't gotten around to cleaning them up yet. This sort of thing
happens when you move code from a build target where the checks are disabled to
a build target where they're not.

[stefan-webrtc, 2017-09-01 12:41:29]

https://codereview.webrtc.org/3004723002/diff/180001/webrtc/api/rtpparameters.h
File webrtc/api/rtpparameters.h (right):

https://codereview.webrtc.org/3004723002/diff/180001/webrtc/api/rtpparameters...
webrtc/api/rtpparameters.h:201: RtpHeaderExtensionCapability();
On 2017/09/01 12:37:51, kwiberg-webrtc wrote:
> On 2017/09/01 12:26:50, stefan-webrtc wrote:
> > On 2017/08/31 19:42:35, Taylor Brandstetter wrote:
> > > Since these are all POD structs, doesn't the chromium style guide say it's
> ok
> > to
> > > have inlined constructors and no explicit destructor? I'm fine with this
> > change,
> > > I'm just wondering whether it's recommended by a style guide or is just a
> > > personal preference.
> > 
> > TBH, I did this since that's the only way I got it to build. For instance,
if
> I
> > remove ~RtpRtxParameters(), the chromium style complains:
> > ../../webrtc/api/rtpparameters.h:315:1: error: [chromium-style] Complex
> > class/struct needs an explicit out-of-line destructor.
> > 
> > Probably RtpRtxParameters isn't considered POD because of the use of
Optional.
> 
> Yes, Optional<T> isn't a trivial type even for trivial Ts, since it isn't
> trivially default constructible.
> 
> > Why this wasn't a problem before I don't know. Maybe one of my changes
somehow
> > has enabled these chromium style checks, can't tell how though?
> 
> A large number of build targets explicitly disable the Chromium style checks
> because we haven't gotten around to cleaning them up yet. This sort of thing
> happens when you move code from a build target where the checks are disabled
to
> a build target where they're not.

Yes, but what I don't understand is how I enabled the checks for existing code
in this file. :)

[commit-bot: I haz the power, 2017-09-01 12:59:55]

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

[commit-bot: I haz the power, 2017-09-01 12:59:57]

Try jobs failed on following builders:
  linux_msan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/23507)

[stefan-webrtc, 2017-09-01 13:29:38]

Description was changed from

==========
Move RtpExtension to api/ directory and config.h/.cc to call/.

BUG=webrtc:5876
==========

to

==========
Move RtpExtension to api/ directory and config.h/.cc to call/.

BUG=webrtc:5876
R=deadbeef@webrtc.org, solenberg@webrtc.org

Review-Url: https://codereview.webrtc.org/3004723002 .
Cr-Commit-Position: refs/heads/master@{#19639}
Committed:
https://chromium.googlesource.com/external/webrtc/+/1acbd68718476b2754a7872fb...
==========

[stefan-webrtc, 2017-09-01 13:29:41]

Committed patchset #11 (id:200001) manually as
1acbd68718476b2754a7872fb72e3a8a74166eb9 (presubmit successful).