Expose RTCPeerConnection.icegatheringstatechange.

third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp

Index: third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
diff --git a/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp b/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
index 85cb675df52591ff30a073a66e2606b8d9868286..f84414a052ad188a1a669ac02d6b50053bb9ba16 100644
--- a/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
+++ b/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
@@ -1389,7 +1389,11 @@ void RTCPeerConnection::changeSignalingState(SignalingState signalingState) {
 
 void RTCPeerConnection::changeIceGatheringState(
     ICEGatheringState iceGatheringState) {
-  m_iceGatheringState = iceGatheringState;
+  if (m_iceConnectionState != ICEConnectionStateClosed &&

[hbos_chromium, 2017/02/08 12:21:24]

To me it's not clear what the ICE agent[1] should do when the peer connection
closes and the ICE agent is destroyed,[2] "abruptly ending any active ICE
processing and any active streaming, and releasing any relevant resources".

None of the states "new", "gathering" or "complete"[3] seem to apply to the case
of closing where all RTCIceTransport's are set to "closed". Perhaps we should
file a spec bug to clarify?

This code previously set the state to "complete" but this if-statement would
ignore that when closing. I think if close should no longer change the state we
should change RTCPeerConnection::closeInternal() to not invoke
changeIceGatheringState() rather than modify changeIceGatheringState() to
silently ignore state changes after closed.

If changing the event to is unexpected and shouldn't happen while already
closed, we can DCHECK instead, exposing bugs if this does happen.

If we do change the state to "completed", the spec says to "fire an event named
icecandidate with null at connection". I wonder if we should invoke
RTCPeerConnection::didGenerateICECandidate with a null candidate here in that
case or if that would cause it to get fired twice in other cases? Can you look
into this or add a TODO?

[1] https://w3c.github.io/webrtc-pc/#dfn-ice-agent
[2] https://w3c.github.io/webrtc-pc/#dom-rtcpeerconnection-close
[3] https://w3c.github.io/webrtc-pc/#idl-def-rtcicegatheringstate

[fippo, 2017/02/08 12:46:03]

SGTM -- change the state but not emit an event.
That is done in
https://codesearch.chromium.org/chromium/src/content/renderer/media/rtc_peer_...

[fippo, 2017/02/08 12:50:35]

https://w3c.github.io/webrtc-pc/#operation actually says:

To update the ICE gathering state of an RTCPeerConnection instance connection,
the User Agent must queue a task that runs the following steps:
  If connection's [[isClosed]] slot is true, abort these steps.

Which means the code is doing what the spec says. But it does seem weird...

[hbos_chromium, 2017/02/08 13:52:54]

Setting [[isClosed]] to false is the last step of close() (step 7), steps 3. and
4. which destroy the ICE Agent and set the state of each RTCIceTransport to
closed occur before then, so I wonder if "abort these steps" applies here.

+      m_iceGatheringState != iceGatheringState) {
+    m_iceGatheringState = iceGatheringState;
+    scheduleDispatchEvent(Event::create(EventTypeNames::icegatheringstatechange));

[hta - Chromium, 2017/02/08 13:35:21]

Remember to run "git cl format" in your client and submit the result - that
parenthesis looks a little out of touch.

+  }
 }
 
 bool RTCPeerConnection::setIceConnectionState(