Enable generation of local host candidate.

Enable generation of local host candidate.

When iceServers is not specified or set as undefined, treat it as a special case that we'll try to generate local host candidate if adapter enumeration is determined as disabled in webrtc stack.

iceServers:null is still treated as malformed javascript which is consistent with FF.

iceServers:[] will not generate local host candidate.
missing iceServers or the whole RTCConfiguration will be treated as signal to generate local host candidate.

BUG=webrtc:4517
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=202399

[guoweis_left_chromium, 2015-08-27 22:02:18]

Patchset #1 (id:1) has been deleted

[guoweis_left_chromium, 2015-08-27 22:04:54]

guoweis@chromium.org changed reviewers:
+ juberti@chromium.org, pthatcher@chromium.org

[guoweis_left_chromium, 2015-08-27 22:06:59]

Patchset #1 (id:20001) has been deleted

[guoweis_left_chromium, 2015-08-27 22:07:36]

On 2015/08/27 22:06:59, guoweis_chromium wrote:
> Patchset #1 (id:20001) has been deleted

PTAL.

[juberti, 2015-08-28 00:34:58]

juberti@google.com changed reviewers:
+ juberti@google.com

[juberti, 2015-08-28 00:34:59]

https://codereview.chromium.org/1315413004/diff/40001/Source/platform/mediast...
File Source/platform/mediastream/RTCConfiguration.h (left):

https://codereview.chromium.org/1315413004/diff/40001/Source/platform/mediast...
Source/platform/mediastream/RTCConfiguration.h:107:
HeapVector<Member<RTCIceServer>> m_servers;
Could this be a pointer instead? Then, if undefined, it could be null.

[guoweis_left_chromium, 2015-08-28 04:53:58]

https://codereview.chromium.org/1315413004/diff/40001/Source/platform/mediast...
File Source/platform/mediastream/RTCConfiguration.h (left):

https://codereview.chromium.org/1315413004/diff/40001/Source/platform/mediast...
Source/platform/mediastream/RTCConfiguration.h:107:
HeapVector<Member<RTCIceServer>> m_servers;
On 2015/08/28 00:34:58, juberti wrote:
> Could this be a pointer instead? Then, if undefined, it could be null.

since we're not going to expose m_servers public, this doesn't remove the need
of having an read accessor enableLocalhostCandidate(). This does bring
complexity into appendServer(), numberOfServers(), server() and inline_trace as
they need to handle the nullptr case. What's the right tradeoff?

[guoweis_left_chromium, 2015-08-31 20:05:21]

guoweis@chromium.org changed reviewers:
- juberti@chromium.org, juberti@google.com, pthatcher@chromium.org

[guoweis_left_chromium, 2015-08-31 20:09:23]

guoweis@chromium.org changed reviewers:
+ juberti@chromium.org, pthatcher@chromium.org, tommi@chromium.org

[guoweis_left_chromium, 2015-08-31 20:09:24]

+tommi for this.

Tommi, is there any generic, standard way in WebKits such that the upper layer
(in rtc_peer_connection_handler.cc) could tell whether an array is [] or
undefined? 

The concern is that enableLocalhostCandidate() is too specific a concept in
WebKits - and the complicated parsing code in
Source/modules/mediastream/RTCPeerConnection.cpp

Want to know your thought.

[guoweis_left_chromium, 2015-09-09 23:53:29]

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

[commit-bot: I haz the power, 2015-09-09 23:53:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1315413004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1315413004/60001

[commit-bot: I haz the power, 2015-09-09 23:53:52]

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

[commit-bot: I haz the power, 2015-09-09 23:53:53]

Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are
accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[guoweis_left_chromium, 2015-09-10 03:27:18]

Patchset #2 (id:60001) has been deleted

[guoweis_left_chromium, 2015-09-10 04:00:59]

On 2015/09/09 23:53:53, commit-bot: I haz the power wrote:
> Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are
> accepted.
> Even if an L-G-T-M may have been provided, it was from a non-committer,
> _not_ a full super star committer.
> See http://www.chromium.org/getting-involved/become-a-committer
> Note that this has nothing to do with OWNERS files.

PTAL.

[guoweis_left_chromium, 2015-09-10 04:02:43]

guoweis@chromium.org changed reviewers:
- tommi@chromium.org

[guoweis_left_chromium, 2015-09-10 04:06:00]

guoweis@chromium.org changed reviewers:
+ adamk@chromium.org, dstockwell@chromium.org, tommi@chromium.org

[guoweis_left_chromium, 2015-09-10 04:06:00]

adamk@chromium.org - please review Source/Platform
tommi@chromium.org - please review Source/Modules
dstockwell@chromium.org - please review public/Platform

[guoweis_left_chromium, 2015-09-10 04:27:22]

On 2015/09/10 04:06:00, guoweis_chromium wrote:
> mailto:adamk@chromium.org - please review Source/Platform
> mailto:tommi@chromium.org - please review Source/Modules
> mailto:dstockwell@chromium.org - please review public/Platform

Ok. I missed that there is some failure in
LayoutTests/fast/mediastream/RTCPeerConnection.html. I'll fix and get back to
you.

[guoweis_left_chromium, 2015-09-10 22:05:30]

On 2015/09/10 04:27:22, guoweis_chromium wrote:
> On 2015/09/10 04:06:00, guoweis_chromium wrote:
> > mailto:adamk@chromium.org - please review Source/Platform
> > mailto:tommi@chromium.org - please review Source/Modules
> > mailto:dstockwell@chromium.org - please review public/Platform
> 
> Ok. I missed that there is some failure in
> LayoutTests/fast/mediastream/RTCPeerConnection.html. I'll fix and get back to
> you.

RTCPeerConnection.html test passed. PTAL.

[guoweis_left_chromium, 2015-09-14 19:26:10]

Ping.

[dstockwell, 2015-09-16 00:52:14]

public lgtm

[pthatcher2, 2015-09-16 04:09:58]

https://codereview.chromium.org/1315413004/diff/120001/LayoutTests/fast/media...
File LayoutTests/fast/mediastream/RTCPeerConnection.html (right):

https://codereview.chromium.org/1315413004/diff/120001/LayoutTests/fast/media...
LayoutTests/fast/mediastream/RTCPeerConnection.html:24: shouldNotThrow("new
webkitRTCPeerConnection({fooServers:[], iceServers:[{urls:['stun:foo.com',
'turn:foo.com']}]}, null);");
Why bother having fooServers?

https://codereview.chromium.org/1315413004/diff/120001/LayoutTests/fast/media...
LayoutTests/fast/mediastream/RTCPeerConnection.html:29: shouldNotThrow("new
webkitRTCPeerConnection({fooServers:[]}, null);");
What does this test buy us over {}?  In other words, why bother having
fooServers?

https://codereview.chromium.org/1315413004/diff/120001/Source/modules/mediast...
File Source/modules/mediastream/RTCPeerConnection.cpp (right):

https://codereview.chromium.org/1315413004/diff/120001/Source/modules/mediast...
Source/modules/mediastream/RTCPeerConnection.cpp:128: }
Can you put the early returns first?

if (DictionaryHelper::get(configuration, "iceServers", iceServers)) {
  ok = iceServers.length(numberOfServers)
  if (!ok) {
    exceptionState.throwTypeError("Malformed RTCConfiguration");
    return 0;
  )
} else {
  v8::Local<v8::Value> iceServersValue;
  ok = (!DictionaryHelper::get(configuration, "iceServers", iceServersValue)
           || iceServersValue->IsUndefined()); 
  if (!ok) {
    exceptionState.throwTypeError("Malformed RTCConfiguration");
    return 0;    
  }
  createIceServerArray = false;
}

https://codereview.chromium.org/1315413004/diff/120001/Source/platform/export...
File Source/platform/exported/WebRTCConfiguration.cpp (right):

https://codereview.chromium.org/1315413004/diff/120001/Source/platform/export...
Source/platform/exported/WebRTCConfiguration.cpp:116: // TODO(guoweis): Remove
this function.
When?

https://codereview.chromium.org/1315413004/diff/120001/public/platform/WebRTC...
File public/platform/WebRTCConfiguration.h (right):

https://codereview.chromium.org/1315413004/diff/120001/public/platform/WebRTC...
public/platform/WebRTCConfiguration.h:136: // TODO(guoweis): Remove the next 2
functions.
When?

[tommi (sloooow) - chröme, 2015-09-16 06:19:27]

https://codereview.chromium.org/1315413004/diff/120001/Source/modules/mediast...
File Source/modules/mediastream/RTCPeerConnection.cpp (right):

https://codereview.chromium.org/1315413004/diff/120001/Source/modules/mediast...
Source/modules/mediastream/RTCPeerConnection.cpp:124: if (!ok) {
do we need to handle numberOfServers == 0? (looks like that wasn't done before,
so this might be OK as is, but I'm wondering if you need both numberOfServers
and createIceServerArray or if one of them is enough)

[guoweis_left_chromium, 2015-09-16 17:28:47]

Patchset #5 (id:140001) has been deleted

[guoweis_left_chromium, 2015-09-16 17:29:05]

PTAL.

https://codereview.chromium.org/1315413004/diff/120001/LayoutTests/fast/media...
File LayoutTests/fast/mediastream/RTCPeerConnection.html (right):

https://codereview.chromium.org/1315413004/diff/120001/LayoutTests/fast/media...
LayoutTests/fast/mediastream/RTCPeerConnection.html:24: shouldNotThrow("new
webkitRTCPeerConnection({fooServers:[], iceServers:[{urls:['stun:foo.com',
'turn:foo.com']}]}, null);");
On 2015/09/16 04:09:58, pthatcher2 wrote:
> Why bother having fooServers?

There are existing tests using "fooServers" which expects exception. The reason
for that was iceServers was missing. "iceServers" was mandate but not anymore.

https://codereview.chromium.org/1315413004/diff/120001/LayoutTests/fast/media...
LayoutTests/fast/mediastream/RTCPeerConnection.html:29: shouldNotThrow("new
webkitRTCPeerConnection({fooServers:[]}, null);");
On 2015/09/16 04:09:58, pthatcher2 wrote:
> What does this test buy us over {}?  In other words, why bother having
> fooServers?

Done.

https://codereview.chromium.org/1315413004/diff/120001/Source/modules/mediast...
File Source/modules/mediastream/RTCPeerConnection.cpp (right):

https://codereview.chromium.org/1315413004/diff/120001/Source/modules/mediast...
Source/modules/mediastream/RTCPeerConnection.cpp:124: if (!ok) {
On 2015/09/16 06:19:27, tommi wrote:
> do we need to handle numberOfServers == 0? (looks like that wasn't done
before,
> so this might be OK as is, but I'm wondering if you need both numberOfServers
> and createIceServerArray or if one of them is enough)

numberOfServers == 0, i.e. iceServers:[], is treated as a way to not cause local
host candidate generated.

https://codereview.chromium.org/1315413004/diff/120001/Source/modules/mediast...
Source/modules/mediastream/RTCPeerConnection.cpp:128: }
On 2015/09/16 04:09:58, pthatcher2 wrote:
> Can you put the early returns first?
> 
> if (DictionaryHelper::get(configuration, "iceServers", iceServers)) {
>   ok = iceServers.length(numberOfServers)
>   if (!ok) {
>     exceptionState.throwTypeError("Malformed RTCConfiguration");
>     return 0;
>   )
> } else {
>   v8::Local<v8::Value> iceServersValue;
>   ok = (!DictionaryHelper::get(configuration, "iceServers", iceServersValue)
>            || iceServersValue->IsUndefined()); 
>   if (!ok) {
>     exceptionState.throwTypeError("Malformed RTCConfiguration");
>     return 0;    
>   }
>   createIceServerArray = false;
> }

Done.

[tommi (sloooow) - chröme, 2015-09-16 18:19:45]

lgtm.  minor questions below, but I'm fine either way. Just want to make sure
creating an empty array is intended.

https://codereview.chromium.org/1315413004/diff/120001/Source/modules/mediast...
File Source/modules/mediastream/RTCPeerConnection.cpp (right):

https://codereview.chromium.org/1315413004/diff/120001/Source/modules/mediast...
Source/modules/mediastream/RTCPeerConnection.cpp:124: if (!ok) {
On 2015/09/16 17:29:05, guoweis_chromium wrote:
> On 2015/09/16 06:19:27, tommi wrote:
> > do we need to handle numberOfServers == 0? (looks like that wasn't done
> before,
> > so this might be OK as is, but I'm wondering if you need both
numberOfServers
> > and createIceServerArray or if one of them is enough)
> 
> numberOfServers == 0, i.e. iceServers:[], is treated as a way to not cause
local
> host candidate generated.

ok, but whenever numberOfServers is 0, won't (or should) createIceServerArray be
false?  (and when it's != 0, then createIceServerArray will be true).  Not a big
issue, I'm just wondering if we need two variables for this.

https://codereview.chromium.org/1315413004/diff/180001/Source/modules/mediast...
File Source/modules/mediastream/RTCPeerConnection.cpp (right):

https://codereview.chromium.org/1315413004/diff/180001/Source/modules/mediast...
Source/modules/mediastream/RTCPeerConnection.cpp:112: }
do we need to check for numberOfServers==0 or do we want to create the server
array even if it's empty?

[adamk, 2015-09-16 20:50:44]

lgtm for Source/platform

[guoweis_left_chromium, 2015-09-16 22:10:20]

landing this one now.

https://codereview.chromium.org/1315413004/diff/120001/Source/modules/mediast...
File Source/modules/mediastream/RTCPeerConnection.cpp (right):

https://codereview.chromium.org/1315413004/diff/120001/Source/modules/mediast...
Source/modules/mediastream/RTCPeerConnection.cpp:124: if (!ok) {
On 2015/09/16 18:19:45, tommi wrote:
> On 2015/09/16 17:29:05, guoweis_chromium wrote:
> > On 2015/09/16 06:19:27, tommi wrote:
> > > do we need to handle numberOfServers == 0? (looks like that wasn't done
> > before,
> > > so this might be OK as is, but I'm wondering if you need both
> numberOfServers
> > > and createIceServerArray or if one of them is enough)
> > 
> > numberOfServers == 0, i.e. iceServers:[], is treated as a way to not cause
> local
> > host candidate generated.
> 
> ok, but whenever numberOfServers is 0, won't (or should) createIceServerArray
be
> false?  (and when it's != 0, then createIceServerArray will be true).  Not a
big
> issue, I'm just wondering if we need two variables for this.

when there is no iceServers specified (or undefined), we want
createiceServerArray to be false. When there is iceServers, we want it to be
true. However, when we do have iceServers but the length is 0, (iceServers:[]),
we still want createiceServerArray to true. I think we still need 2 variables.

https://codereview.chromium.org/1315413004/diff/180001/Source/modules/mediast...
File Source/modules/mediastream/RTCPeerConnection.cpp (right):

https://codereview.chromium.org/1315413004/diff/180001/Source/modules/mediast...
Source/modules/mediastream/RTCPeerConnection.cpp:112: }
On 2015/09/16 18:19:45, tommi wrote:
> do we need to check for numberOfServers==0 or do we want to create the server
> array even if it's empty?

for iceServers:[] case, we do want to create the server array, i.e. to signal
webrtc NOT to create local host candidate.

[guoweis_left_chromium, 2015-09-16 22:10:29]

The CQ bit was checked by guoweis@chromium.org

[guoweis_left_chromium, 2015-09-16 22:10:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
dstockwell@chromium.org
Link to the patchset: https://codereview.chromium.org/1315413004/#ps180001
(title: " ")

[commit-bot: I haz the power, 2015-09-16 22:10:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1315413004/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1315413004/180001

[commit-bot: I haz the power, 2015-09-16 23:44:38]

Committed patchset #6 (id:180001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=202399