Expose RTCPeerConnection.icegatheringstatechange.

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

 /*
  * Copyright (C) 2012 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  *
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 1371 matching lines @@
 void RTCPeerConnection::changeSignalingState(SignalingState signalingState) {
   if (m_signalingState != SignalingStateClosed &&
       m_signalingState != signalingState) {
     m_signalingState = signalingState;
     scheduleDispatchEvent(Event::create(EventTypeNames::signalingstatechange));
   }
 }
 
 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(
     ICEConnectionState iceConnectionState) {
   if (m_iceConnectionState != ICEConnectionStateClosed &&
       m_iceConnectionState != iceConnectionState) {
     m_iceConnectionState = iceConnectionState;
     if (m_iceConnectionState == ICEConnectionStateConnected)
       recordRapporMetrics();
 
@@ skipping 83 matching lines @@
 DEFINE_TRACE(RTCPeerConnection) {
   visitor->trace(m_localStreams);
   visitor->trace(m_remoteStreams);
   visitor->trace(m_dispatchScheduledEventRunner);
   visitor->trace(m_scheduledEvents);
   EventTargetWithInlineData::trace(visitor);
   SuspendableObject::trace(visitor);
 }
 
 }  // namespace blink