Add signaling to support ICE renomination.

Add signaling to support ICE renomination.
By default, this will tell the remote side that I am supporting ICE renomination.
It does not use ICE renomination yet even if the remote side supports it.

R=deadbeef@webrtc.org, pthatcher@webrtc.org, skvlad@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/4cedf2b78cefba056ca423fa101bdb2e8cfed79e

[honghaiz3, 2016-08-05 20:55:03]

Patchset #1 (id:1) has been deleted

[honghaiz3, 2016-08-05 20:55:12]

Patchset #1 (id:20001) has been deleted

[honghaiz3, 2016-08-05 20:55:20]

Patchset #1 (id:40001) has been deleted

[honghaiz3, 2016-08-05 20:57:12]

Description was changed from

==========
Add signaling to support ICE renomination.

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

to

==========
Add signaling to support ICE renomination.
By default, this will tell the remote side that I am supporting ICE
renomination. 
It does not use ICE renomination yet even if the remote side supports it. It may
be safer to to do it in a separate CL. 

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

[honghaiz3, 2016-08-05 20:57:27]

Description was changed from

==========
Add signaling to support ICE renomination.
By default, this will tell the remote side that I am supporting ICE
renomination. 
It does not use ICE renomination yet even if the remote side supports it. It may
be safer to to do it in a separate CL. 

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

to

==========
Add signaling to support ICE renomination.
By default, this will tell the remote side that I am supporting ICE
renomination. 
It does not use ICE renomination yet even if the remote side supports it. It
will be safer to to do it in a separate CL. 

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

[honghaiz3, 2016-08-05 20:57:51]

Description was changed from

==========
Add signaling to support ICE renomination.
By default, this will tell the remote side that I am supporting ICE
renomination. 
It does not use ICE renomination yet even if the remote side supports it. It
will be safer to to do it in a separate CL. 

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

to

==========
Add signaling to support ICE renomination.
By default, this will tell the remote side that I am supporting ICE
renomination. 
It does not use ICE renomination yet even if the remote side supports it. It
will be safer to to do that in a separate CL. 

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

[honghaiz3, 2016-08-05 20:58:32]

honghaiz@webrtc.org changed reviewers:
+ deadbeef@webrtc.org, pthatcher@webrtc.org

[pthatcher1, 2016-08-08 21:56:23]

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.h (right):

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.h:204: return true;
Can the unit tests just use SetRemoteIceParameters?

IceParameters remote_params = channel.remote_ice_parameters();
remote_params.renomination = remote_supports_renomination;
channel.SetRemoteIceParameters(remote_params);

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/transport...
File webrtc/p2p/base/transportchannelimpl.h (right):

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportchannelimpl.h:60: // in the ice_params must be set
before candidate gathering can start.
Non-ICE transport channels can't ignore them if they contain an inner ICE
transport channel.  Then they have to pass them down to the ICE transport

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/transport...
File webrtc/p2p/base/transportdescriptionfactory.cc (right):

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportdescriptionfactory.cc:42: }
I don't see any unit test coverage for this.

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/transport...
File webrtc/p2p/base/transportdescriptionfactory.h (right):

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportdescriptionfactory.h:27: bool ice_renomination = true;
I think we should make this false like  use_renomination_if_remote_supports, and
make both true if the application opts into it (and later make the default of
both true, and not allow opt-out).

[Taylor Brandstetter, 2016-08-08 22:28:17]

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/dtlstrans...
File webrtc/p2p/base/dtlstransportchannel.h (right):

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportchannel.h:168:
channel_->SetIceParameters(ice_params);
I think the introduction of IceParameters is a great idea, since it matches an
ORTC struct:
http://ortc.org/wp-content/uploads/2016/03/ortc.html#idl-def-RTCIceParameters

But along those lines, it would make sense to put the ICE mode in this struct as
well. If not in this CL, could you add a TODO to do that?

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:311: if (!current_ice || *current_ice !=
ice_params) {
Should we support renomination going from true to false (or the reverse)? I
don't see a test for that.

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.h (right):

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.h:199: bool
set_remote_supports_renomination(bool remote_supports_renomination) {
Can the unit tests just use SetRemoteIceParameters now?

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/transport.h
File webrtc/p2p/base/transport.h (right):

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/transport...
webrtc/p2p/base/transport.h:217: bool use_renomination_if_remote_supports =
false;
Why not just use `SetIceParameters` with `renomination` set to true for this
functionality? In fact, that's really what we *should* do, because if you SDP
munge to remove the "renomination" ICE option from the local description, we
shouldn't use renomination.

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/transport...
File webrtc/p2p/base/transportchannelimpl.h (right):

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportchannelimpl.h:60: // in the ice_params must be set
before candidate gathering can start.
nit: "in the ice_params" -> "in |ice_params|" to match current style.

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/transport...
File webrtc/p2p/base/transportdescription.h (right):

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportdescription.h:66: bool renomination;
Why not have "renomination = false" here and then use a default constructor
("IceParameters() = default;") below?

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportdescription.h:71: : ufrag(ice_ufrag), pwd(ice_pwd),
renomination(ice_renomination) {}
nit: The "ice_" prefix in the names seems redundant, since it's an IceParameters
struct.

[honghaiz3, 2016-08-11 04:30:00]

Patchset #2 (id:80001) has been deleted

[honghaiz3, 2016-08-11 04:48:15]

Patchset #2 (id:100001) has been deleted

[honghaiz3, 2016-08-11 04:57:58]

PTAL. Thanks!

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/dtlstrans...
File webrtc/p2p/base/dtlstransportchannel.h (right):

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/dtlstrans...
webrtc/p2p/base/dtlstransportchannel.h:168:
channel_->SetIceParameters(ice_params);
On 2016/08/08 22:28:15, Taylor Brandstetter wrote:
> I think the introduction of IceParameters is a great idea, since it matches an
> ORTC struct:
> http://ortc.org/wp-content/uploads/2016/03/ortc.html#idl-def-RTCIceParameters
> 
> But along those lines, it would make sense to put the ICE mode in this struct
as
> well. If not in this CL, could you add a TODO to do that?

Added TODO. I did not include in this CL because currently ICE mode is set from
a different path than ICE parameters. It is not obvious how to merge that two.

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:311: if (!current_ice || *current_ice !=
ice_params) {
On 2016/08/08 22:28:15, Taylor Brandstetter wrote:
> Should we support renomination going from true to false (or the reverse)? I
> don't see a test for that.
Implementation-wide, I think it allows renomination to change. It will take what
ever is set last. 
But standard/protocol-wise, officially supporting it would require extra
engineering work (at least for the tests you mentioned). 
In practice, there is little reason to change renomination in a session. So I'd
prefer not spending time on that test.

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.h (right):

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.h:199: bool
set_remote_supports_renomination(bool remote_supports_renomination) {
On 2016/08/08 22:28:16, Taylor Brandstetter wrote:
> Can the unit tests just use SetRemoteIceParameters now?

Done.

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.h:204: return true;
On 2016/08/08 21:56:23, pthatcher1 wrote:
> Can the unit tests just use SetRemoteIceParameters?
> 
> IceParameters remote_params = channel.remote_ice_parameters();
> remote_params.renomination = remote_supports_renomination;
> channel.SetRemoteIceParameters(remote_params);

Done.

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/transport.h
File webrtc/p2p/base/transport.h (right):

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/transport...
webrtc/p2p/base/transport.h:217: bool use_renomination_if_remote_supports =
false;
On 2016/08/08 22:28:16, Taylor Brandstetter wrote:
> Why not just use `SetIceParameters` with `renomination` set to true for this
> functionality? In fact, that's really what we *should* do, because if you SDP
> munge to remove the "renomination" ICE option from the local description, we
> shouldn't use renomination.

So we will have a single control to control both 
1. Whether we support ICE renomination. 
2. Whether we will use ICE renomination if the remote supports. 
It makes sense to me. We don't need two control knobs for them.

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/transport...
File webrtc/p2p/base/transportchannelimpl.h (right):

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportchannelimpl.h:60: // in the ice_params must be set
before candidate gathering can start.
On 2016/08/08 21:56:23, pthatcher1 wrote:
> Non-ICE transport channels can't ignore them if they contain an inner ICE
> transport channel.  Then they have to pass them down to the ICE transport

I am not sure in what circumstance a Non-ICE transport channel will contain an
ICE transport channel. 
Note I am just copying the comments over with some name changes.

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportchannelimpl.h:60: // in the ice_params must be set
before candidate gathering can start.
On 2016/08/08 22:28:16, Taylor Brandstetter wrote:
> nit: "in the ice_params" -> "in |ice_params|" to match current style.

Done.

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/transport...
File webrtc/p2p/base/transportdescription.h (right):

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportdescription.h:66: bool renomination;
On 2016/08/08 22:28:16, Taylor Brandstetter wrote:
> Why not have "renomination = false" here and then use a default constructor
> ("IceParameters() = default;") below?

Done. Thanks!

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportdescription.h:71: : ufrag(ice_ufrag), pwd(ice_pwd),
renomination(ice_renomination) {}
On 2016/08/08 22:28:16, Taylor Brandstetter wrote:
> nit: The "ice_" prefix in the names seems redundant, since it's an
IceParameters
> struct.
I generally prefer that the parameter names are different from the member
variables to avoid potential confusion and shadowing, even though there is no
immediate problem here.

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/transport...
File webrtc/p2p/base/transportdescriptionfactory.cc (right):

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportdescriptionfactory.cc:42: }
On 2016/08/08 21:56:23, pthatcher1 wrote:
> I don't see any unit test coverage for this.

Done.

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/transport...
File webrtc/p2p/base/transportdescriptionfactory.h (right):

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportdescriptionfactory.h:27: bool ice_renomination = true;
On 2016/08/08 21:56:23, pthatcher1 wrote:
> I think we should make this false like  use_renomination_if_remote_supports,
and
> make both true if the application opts into it (and later make the default of
> both true, and not allow opt-out).

Done.

https://codereview.webrtc.org/2224563004/diff/140001/webrtc/api/peerconnectio...
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/2224563004/diff/140001/webrtc/api/peerconnectio...
webrtc/api/peerconnection.cc:521: ice_restart = value;
This appeared to be wrong before.
Plus this value was never actually used before because transport_options is
empty yet. 

This CL changed the behavior by using this value. 
I think we probably want to remove setting ice_restart here because it is set in
webrtcsession.
WDYT?

[Taylor Brandstetter, 2016-08-11 22:36:50]

I think if we need a knob for "generate offer/answer with 'renomination' ICE
option", it would be better to put it in the RTCConfiguration than in the
RTCOfferAnswerOptions.

Why do we need a knob though (question for Peter)? We've made many other ICE
modifications without any knob to turn them on, and this one at least requires
negotiation and has an IETF draft, so it seems pretty safe. In other words, I'd
suggest we let ice_renomination default to true like it was in the first version
of this CL:
https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/transport...

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/transport...
File webrtc/p2p/base/transportdescription.h (right):

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportdescription.h:71: : ufrag(ice_ufrag), pwd(ice_pwd),
renomination(ice_renomination) {}
On 2016/08/11 04:57:57, honghaiz3 wrote:
> On 2016/08/08 22:28:16, Taylor Brandstetter wrote:
> > nit: The "ice_" prefix in the names seems redundant, since it's an
> IceParameters
> > struct.
> I generally prefer that the parameter names are different from the member
> variables to avoid potential confusion and shadowing, even though there is no
> immediate problem here. 

Acknowledged; that's a fair point.

https://codereview.webrtc.org/2224563004/diff/140001/webrtc/api/peerconnectio...
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/2224563004/diff/140001/webrtc/api/peerconnectio...
webrtc/api/peerconnection.cc:521: ice_restart = value;
On 2016/08/11 04:57:58, honghaiz3 wrote:
> This appeared to be wrong before.
> Plus this value was never actually used before because transport_options is
> empty yet. 
> 
> This CL changed the behavior by using this value. 
> I think we probably want to remove setting ice_restart here because it is set
in
> webrtcsession.
> WDYT?

You're right; setting the value here (in ParseConstraintsForAnswer) does
nothing, since only the presence of an ICE restart in the offer determines if
there's an ICE restart in the answer. We can remove this.

https://codereview.webrtc.org/2224563004/diff/140001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel.h (right):

https://codereview.webrtc.org/2224563004/diff/140001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.h:207: }
I hoped by using SetRemoteIceParameters we could avoid the additional "public
for unit tests" methods. Why are these necessary? Can the unit test itself
remember the parameters it set?

[honghaiz3, 2016-08-12 18:19:44]

Description was changed from

==========
Add signaling to support ICE renomination.
By default, this will tell the remote side that I am supporting ICE
renomination. 
It does not use ICE renomination yet even if the remote side supports it. It
will be safer to to do that in a separate CL. 

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

to

==========
Add signaling to support ICE renomination.
By default, this will tell the remote side that I am supporting ICE
renomination. 
It does not use ICE renomination yet even if the remote side supports it.
==========

[honghaiz3, 2016-08-12 18:26:40]

Regarding the control knob of renomination. 
We have three options:
1. Do control knob. If both sides support it, it will be enabled. 
2. Have a control knob, set default to true for mobile for now. and set default
to false for the browser. 
If we choose this, it looks easier to add the control as a constraint (similar
to ice_restart). We can remove it later and replace with option 1. 
3. Have a control knob. Each app has to opt in. 
I prefer 1 or 2. 
I will change this part after we converge on the option we choose.

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/transport...
File webrtc/p2p/base/transportdescription.h (right):

https://codereview.webrtc.org/2224563004/diff/60001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportdescription.h:71: : ufrag(ice_ufrag), pwd(ice_pwd),
renomination(ice_renomination) {}
On 2016/08/11 22:36:50, Taylor Brandstetter wrote:
> On 2016/08/11 04:57:57, honghaiz3 wrote:
> > On 2016/08/08 22:28:16, Taylor Brandstetter wrote:
> > > nit: The "ice_" prefix in the names seems redundant, since it's an
> > IceParameters
> > > struct.
> > I generally prefer that the parameter names are different from the member
> > variables to avoid potential confusion and shadowing, even though there is
no
> > immediate problem here. 
> 
> Acknowledged; that's a fair point.

Acknowledged.

https://codereview.webrtc.org/2224563004/diff/140001/webrtc/api/peerconnectio...
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/2224563004/diff/140001/webrtc/api/peerconnectio...
webrtc/api/peerconnection.cc:521: ice_restart = value;
On 2016/08/11 22:36:50, Taylor Brandstetter wrote:
> On 2016/08/11 04:57:58, honghaiz3 wrote:
> > This appeared to be wrong before.
> > Plus this value was never actually used before because transport_options is
> > empty yet. 
> > 
> > This CL changed the behavior by using this value. 
> > I think we probably want to remove setting ice_restart here because it is
set
> in
> > webrtcsession.
> > WDYT?
> 
> You're right; setting the value here (in ParseConstraintsForAnswer) does
> nothing, since only the presence of an ICE restart in the offer determines if
> there's an ICE restart in the answer. We can remove this.

Done.

https://codereview.webrtc.org/2224563004/diff/140001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel.h (right):

https://codereview.webrtc.org/2224563004/diff/140001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.h:207: }
On 2016/08/11 22:36:50, Taylor Brandstetter wrote:
> I hoped by using SetRemoteIceParameters we could avoid the additional "public
> for unit tests" methods. Why are these necessary? Can the unit test itself
> remember the parameters it set?

Done.

[honghaiz3, 2016-08-12 18:26:49]

Patchset #4 (id:160001) has been deleted

[Taylor Brandstetter, 2016-08-17 01:08:18]

Since the "Offer renomination ice-option?" knob will go away eventually, it
probably doesn't matter whether it goes in the RTCConfiguration or the
RTCOfferAnswerOptions. So lgtm.

Peter, what's the plan for making the option offered by default?

https://codereview.webrtc.org/2224563004/diff/200001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2224563004/diff/200001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:311: IceParameters
GetIceParams(const IceParameters& ice, bool renomination) {
nit: "CreateIceParamsWithRenomination" or something similar may be more clear.

[pthatcher1, 2016-08-17 17:21:44]

On 2016/08/17 01:08:18, Taylor Brandstetter wrote:
> Since the "Offer renomination ice-option?" knob will go away eventually, it
> probably doesn't matter whether it goes in the RTCConfiguration or the
> RTCOfferAnswerOptions. So lgtm.

I'd prefer RTCConfiguration.  I'd rather have as little as possible in
RTCOfferAnswerOptions.

> 
> Peter, what's the plan for making the option offered by default?

Roll out in chrome behind a field trial (start with off by default, then roll to
on by default).  See if anyone turns it off or complains.  If not, remove the
option to turn it off.


> 
>
https://codereview.webrtc.org/2224563004/diff/200001/webrtc/p2p/base/p2ptrans...
> File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):
> 
>
https://codereview.webrtc.org/2224563004/diff/200001/webrtc/p2p/base/p2ptrans...
> webrtc/p2p/base/p2ptransportchannel_unittest.cc:311: IceParameters
> GetIceParams(const IceParameters& ice, bool renomination) {
> nit: "CreateIceParamsWithRenomination" or something similar may be more clear.

[pthatcher1, 2016-08-17 17:21:45]

On 2016/08/17 01:08:18, Taylor Brandstetter wrote:
> Since the "Offer renomination ice-option?" knob will go away eventually, it
> probably doesn't matter whether it goes in the RTCConfiguration or the
> RTCOfferAnswerOptions. So lgtm.

I'd prefer RTCConfiguration.  I'd rather have as little as possible in
RTCOfferAnswerOptions.

> 
> Peter, what's the plan for making the option offered by default?

Roll out in chrome behind a field trial (start with off by default, then roll to
on by default).  See if anyone turns it off or complains.  If not, remove the
option to turn it off.


> 
>
https://codereview.webrtc.org/2224563004/diff/200001/webrtc/p2p/base/p2ptrans...
> File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):
> 
>
https://codereview.webrtc.org/2224563004/diff/200001/webrtc/p2p/base/p2ptrans...
> webrtc/p2p/base/p2ptransportchannel_unittest.cc:311: IceParameters
> GetIceParams(const IceParameters& ice, bool renomination) {
> nit: "CreateIceParamsWithRenomination" or something similar may be more clear.

[pthatcher1, 2016-08-17 17:25:46]

https://codereview.webrtc.org/2224563004/diff/200001/webrtc/api/peerconnectio...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/2224563004/diff/200001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface.h:321: bool ice_renomination = false;
It seems so much easier to just put this in RTCConfiguration.

https://codereview.webrtc.org/2224563004/diff/200001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2224563004/diff/200001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:311: IceParameters
GetIceParams(const IceParameters& ice, bool renomination) {
On 2016/08/17 01:08:17, Taylor Brandstetter wrote:
> nit: "CreateIceParamsWithRenomination" or something similar may be more clear.

+1

[honghaiz3, 2016-08-17 20:21:49]

Thanks! PTAL.

https://codereview.webrtc.org/2224563004/diff/200001/webrtc/api/peerconnectio...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/2224563004/diff/200001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface.h:321: bool ice_renomination = false;
On 2016/08/17 17:25:46, pthatcher1 wrote:
> It seems so much easier to just put this in RTCConfiguration.  

Done.

https://codereview.webrtc.org/2224563004/diff/200001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2224563004/diff/200001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:311: IceParameters
GetIceParams(const IceParameters& ice, bool renomination) {
On 2016/08/17 17:25:46, pthatcher1 wrote:
> On 2016/08/17 01:08:17, Taylor Brandstetter wrote:
> > nit: "CreateIceParamsWithRenomination" or something similar may be more
clear.
> 
> +1

Done.

[honghaiz3, 2016-08-17 20:35:34]

Patchset #7 (id:240001) has been deleted

[Taylor Brandstetter, 2016-08-17 22:02:00]

https://codereview.webrtc.org/2224563004/diff/260001/webrtc/api/peerconnectio...
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/2224563004/diff/260001/webrtc/api/peerconnectio...
webrtc/api/peerconnection.cc:476: cricket::CN_AUDIO, cricket::CN_VIDEO,
cricket::CN_DATA};
This won't work interoperably. The offer could contain any content names;
"CN_AUDIO" etc. are just defaults.

I would recommend setting TransportOptions::ice_renomination where the
TransportOptions are initialized. Meaning in GetOptionsForOffer and
InitializeOptionsForAnswer (you're already doing the latter). There could be a
helper function "MakeTransportOptions" that just does:

TransportOptions options;
options.ice_renomination = ice_renomination_;
return options;

https://codereview.webrtc.org/2224563004/diff/260001/webrtc/api/peerconnectio...
File webrtc/api/peerconnection_unittest.cc (right):

https://codereview.webrtc.org/2224563004/diff/260001/webrtc/api/peerconnectio...
webrtc/api/peerconnection_unittest.cc:2113: TEST_F(P2PTestConductor,
IceRenominationWithoutIceRenomination) {
nit: IceRenominationWithoutIceRenomination sounds confusing. Could it be
IceRenominationDisabled and IceRenominationEnabled?

https://codereview.webrtc.org/2224563004/diff/260001/webrtc/api/peerconnectio...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/2224563004/diff/260001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface.h:309: bool ice_renomination = false;
nit: Could add comment explaining that this controls whether "renomination" is
added to the ICE options.

https://codereview.webrtc.org/2224563004/diff/260001/webrtc/api/peerconnectio...
File webrtc/api/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/2224563004/diff/260001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface_unittest.cc:2772:
TEST(CreateSessionOptionsTest, GetOptionsWithIceRenomination) {
This test can be removed if setting the ice_renomination flag happens outside of
ExtractMediaSessionOptions.

https://codereview.webrtc.org/2224563004/diff/260001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2224563004/diff/260001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:318: void CreateChannels(int
num, bool renomination = false) {
nit: Similar to my last comment, I'd prefer to have two methods:
CreateChannelsWithRenomination and a plain CreateChannels (for when the test
doesn't care whether renomination is used).

[honghaiz3, 2016-08-19 18:42:01]

Thanks! PTAL.

https://codereview.webrtc.org/2224563004/diff/260001/webrtc/api/peerconnectio...
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/2224563004/diff/260001/webrtc/api/peerconnectio...
webrtc/api/peerconnection.cc:476: cricket::CN_AUDIO, cricket::CN_VIDEO,
cricket::CN_DATA};
On 2016/08/17 22:01:59, Taylor Brandstetter wrote:
> This won't work interoperably. The offer could contain any content names;
> "CN_AUDIO" etc. are just defaults.
> 
> I would recommend setting TransportOptions::ice_renomination where the
> TransportOptions are initialized. Meaning in GetOptionsForOffer and
> InitializeOptionsForAnswer (you're already doing the latter). There could be a
> helper function "MakeTransportOptions" that just does:
> 
> TransportOptions options;
> options.ice_renomination = ice_renomination_;
> return options;
I don't think that would work. 
When we initialize an answer, it can extract the content name from the remote
description. But when we initialize an offer, there is no existing local
description or remote one. 
I think "audio", "video", "data" might be default in the standard, but they are
hard-coded in the offer by webrtc and they are unlikely to change. If they will
ever change, the test I wrote should capture it. 
We could make it work with arbitrarily chosen content, but that will need some
convoluted changes.

https://codereview.webrtc.org/2224563004/diff/260001/webrtc/api/peerconnectio...
File webrtc/api/peerconnection_unittest.cc (right):

https://codereview.webrtc.org/2224563004/diff/260001/webrtc/api/peerconnectio...
webrtc/api/peerconnection_unittest.cc:2113: TEST_F(P2PTestConductor,
IceRenominationWithoutIceRenomination) {
On 2016/08/17 22:02:00, Taylor Brandstetter wrote:
> nit: IceRenominationWithoutIceRenomination sounds confusing. Could it be
> IceRenominationDisabled and IceRenominationEnabled?

Done. Good points.

https://codereview.webrtc.org/2224563004/diff/260001/webrtc/api/peerconnectio...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/2224563004/diff/260001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface.h:309: bool ice_renomination = false;
On 2016/08/17 22:02:00, Taylor Brandstetter wrote:
> nit: Could add comment explaining that this controls whether "renomination" is
> added to the ICE options.

Done.

https://codereview.webrtc.org/2224563004/diff/260001/webrtc/api/peerconnectio...
File webrtc/api/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/2224563004/diff/260001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface_unittest.cc:2772:
TEST(CreateSessionOptionsTest, GetOptionsWithIceRenomination) {
On 2016/08/17 22:02:00, Taylor Brandstetter wrote:
> This test can be removed if setting the ice_renomination flag happens outside
of
> ExtractMediaSessionOptions.
True but only if we removed it from the ExtractMediaSessionOptions.

https://codereview.webrtc.org/2224563004/diff/260001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2224563004/diff/260001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:318: void CreateChannels(int
num, bool renomination = false) {
On 2016/08/17 22:02:00, Taylor Brandstetter wrote:
> nit: Similar to my last comment, I'd prefer to have two methods:
> CreateChannelsWithRenomination and a plain CreateChannels (for when the test
> doesn't care whether renomination is used).
I would argue for using a single method with a renomination parameter. 
It improves code reuse. 
Plus, we may want to change in the future that renomination is true by default
in all the test, or each test has two versions, one with renomination and one
without. Using an argument will make those change simpler.

[Taylor Brandstetter, 2016-08-20 00:52:13]

https://codereview.webrtc.org/2224563004/diff/260001/webrtc/api/peerconnectio...
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/2224563004/diff/260001/webrtc/api/peerconnectio...
webrtc/api/peerconnection.cc:476: cricket::CN_AUDIO, cricket::CN_VIDEO,
cricket::CN_DATA};
On 2016/08/19 18:42:00, honghaiz3 wrote:
> On 2016/08/17 22:01:59, Taylor Brandstetter wrote:
> > This won't work interoperably. The offer could contain any content names;
> > "CN_AUDIO" etc. are just defaults.
> > 
> > I would recommend setting TransportOptions::ice_renomination where the
> > TransportOptions are initialized. Meaning in GetOptionsForOffer and
> > InitializeOptionsForAnswer (you're already doing the latter). There could be
a
> > helper function "MakeTransportOptions" that just does:
> > 
> > TransportOptions options;
> > options.ice_renomination = ice_renomination_;
> > return options;
> I don't think that would work. 
> When we initialize an answer, it can extract the content name from the remote
> description. But when we initialize an offer, there is no existing local
> description or remote one.
> I think "audio", "video", "data" might be default in the standard, but they
are
> hard-coded in the offer by webrtc and they are unlikely to change. If they
will
> ever change, the test I wrote should capture it. 
> We could make it work with arbitrarily chosen content, but that will need some
> convoluted changes. 

Sorry, I just noticed that GetOptionsForOffer *does* add the content names from
the current local description. So there's nothing lacking.

I'd just suggest moving this code into GetOptionsForOffer where the other
TransportOptions are added, since I believe ExtractMediaSessionOptions is meant
as "extract media session options from RTCOfferAnswerOptions."

https://codereview.webrtc.org/2224563004/diff/260001/webrtc/api/peerconnectio...
webrtc/api/peerconnection.cc:1621: }
Here's where I think it would make sense to add the default TransportOptions
(for CN_AUDIO, CN_VIDEO) and initialize ice_renomination to the proper value.

https://codereview.webrtc.org/2224563004/diff/260001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2224563004/diff/260001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:318: void CreateChannels(int
num, bool renomination = false) {
On 2016/08/19 18:42:01, honghaiz3 wrote:
> On 2016/08/17 22:02:00, Taylor Brandstetter wrote:
> > nit: Similar to my last comment, I'd prefer to have two methods:
> > CreateChannelsWithRenomination and a plain CreateChannels (for when the test
> > doesn't care whether renomination is used).
> I would argue for using a single method with a renomination parameter. 
> It improves code reuse. 
> Plus, we may want to change in the future that renomination is true by default
> in all the test, or each test has two versions, one with renomination and one
> without. Using an argument will make those change simpler. 

I do agree there should be a renomination argument; what I meant was:

void CreateChannelsWithRenomination(int num, bool renomination) {
...
}

void CreateChannels(int num) { CreateChannelsWithRenomination(num, false;); }

The "false" can still be changed to a "true", I'm just saying I prefer when a
method's name gives a hint about what its arguments mean. But this was just a
nit so it's good as is.

[honghaiz3, 2016-08-22 18:46:44]

https://codereview.webrtc.org/2224563004/diff/260001/webrtc/api/peerconnectio...
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/2224563004/diff/260001/webrtc/api/peerconnectio...
webrtc/api/peerconnection.cc:476: cricket::CN_AUDIO, cricket::CN_VIDEO,
cricket::CN_DATA};
On 2016/08/20 00:52:13, Taylor Brandstetter wrote:
> On 2016/08/19 18:42:00, honghaiz3 wrote:
> > On 2016/08/17 22:01:59, Taylor Brandstetter wrote:
> > > This won't work interoperably. The offer could contain any content names;
> > > "CN_AUDIO" etc. are just defaults.
> > > 
> > > I would recommend setting TransportOptions::ice_renomination where the
> > > TransportOptions are initialized. Meaning in GetOptionsForOffer and
> > > InitializeOptionsForAnswer (you're already doing the latter). There could
be
> a
> > > helper function "MakeTransportOptions" that just does:
> > > 
> > > TransportOptions options;
> > > options.ice_renomination = ice_renomination_;
> > > return options;
> > I don't think that would work. 
> > When we initialize an answer, it can extract the content name from the
remote
> > description. But when we initialize an offer, there is no existing local
> > description or remote one.
> > I think "audio", "video", "data" might be default in the standard, but they
> are
> > hard-coded in the offer by webrtc and they are unlikely to change. If they
> will
> > ever change, the test I wrote should capture it. 
> > We could make it work with arbitrarily chosen content, but that will need
some
> > convoluted changes. 
> 
> Sorry, I just noticed that GetOptionsForOffer *does* add the content names
from
> the current local description. So there's nothing lacking.
> 
> I'd just suggest moving this code into GetOptionsForOffer where the other
> TransportOptions are added, since I believe ExtractMediaSessionOptions is
meant
> as "extract media session options from RTCOfferAnswerOptions."

Done.

https://codereview.webrtc.org/2224563004/diff/260001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2224563004/diff/260001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:318: void CreateChannels(int
num, bool renomination = false) {
On 2016/08/20 00:52:13, Taylor Brandstetter wrote:
> On 2016/08/19 18:42:01, honghaiz3 wrote:
> > On 2016/08/17 22:02:00, Taylor Brandstetter wrote:
> > > nit: Similar to my last comment, I'd prefer to have two methods:
> > > CreateChannelsWithRenomination and a plain CreateChannels (for when the
test
> > > doesn't care whether renomination is used).
> > I would argue for using a single method with a renomination parameter. 
> > It improves code reuse. 
> > Plus, we may want to change in the future that renomination is true by
default
> > in all the test, or each test has two versions, one with renomination and
one
> > without. Using an argument will make those change simpler. 
> 
> I do agree there should be a renomination argument; what I meant was:
> 
> void CreateChannelsWithRenomination(int num, bool renomination) {
> ...
> }
> 
> void CreateChannels(int num) { CreateChannelsWithRenomination(num, false;); }
> 
> The "false" can still be changed to a "true", I'm just saying I prefer when a
> method's name gives a hint about what its arguments mean. But this was just a
> nit so it's good as is.

Done.

[honghaiz3, 2016-08-22 18:48:05]

Patchset #10 (id:320001) has been deleted

[Taylor Brandstetter, 2016-08-22 20:48:42]

lgtm

[pthatcher1, 2016-08-22 23:02:29]

https://codereview.webrtc.org/2224563004/diff/340001/webrtc/api/peerconnectio...
File webrtc/api/peerconnection.cc (left):

https://codereview.webrtc.org/2224563004/diff/340001/webrtc/api/peerconnectio...
webrtc/api/peerconnection.cc:525: 
There is so much refactoring in this CL now that it's hard to tell the what's a
behavior change and what's not.  Can we make refactorings like this a separate
CL from the behavior change?

https://codereview.webrtc.org/2224563004/diff/340001/webrtc/api/peerconnectio...
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/2224563004/diff/340001/webrtc/api/peerconnectio...
webrtc/api/peerconnection.cc:1603: InitializeMediaSessionOptions(true,
session_options);
Can you put a "bool is_offer = true" and then
InitializeMediaSessionOptions(is_offer, session_options)?  That would make it
easier to read.   Here and the other 2 places.

https://codereview.webrtc.org/2224563004/diff/340001/webrtc/api/peerconnectio...
webrtc/api/peerconnection.cc:1668: cricket::CN_AUDIO, cricket::CN_VIDEO,
cricket::CN_DATA};
Isn't it kind of dangerous to assume we want these three content names?  Would
it be cleaner to add a MediaSessionOptions.ice_renomination?  Then in
MediaSessionDescriptionFactory::AddVideoContentForOffer (and similar), we could
have cricket::GetTransportOptions copy from MediaSessionOptions.ice_renomination
to TransportOptions.ice_renomination in one place, and not have to do it 3
places here.  And we wouldn't have to assume content names here.

[honghaiz3, 2016-08-23 00:46:21]

Patchset #11 (id:360001) has been deleted

[honghaiz3, 2016-08-23 00:52:28]

Thanks! PTAL.

https://codereview.webrtc.org/2224563004/diff/340001/webrtc/api/peerconnectio...
File webrtc/api/peerconnection.cc (left):

https://codereview.webrtc.org/2224563004/diff/340001/webrtc/api/peerconnectio...
webrtc/api/peerconnection.cc:525: 
On 2016/08/22 23:02:29, pthatcher1 wrote:
> There is so much refactoring in this CL now that it's hard to tell the what's
a
> behavior change and what's not.  Can we make refactorings like this a separate
> CL from the behavior change?  
Reverted this change. This part is basically a no-op anyway.

https://codereview.webrtc.org/2224563004/diff/340001/webrtc/api/peerconnectio...
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/2224563004/diff/340001/webrtc/api/peerconnectio...
webrtc/api/peerconnection.cc:1603: InitializeMediaSessionOptions(true,
session_options);
On 2016/08/22 23:02:29, pthatcher1 wrote:
> Can you put a "bool is_offer = true" and then
> InitializeMediaSessionOptions(is_offer, session_options)?  That would make it
> easier to read.   Here and the other 2 places.

This method is removed now.

https://codereview.webrtc.org/2224563004/diff/340001/webrtc/api/peerconnectio...
webrtc/api/peerconnection.cc:1668: cricket::CN_AUDIO, cricket::CN_VIDEO,
cricket::CN_DATA};
On 2016/08/22 23:02:29, pthatcher1 wrote:
> Isn't it kind of dangerous to assume we want these three content names?  Would
> it be cleaner to add a MediaSessionOptions.ice_renomination?  Then in
> MediaSessionDescriptionFactory::AddVideoContentForOffer (and similar), we
could
> have cricket::GetTransportOptions copy from
MediaSessionOptions.ice_renomination
> to TransportOptions.ice_renomination in one place, and not have to do it 3
> places here.  And we wouldn't have to assume content names here.

Done.

[Taylor Brandstetter, 2016-08-23 21:54:56]

lgtm

By the way, in the future it may be helpful to give a description for patches,
so I know which ones I've already reviewed.

[skvlad, 2016-08-26 23:59:09]

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

[skvlad, 2016-08-26 23:59:10]

lgtm

https://codereview.webrtc.org/2224563004/diff/400001/webrtc/api/peerconnectio...
File webrtc/api/peerconnection_unittest.cc (right):

https://codereview.webrtc.org/2224563004/diff/400001/webrtc/api/peerconnectio...
webrtc/api/peerconnection_unittest.cc:395: void SetExpectIceRenomination() {
expect_ice_renomination_ = true; }
Should this take a bool just like SetExpectRemoteIceRenomination()?

https://codereview.webrtc.org/2224563004/diff/400001/webrtc/api/peerconnectio...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/2224563004/diff/400001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface.h:311: bool ice_renomination = false;
Would it be better to call it "enable_ice_renomination" /
"support_ice_renomination"? Boolean variables are easier to read when they are
verbs (enable_foo) than nouns.
Or you can use use an enum to more explicitly indicate what it means
(IceRenominationCapability::Disabled, IceRenominationCapability::Required,...)

https://codereview.webrtc.org/2224563004/diff/400001/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/2224563004/diff/400001/webrtc/p2p/base/port.h#n...
webrtc/p2p/base/port.h:594: // ufrag in |remote_ice_parameters| matches the
candidate's ufrag, and the
There are no |remote_ice_parameters| any more. Should this be
|remote_candidate_|?

https://codereview.webrtc.org/2224563004/diff/400001/webrtc/pc/mediasession.cc
File webrtc/pc/mediasession.cc (right):

https://codereview.webrtc.org/2224563004/diff/400001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:730: transport_options.ice_renomination =
options.ice_renomination;
Is options.ice_renomination always supposed to override
transport_options.ice_renomination? In that case, what is the reason to keep
them separate?

[honghaiz3, 2016-08-29 18:52:53]

PTAL.

https://codereview.webrtc.org/2224563004/diff/400001/webrtc/api/peerconnectio...
File webrtc/api/peerconnection_unittest.cc (right):

https://codereview.webrtc.org/2224563004/diff/400001/webrtc/api/peerconnectio...
webrtc/api/peerconnection_unittest.cc:395: void SetExpectIceRenomination() {
expect_ice_renomination_ = true; }
On 2016/08/26 23:59:10, skvlad wrote:
> Should this take a bool just like SetExpectRemoteIceRenomination()?
Done.

https://codereview.webrtc.org/2224563004/diff/400001/webrtc/api/peerconnectio...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/2224563004/diff/400001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface.h:311: bool ice_renomination = false;
On 2016/08/26 23:59:10, skvlad wrote:
> Would it be better to call it "enable_ice_renomination" /
> "support_ice_renomination"? Boolean variables are easier to read when they are
> verbs (enable_foo) than nouns.
> Or you can use use an enum to more explicitly indicate what it means
> (IceRenominationCapability::Disabled, IceRenominationCapability::Required,...)

Done.

https://codereview.webrtc.org/2224563004/diff/400001/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/2224563004/diff/400001/webrtc/p2p/base/port.h#n...
webrtc/p2p/base/port.h:594: // ufrag in |remote_ice_parameters| matches the
candidate's ufrag, and the
On 2016/08/26 23:59:10, skvlad wrote:
> There are no |remote_ice_parameters| any more. Should this be
> |remote_candidate_|?

Done. Good observation. Changed this to |params| and updated the comments.

https://codereview.webrtc.org/2224563004/diff/400001/webrtc/pc/mediasession.cc
File webrtc/pc/mediasession.cc (right):

https://codereview.webrtc.org/2224563004/diff/400001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:730: transport_options.ice_renomination =
options.ice_renomination;
On 2016/08/26 23:59:10, skvlad wrote:
> Is options.ice_renomination always supposed to override
> transport_options.ice_renomination? In that case, what is the reason to keep
> them separate?
Good point. I think that is the reason I have not chosen this approach at the
initial version (see patch 10). 
The reason to keep two ice_renomination options which look redundant is that the
transport options are keyed by the content names, and when we set the transport
options, the content names may not be available yet (for the first offer, for
example). So it is safer to set the ice_renomination in the Session Option and
copy it over to the transport option when the content name is known.

[skvlad, 2016-08-30 01:29:25]

lgtm

[pthatcher1, 2016-08-31 00:28:42]

lgtm

[honghaiz3, 2016-08-31 05:13:03]

The CQ bit was checked by honghaiz@webrtc.org

[honghaiz3, 2016-08-31 05:13:04]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org,
pthatcher@webrtc.org, skvlad@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2224563004/#ps480001 (title:
"Merge")

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

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

[commit-bot: I haz the power, 2016-08-31 07:13:40]

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

[commit-bot: I haz the power, 2016-08-31 07:13:41]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[honghaiz3, 2016-08-31 15:18:26]

Description was changed from

==========
Add signaling to support ICE renomination.
By default, this will tell the remote side that I am supporting ICE
renomination. 
It does not use ICE renomination yet even if the remote side supports it.
==========

to

==========
Add signaling to support ICE renomination.
By default, this will tell the remote side that I am supporting ICE
renomination.
It does not use ICE renomination yet even if the remote side supports it.

R=deadbeef@webrtc.org, pthatcher@webrtc.org, skvlad@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/4cedf2b78cefba056ca423fa1...
==========

[commit-bot: I haz the power, 2016-08-31 15:18:26]

Description was changed from

==========
Add signaling to support ICE renomination.
By default, this will tell the remote side that I am supporting ICE
renomination. 
It does not use ICE renomination yet even if the remote side supports it.
==========

to

==========
Add signaling to support ICE renomination.
By default, this will tell the remote side that I am supporting ICE
renomination.
It does not use ICE renomination yet even if the remote side supports it.

R=deadbeef@webrtc.org, pthatcher@webrtc.org, skvlad@webrtc.org

Committed: https://crrev.com/4cedf2b78cefba056ca423fa101bdb2e8cfed79e
Cr-Commit-Position: refs/heads/master@{#13998}
==========

[commit-bot: I haz the power, 2016-08-31 15:18:28]

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

[honghaiz3, 2016-08-31 15:18:28]

Committed patchset #16 (id:480001) manually as
4cedf2b78cefba056ca423fa101bdb2e8cfed79e (presubmit successful).