Use AggressiveConfiguration as the default configuration in IOS

Use AggressiveConfiguration as the default configuration in IOS

R=haysc@webrtc.org, pthatcher@webrtc.org, tkchin@webrtc.org

Committed: https://crrev.com/f7ddc06a43408299b2ae07f6b4a40db2b81670a6
Cr-Commit-Position: refs/heads/master@{#14030}

[honghaiz3, 2016-08-31 05:02:07]

Description was changed from

==========
Change the RTCConfiguration in objective c to use AggressiveConfiguration.
==========

to

==========
Use AggressiveConfiguration as the default configuration in IOS
==========

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

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

[tkchin_webrtc, 2016-08-31 07:37:51]

https://codereview.webrtc.org/2297663004/diff/1/webrtc/sdk/objc/Framework/Cla...
File webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm (right):

https://codereview.webrtc.org/2297663004/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm:45:
PeerConnectionInterface::RTCConfiguration::AggressiveConfiguration();
why is this the default just for iOS and not the default for RTCConfiguration?

[honghaiz3, 2016-08-31 15:08:52]

https://codereview.webrtc.org/2297663004/diff/1/webrtc/sdk/objc/Framework/Cla...
File webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm (right):

https://codereview.webrtc.org/2297663004/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm:45:
PeerConnectionInterface::RTCConfiguration::AggressiveConfiguration();
On 2016/08/31 07:37:51, tkchin_webrtc_OOO_till_Sept_12 wrote:
> why is this the default just for iOS and not the default for RTCConfiguration?

We plan to have a separate default config for native application than the one
used by Chrome.
This will make it simpler to change the default config for native applications
in the future. 
Nevertheless, the config is not really specific to native apps. Any app can opt
to use this config to obtain better performance although with some risk. So it
is named AggressiveConfiguration instead. 
The change on Android has been made in 
https://codereview.webrtc.org/2295493002/

[tkchin_webrtc, 2016-08-31 17:50:11]

On 2016/08/31 15:08:52, honghaiz3 wrote:
>
https://codereview.webrtc.org/2297663004/diff/1/webrtc/sdk/objc/Framework/Cla...
> File webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm (right):
> 
>
https://codereview.webrtc.org/2297663004/diff/1/webrtc/sdk/objc/Framework/Cla...
> webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm:45:
> PeerConnectionInterface::RTCConfiguration::AggressiveConfiguration();
> On 2016/08/31 07:37:51, tkchin_webrtc_OOO_till_Sept_12 wrote:
> > why is this the default just for iOS and not the default for
RTCConfiguration?
> 
> We plan to have a separate default config for native application than the one
> used by Chrome.
> This will make it simpler to change the default config for native applications
> in the future. 
> Nevertheless, the config is not really specific to native apps. Any app can
opt
> to use this config to obtain better performance although with some risk. So it
> is named AggressiveConfiguration instead. 
> The change on Android has been made in 
> https://codereview.webrtc.org/2295493002/

lgtm

[honghaiz3, 2016-08-31 18:42:02]

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

[honghaiz3, 2016-08-31 18:42:02]

Hi, 

I just realize that we also need to change the default config when calling
CreateNativeConfig(). 
Whenever we need to create a pointer to RTCConfiguration, and if we do this with
the static method to return an aggressive config, it will look awkward like
this:
RTCConfiguration *config = new RTCConfiguration();
*config = RTCConfiguration::AggressiveConfiguration(); 

Instead, if we use an argument in the constructor, it will be cleaner like this:

RTCConfiguration *config = new RTCConfiguration(true);
We can even remove the AggressiveConfiguration and SafeConfiguration methods. 
I changed the CL to use a new constructor. Let me know what you think.

[Chuck, 2016-08-31 19:20:38]

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

[Chuck, 2016-08-31 19:20:39]

lgtm

[pthatcher1, 2016-09-01 07:05:44]

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

https://codereview.webrtc.org/2297663004/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface.h:257: }
I can understand why you would want to have a constructor with a parameter
instead of a static method.  But could we at least use an enum instead of a
bool?

Then we could have:

RTCConfiguration(AGGRESSIVE)
or
RTCConfiguration(SAFE)

which is much easier to read than RTCConfiguration(true), and gives more
flexibility in the future.

[tkchin_webrtc, 2016-09-01 12:38:25]

On 2016/09/01 07:05:44, pthatcher1 wrote:
>
https://codereview.webrtc.org/2297663004/diff/20001/webrtc/api/peerconnection...
> File webrtc/api/peerconnectioninterface.h (right):
> 
>
https://codereview.webrtc.org/2297663004/diff/20001/webrtc/api/peerconnection...
> webrtc/api/peerconnectioninterface.h:257: }
> I can understand why you would want to have a constructor with a parameter
> instead of a static method.  But could we at least use an enum instead of a
> bool?
> 
> Then we could have:
> 
> RTCConfiguration(AGGRESSIVE)
> or
> RTCConfiguration(SAFE)
> 
> which is much easier to read than RTCConfiguration(true), and gives more
> flexibility in the future.

+1 on configuration preset enum

[honghaiz3, 2016-09-01 16:07:41]

Patchset #3 (id:40001) has been deleted

[honghaiz3, 2016-09-01 16:07:50]

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

https://codereview.webrtc.org/2297663004/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface.h:257: }
On 2016/09/01 07:05:44, pthatcher1 wrote:
> I can understand why you would want to have a constructor with a parameter
> instead of a static method.  But could we at least use an enum instead of a
> bool?
> 
> Then we could have:
> 
> RTCConfiguration(AGGRESSIVE)
> or
> RTCConfiguration(SAFE)
> 
> which is much easier to read than RTCConfiguration(true), and gives more
> flexibility in the future.  

Done.

[honghaiz3, 2016-09-01 16:07:56]

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

[commit-bot: I haz the power, 2016-09-01 16:08:02]

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

[commit-bot: I haz the power, 2016-09-01 16:11:55]

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

[commit-bot: I haz the power, 2016-09-01 16:11:57]

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

[honghaiz3, 2016-09-01 16:57:51]

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

[commit-bot: I haz the power, 2016-09-01 16:58:03]

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, 2016-09-01 18:58:41]

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

[commit-bot: I haz the power, 2016-09-01 18:58:42]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_clang_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_drmemory_light on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_clang_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_gyp_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[pthatcher1, 2016-09-01 21:25:49]

lgtm

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

https://codereview.webrtc.org/2297663004/diff/80001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface.h:231: // A configuration that is safer to
use, despite it may not have the best
despite it may not have => despite it not having

[honghaiz3, 2016-09-01 22:05:52]

The CQ bit was checked by honghaiz@webrtc.org

[honghaiz3, 2016-09-01 22:05:53]

The patchset sent to the CQ was uploaded after l-g-t-m from tkchin@webrtc.org,
haysc@webrtc.org, pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2297663004/#ps100001 (title:
"Fix a comment")

[commit-bot: I haz the power, 2016-09-01 22:06:03]

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

[honghaiz3, 2016-09-01 22:06:29]

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

https://codereview.webrtc.org/2297663004/diff/80001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface.h:231: // A configuration that is safer to
use, despite it may not have the best
On 2016/09/01 21:25:49, pthatcher1 wrote:
> despite it may not have => despite it not having

Done.

[honghaiz3, 2016-09-01 22:34:15]

Description was changed from

==========
Use AggressiveConfiguration as the default configuration in IOS
==========

to

==========
Use AggressiveConfiguration as the default configuration in IOS

R=haysc@webrtc.org, pthatcher@webrtc.org, tkchin@webrtc.org

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

[commit-bot: I haz the power, 2016-09-01 22:34:16]

Description was changed from

==========
Use AggressiveConfiguration as the default configuration in IOS
==========

to

==========
Use AggressiveConfiguration as the default configuration in IOS

R=haysc@webrtc.org, pthatcher@webrtc.org, tkchin@webrtc.org

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

[honghaiz3, 2016-09-01 22:34:17]

Committed patchset #5 (id:100001) manually as
f7ddc06a43408299b2ae07f6b4a40db2b81670a6 (presubmit successful).

[commit-bot: I haz the power, 2016-09-01 22:34:17]

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