Merge RTCConfiguration into WebRTCConfiguration

Merge RTCConfiguration into WebRTCConfiguration

This makes WebRTCConfiguration into a data struct instead of wrapping a
RTCConfiguration. The motivation for this is to introduce
RTCConfiguration as an IDL dictionary, at which point the extra layer
starts to look unnecessary.

BUG=649343
Committed: https://crrev.com/15d04a3473cb60cba6ceb7205353df67642126f7
Cr-Commit-Position: refs/heads/master@{#426836}

[foolip, 2016-10-13 13:04:57]

The CQ bit was checked by foolip@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 13:05:08]

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

[foolip, 2016-10-13 13:05:53]

foolip@chromium.org changed reviewers:
+ guidou@chromium.org, hbos@chromium.org, tommi@chromium.org

[foolip, 2016-10-13 13:10:21]

foolip@chromium.org changed reviewers:
+ jochen@chromium.org

[foolip, 2016-10-13 13:10:21]

https://codereview.chromium.org/2415673003/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebRTCConfiguration.h (right):

https://codereview.chromium.org/2415673003/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebRTCConfiguration.h:60:
std::vector<std::unique_ptr<WebRTCCertificate>> certificates;
jochen@, is this verboten? The basic trouble here is that a WebVector<T> can
only be created from an existing WebVector<T> or something with begin() and
end(), like Vector<T>.

One could add a move assignment operator to WebVector that merely assigns to the
m_data std::vector<T> within, but then it would be necessary to create a
temporary std::vector in the code creating a WebRTCConfiguration, and then isn't
that frowned upon?

[commit-bot: I haz the power, 2016-10-13 13:13:58]

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

[commit-bot: I haz the power, 2016-10-13 13:13:59]

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

[Guido Urdaneta, 2016-10-13 13:25:48]

lgtm % nit, but it looks you need to update a unit test to include the new
struct.

https://codereview.chromium.org/2415673003/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebRTCConfiguration.h (right):

https://codereview.chromium.org/2415673003/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebRTCConfiguration.h:34: #include
"WebCommon.h"
nit:
#include <vector>
#include <memory>
for std::vector and std::unique_ptr?

[haraken, 2016-10-13 13:27:28]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2016-10-13 13:27:28]

https://codereview.chromium.org/2415673003/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebRTCConfiguration.h (right):

https://codereview.chromium.org/2415673003/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebRTCConfiguration.h:60:
std::vector<std::unique_ptr<WebRTCCertificate>> certificates;
On 2016/10/13 13:10:21, foolip wrote:
> jochen@, is this verboten? The basic trouble here is that a WebVector<T> can
> only be created from an existing WebVector<T> or something with begin() and
> end(), like Vector<T>.

Drive-by: Would you elaborate on the problem? Can you just create an empty
WebVector and append elements later?

> 
> One could add a move assignment operator to WebVector that merely assigns to
the
> m_data std::vector<T> within, but then it would be necessary to create a
> temporary std::vector in the code creating a WebRTCConfiguration, and then
isn't
> that frowned upon?

[foolip, 2016-10-13 13:43:33]

https://codereview.chromium.org/2415673003/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebRTCConfiguration.h (right):

https://codereview.chromium.org/2415673003/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebRTCConfiguration.h:60:
std::vector<std::unique_ptr<WebRTCCertificate>> certificates;
On 2016/10/13 13:27:28, haraken wrote:
> On 2016/10/13 13:10:21, foolip wrote:
> > jochen@, is this verboten? The basic trouble here is that a WebVector<T> can
> > only be created from an existing WebVector<T> or something with begin() and
> > end(), like Vector<T>.
> 
> Drive-by: Would you elaborate on the problem? Can you just create an empty
> WebVector and append elements later?

WebVector<T>::append doesn't exist, but I'm not sure if it's intentional or not.
The only way to update it is using `vector = something` and
`vector.assign(something)` but neither forms work with move-only types.

[foolip, 2016-10-13 13:56:41]

The CQ bit was checked by foolip@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 13:56:55]

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

[haraken, 2016-10-13 14:04:01]

haraken@chromium.org changed reviewers:
+ esprehn@chromium.org

[haraken, 2016-10-13 14:04:02]

On 2016/10/13 13:43:33, foolip wrote:
>
https://codereview.chromium.org/2415673003/diff/1/third_party/WebKit/public/p...
> File third_party/WebKit/public/platform/WebRTCConfiguration.h (right):
> 
>
https://codereview.chromium.org/2415673003/diff/1/third_party/WebKit/public/p...
> third_party/WebKit/public/platform/WebRTCConfiguration.h:60:
> std::vector<std::unique_ptr<WebRTCCertificate>> certificates;
> On 2016/10/13 13:27:28, haraken wrote:
> > On 2016/10/13 13:10:21, foolip wrote:
> > > jochen@, is this verboten? The basic trouble here is that a WebVector<T>
can
> > > only be created from an existing WebVector<T> or something with begin()
and
> > > end(), like Vector<T>.
> > 
> > Drive-by: Would you elaborate on the problem? Can you just create an empty
> > WebVector and append elements later?
> 
> WebVector<T>::append doesn't exist, but I'm not sure if it's intentional or
not.
> The only way to update it is using `vector = something` and
> `vector.assign(something)` but neither forms work with move-only types.

Hmm, I don't see any issue in supporting WebVector<T>::append.

esprehn@?

[commit-bot: I haz the power, 2016-10-13 14:56:23]

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

[commit-bot: I haz the power, 2016-10-13 14:56:25]

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

[hbos_chromium, 2016-10-17 21:24:54]

Why are tests failing?

https://codereview.chromium.org/2415673003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/2415673003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:366:
certificate->certificateShallowCopy());
I think WebVector should have the ability to append elements, and to be able to
move from a vector instead of copying it. Not only is copying unnecessary in
many cases, but it is incompatible with std::unique_ptr.

You can still make this work with WebVector<std::unique_ptr<RTCCertificate>> but
it's ugly. Create a WebVector of size numberOfCertificates (so it's filled with
null unique_ptrs) and then do rtcConfiguration.certificates[i] =
certificate->certificateShallowCopy();

[esprehn, 2016-10-17 21:27:31]

WebVector wasn't really supposed to be resizable, but I guess we could add
append(). We should add the ability to std::move() into a std::vector though.
How would you handle a static array here?

[hbos_chromium, 2016-10-17 21:30:24]

Does RTCConfiguration, once made into an IDL dictionary, and WebRTCConfiguration
have to be different structs due to layers or can the IDL-generated dictionary
be platform-exported?

[foolip, 2016-10-21 12:05:46]

Tests were failing because WebRTCConfiguration::iceTransports defaulted to
WebRTCIceTransports::None, should have been ::All.

https://codereview.chromium.org/2415673003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/2415673003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:366:
certificate->certificateShallowCopy());
On 2016/10/17 21:24:54, hbos_chromium wrote:
> I think WebVector should have the ability to append elements, and to be able
to
> move from a vector instead of copying it. Not only is copying unnecessary in
> many cases, but it is incompatible with std::unique_ptr.
> 
> You can still make this work with WebVector<std::unique_ptr<RTCCertificate>>
but
> it's ugly. Create a WebVector of size numberOfCertificates (so it's filled
with
> null unique_ptrs) and then do rtcConfiguration.certificates[i] =
> certificate->certificateShallowCopy();

That actually turned out not too ugly, so I'm going to go with that!

[foolip, 2016-10-21 12:09:56]

The CQ bit was checked by foolip@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 12:10:16]

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

[foolip, 2016-10-21 13:09:13]

On 2016/10/17 21:30:24, hbos_chromium wrote:
> Does RTCConfiguration, once made into an IDL dictionary, and
WebRTCConfiguration
> have to be different structs due to layers or can the IDL-generated dictionary
> be platform-exported?

The generated code will be at modules/peerconnection/RTCConfiguration.h, so it
can't be exposed directly. But that's OK, because there will still be a need to
massage it a bit to turn it into a WebRTCConfiguration, for example the ice
servers are flattened into a list where each entry has just one URL.

[foolip, 2016-10-21 13:30:22]

On 2016/10/17 21:27:31, esprehn wrote:
> WebVector wasn't really supposed to be resizable, but I guess we could add
> append(). We should add the ability to std::move() into a std::vector though.
> How would you handle a static array here?

The workaround wasn't terrible, so I'm going with that. Do you mean moving a
WebVector<T>&& into a std::vector<T>? Is it even possible to create a WebVector
from a static array?

[Guido Urdaneta, 2016-10-21 13:36:46]

lgtm % nit

https://codereview.chromium.org/2415673003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/2415673003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:370:
rtcConfiguration.certificates.swap(certificatesCopy);
nit: I think
   rtcConfiguration.certificates = std::move(certificatesCopy) 
would look easier to read.

[commit-bot: I haz the power, 2016-10-21 13:43:52]

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

[commit-bot: I haz the power, 2016-10-21 13:43:53]

Dry run: This issue passed the CQ dry run.

[foolip, 2016-10-21 14:01:00]

https://codereview.chromium.org/2415673003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/2415673003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:370:
rtcConfiguration.certificates.swap(certificatesCopy);
On 2016/10/21 13:36:46, Guido Urdaneta wrote:
> nit: I think
>    rtcConfiguration.certificates = std::move(certificatesCopy) 
> would look easier to read.

Done.

[foolip, 2016-10-21 14:01:32]

The CQ bit was checked by foolip@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 14:01:46]

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

[jochen (gone - plz use gerrit), 2016-10-21 14:55:17]

lgtm with comments

https://codereview.chromium.org/2415673003/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebRTCConfiguration.h (right):

https://codereview.chromium.org/2415673003/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebRTCConfiguration.h:39: #include <vector>
do you still use this?

https://codereview.chromium.org/2415673003/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebRTCConfiguration.h:51: enum class
WebRTCIceTransports { None, Relay, All };
 These should be kNone, kRelay, kAll (same below)

https://codereview.chromium.org/2415673003/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebRTCConfiguration.h:62:
WebVector<std::unique_ptr<WebRTCCertificate>> certificates;
now that you use WebVector, please keep including the header

[commit-bot: I haz the power, 2016-10-21 15:33:10]

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

[commit-bot: I haz the power, 2016-10-21 15:33:11]

Dry run: This issue passed the CQ dry run.

[foolip, 2016-10-21 15:49:57]

https://codereview.chromium.org/2415673003/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebRTCConfiguration.h (right):

https://codereview.chromium.org/2415673003/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebRTCConfiguration.h:39: #include <vector>
On 2016/10/21 14:55:17, jochen wrote:
> do you still use this?

Oops, I'll undo some changes from PS2.

https://codereview.chromium.org/2415673003/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebRTCConfiguration.h:51: enum class
WebRTCIceTransports { None, Relay, All };
On 2016/10/21 14:55:17, jochen wrote:
>  These should be kNone, kRelay, kAll (same below)

Done.

https://codereview.chromium.org/2415673003/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebRTCConfiguration.h:62:
WebVector<std::unique_ptr<WebRTCCertificate>> certificates;
On 2016/10/21 14:55:17, jochen wrote:
> now that you use WebVector, please keep including the header

Done.

[foolip, 2016-10-21 15:51:24]

The CQ bit was checked by foolip@chromium.org

[foolip, 2016-10-21 15:51:25]

The patchset sent to the CQ was uploaded after l-g-t-m from guidou@chromium.org,
jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2415673003/#ps80001
(title: "headers, kConstants")

[commit-bot: I haz the power, 2016-10-21 15:51:48]

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

[hbos_chromium, 2016-10-21 17:27:28]

lgtm

[commit-bot: I haz the power, 2016-10-21 17:47:36]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-21 18:06:48]

Description was changed from

==========
Merge RTCConfiguration into WebRTCConfiguration

This makes WebRTCConfiguration into a data struct instead of wrapping a
RTCConfiguration. The motivation for this is to introduce
RTCConfiguration as an IDL dictionary, at which point the extra layer
starts to look unnecessary.

BUG=649343
==========

to

==========
Merge RTCConfiguration into WebRTCConfiguration

This makes WebRTCConfiguration into a data struct instead of wrapping a
RTCConfiguration. The motivation for this is to introduce
RTCConfiguration as an IDL dictionary, at which point the extra layer
starts to look unnecessary.

BUG=649343

Committed: https://crrev.com/15d04a3473cb60cba6ceb7205353df67642126f7
Cr-Commit-Position: refs/heads/master@{#426836}
==========

[commit-bot: I haz the power, 2016-10-21 18:06:53]

Patchset 5 (id:??) landed as
https://crrev.com/15d04a3473cb60cba6ceb7205353df67642126f7
Cr-Commit-Position: refs/heads/master@{#426836}