Add a switch to redetermine role when ICE restarts.

Add a switch to redetermine role when ICE restarts.

R=pthatcher@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/bfd398ccda27550629ec2440888f4083e4510069

[honghaiz3, 2016-08-29 20:58:59]

Patchset #1 (id:1) has been deleted

[honghaiz3, 2016-08-29 20:59:06]

Patchset #1 (id:20001) has been deleted

[honghaiz3, 2016-08-29 21:07:21]

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

[pthatcher1, 2016-08-29 22:18:07]

https://codereview.webrtc.org/2295493002/diff/40001/webrtc/api/peerconnection...
File webrtc/api/peerconnectionfactory.cc (right):

https://codereview.webrtc.org/2295493002/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnectionfactory.cc:317: redetermine_role_on_ice_restart);
Don't you need to change transportcontroller.h also?  I don't see that in this
CL.

https://codereview.webrtc.org/2295493002/diff/40001/webrtc/api/peerconnection...
File webrtc/api/peerconnectionfactory.h (right):

https://codereview.webrtc.org/2295493002/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnectionfactory.h:95: bool redetermine_role_on_ice_restart);
Can we make this a struct instead of a bool?  Then when we add new things over
time, we just add them to the struct rather than adding more and more arguments.

https://codereview.webrtc.org/2295493002/diff/40001/webrtc/api/peerconnection...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/2295493002/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface.h:309: bool redetermine_role_on_ice_restart =
true;
Should this have a comment?

[honghaiz3, 2016-08-29 23:16:59]

https://codereview.webrtc.org/2295493002/diff/40001/webrtc/api/peerconnection...
File webrtc/api/peerconnectionfactory.cc (right):

https://codereview.webrtc.org/2295493002/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnectionfactory.cc:317: redetermine_role_on_ice_restart);
On 2016/08/29 22:18:07, pthatcher1 wrote:
> Don't you need to change transportcontroller.h also?  I don't see that in this
> CL.
Taylor landed a CL to change the transportcontroller.

https://codereview.webrtc.org/2295493002/diff/40001/webrtc/api/peerconnection...
File webrtc/api/peerconnectionfactory.h (right):

https://codereview.webrtc.org/2295493002/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnectionfactory.h:95: bool redetermine_role_on_ice_restart);
On 2016/08/29 22:18:07, pthatcher1 wrote:
> Can we make this a struct instead of a bool?  Then when we add new things over
> time, we just add them to the struct rather than adding more and more
arguments.
Can we delay using a struct until we need to add a second argument?
The chance is that we may not need a second argument at all, which will make the
struct look weird. 
Plus, I think our plan is to get rid of this argument once we can safely switch
to "not redetermine the role" in Chrome. In fact, I think it might be better to
use a setter method instead of changing the constructor because of this reason,
but we have already made changes to change the constructor.

https://codereview.webrtc.org/2295493002/diff/40001/webrtc/api/peerconnection...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/2295493002/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface.h:309: bool redetermine_role_on_ice_restart =
true;
On 2016/08/29 22:18:07, pthatcher1 wrote:
> Should this have a comment?

Done.

[pthatcher1, 2016-08-29 23:27:42]

https://codereview.webrtc.org/2295493002/diff/40001/webrtc/api/peerconnection...
File webrtc/api/peerconnectionfactory.cc (right):

https://codereview.webrtc.org/2295493002/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnectionfactory.cc:317: redetermine_role_on_ice_restart);
On 2016/08/29 23:16:59, honghaiz3 wrote:
> On 2016/08/29 22:18:07, pthatcher1 wrote:
> > Don't you need to change transportcontroller.h also?  I don't see that in
this
> > CL.
> Taylor landed a CL to change the transportcontroller. 

Ah, it's not in cs.chromium.org yet.

https://codereview.webrtc.org/2295493002/diff/40001/webrtc/api/peerconnection...
File webrtc/api/peerconnectionfactory.h (right):

https://codereview.webrtc.org/2295493002/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnectionfactory.h:95: bool redetermine_role_on_ice_restart);
On 2016/08/29 23:16:59, honghaiz3 wrote:
> On 2016/08/29 22:18:07, pthatcher1 wrote:
> > Can we make this a struct instead of a bool?  Then when we add new things
over
> > time, we just add them to the struct rather than adding more and more
> arguments.
> Can we delay using a struct until we need to add a second argument?
> The chance is that we may not need a second argument at all, which will make
the
> struct look weird. 
> Plus, I think our plan is to get rid of this argument once we can safely
switch
> to "not redetermine the role" in Chrome. In fact, I think it might be better
to
> use a setter method instead of changing the constructor because of this
reason,
> but we have already made changes to change the constructor.  


Good point about removing it soon.

https://codereview.webrtc.org/2295493002/diff/60001/webrtc/api/android/jni/pe...
File webrtc/api/android/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/2295493002/diff/60001/webrtc/api/android/jni/pe...
webrtc/api/android/jni/peerconnection_jni.cc:1634: jfieldID
j_redetermine_role_on_ice_restart_id =
If we're only going to use the switch in Chromium, do we need this Java/JNI
code?

https://codereview.webrtc.org/2295493002/diff/60001/webrtc/api/peerconnection...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/2295493002/diff/60001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface.h:311: bool redetermine_role_on_ice_restart =
true;
Can we make the default = false?

[honghaiz3, 2016-08-29 23:32:27]

PTAL.

https://codereview.webrtc.org/2295493002/diff/60001/webrtc/api/android/jni/pe...
File webrtc/api/android/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/2295493002/diff/60001/webrtc/api/android/jni/pe...
webrtc/api/android/jni/peerconnection_jni.cc:1634: jfieldID
j_redetermine_role_on_ice_restart_id =
On 2016/08/29 23:27:42, pthatcher1 wrote:
> If we're only going to use the switch in Chromium, do we need this Java/JNI
> code?
> 

Done. Removed.

https://codereview.webrtc.org/2295493002/diff/60001/webrtc/api/peerconnection...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/2295493002/diff/60001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface.h:311: bool redetermine_role_on_ice_restart =
true;
On 2016/08/29 23:27:42, pthatcher1 wrote:
> Can we make the default = false?

Done.

[honghaiz3, 2016-08-30 01:33:43]

I think we can also add a new method like
RtcConfiguration::NativeRtcConfiguration to return default configs for native
apps. 
It can be something like 
RtcConfiguration::NativeRtcConfiguration() {
  RtcConfiguration config;
  config.redetermine_role_on_ice_restart = false; // while setting the default
to true. 
  return config;
}
If we use this config as the default config for native app, then we don't need
to touch chromium code. 

WDYT?

[honghaiz3, 2016-08-30 19:59:51]

Patch 4 is the one based on my last suggestion. 
It will make it easier in the future to set other default native configuration,
such as ICE renomination. 
Or we can just go back to Patch 3. 
PTAL.

[pthatcher1, 2016-08-30 20:25:36]

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

https://codereview.webrtc.org/2295493002/diff/100001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface.h:243: static RTCConfiguration
NativeConfiguration() {
Can we call this OptimalConfiguration()?

And have a SafeConfiguration()?

And have comments for each of them describing when one should use one and one
should use the other?

https://codereview.webrtc.org/2295493002/diff/100001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface.h:245: config.redetermine_role_on_ice_restart
= false;
Can you also enable things like max_bundle, required rtcp_mux, and continual
gathering?

[honghaiz3, 2016-08-30 21:32:59]

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

https://codereview.webrtc.org/2295493002/diff/100001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface.h:243: static RTCConfiguration
NativeConfiguration() {
On 2016/08/30 20:25:36, pthatcher1 wrote:
> Can we call this OptimalConfiguration()?
> 
> And have a SafeConfiguration()?
> 
> And have comments for each of them describing when one should use one and one
> should use the other?

Done. Is it better to call it ExperiementalConfiguration, which is more like an
opposite to SafeConfiguration?

https://codereview.webrtc.org/2295493002/diff/100001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface.h:245: config.redetermine_role_on_ice_restart
= false;
On 2016/08/30 20:25:36, pthatcher1 wrote:
> Can you also enable things like max_bundle, required rtcp_mux, and continual
> gathering?

Will do this in a separate CL as discussed.

[pthatcher1, 2016-08-30 23:47:01]

lgtm, assuming you change the name

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

https://codereview.webrtc.org/2295493002/diff/120001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface.h:248: // may be riskier and may need extra
support in the application.
I wouldn't call it experimental.  I'd call it PerformantConfiguration, since "it
has better performance".

[honghaiz3, 2016-08-31 00:02:58]

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

https://codereview.webrtc.org/2295493002/diff/120001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface.h:248: // may be riskier and may need extra
support in the application.
On 2016/08/30 23:47:01, pthatcher1 wrote:
> I wouldn't call it experimental.  I'd call it PerformantConfiguration, since
"it
> has better performance".

Changed to AggressiveConfiguration based on discussion.

[honghaiz3, 2016-08-31 00:03:04]

The CQ bit was checked by honghaiz@webrtc.org

[honghaiz3, 2016-08-31 00:03:04]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2295493002/#ps140001 (title:
"Changed ExperimentalConfiguration to AggressiveConfiguration")

[commit-bot: I haz the power, 2016-08-31 00:03:16]

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 00:23:39]

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

[commit-bot: I haz the power, 2016-08-31 00:23:41]

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

[honghaiz3, 2016-08-31 04:49:07]

The CQ bit was checked by honghaiz@webrtc.org

[honghaiz3, 2016-08-31 04:49:08]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2295493002/#ps160001 (title:
"Remove the objective-c change to submit")

[commit-bot: I haz the power, 2016-08-31 04:49:19]

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 05:08:01]

Description was changed from

==========
Add a switch to redetermine role when ICE restarts.
==========

to

==========
Add a switch to redetermine role when ICE restarts.

R=pthatcher@webrtc.org

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

[honghaiz3, 2016-08-31 05:08:01]

Description was changed from

==========
Add a switch to redetermine role when ICE restarts.
==========

to

==========
Add a switch to redetermine role when ICE restarts.

R=pthatcher@webrtc.org

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

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

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

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

Committed patchset #7 (id:160001) manually as
bfd398ccda27550629ec2440888f4083e4510069 (presubmit successful).