It looks like only very small changes are needed in RTCPeerConnection.cpp to implement this (just copying the pattern of oniceconnectionstatechange).
hbos@: can you take a look?
i have a patch which follows the pattern of onsignalingstatechange (which seemed slightly easier). Only thing I am missing is a test along the lines of https://jsfiddle.net/bgqdskjL/5/ -- where do they live for this area of the code?
There are two categories of tests. LayoutTests: These can test Blink-layer functionality, but use a mock peerconnection. Good for testing that valid parameters pass and invalid parameters throw exceptions and stuff like that, not good for testing actual functionality. https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/peerconnection/?q=LayoutTests+peerconnection&dr BrowserTests: They use real things all the way down, except devices are fake devices. See tests deriving from WebRtcTestBase[1], e.g. WebRtcBrowserTest[2] which set up a call between two tabs. The browser tests open one or two tabs and go to a html[3] which load a bunch of .js files. From the C++ layer (WebRtcTestBase), JavaScript function defined in any of the .js files can be invoked. See for example peerconnection.js[4]. I see there are also other html pages with JavaScript like peerconnection-call.html[5] but I'm not sure from where these things are called. Should be some other browser test. [1] https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/webrtc_browsertest_base.h?sq=package:chromium&dr&l=27 [2] https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/webrtc_browsertest.cc?sq=package:chromium&dr&l=34 [3] https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/webrtc_browsertest.cc?sq=package:chromium&dr&l=22 [4] https://cs.chromium.org/chromium/src/chrome/test/data/webrtc/peerconnection.js?q=peerconnection.js&sq=package:chromium&dr [5] https://cs.chromium.org/chromium/src/content/test/data/media/peerconnection-call.html?q=peerconnection-call.html&sq=package:chromium&dr So either you use mocks and fake that the event happens and verify that the callback is called, or you set up a real browser test (e.g. a call between two tabs) and do real ice state changes.
I'll go for a WebRtcBrowserTest, thanks! Until then I uploaded the patch itself in https://codereview.chromium.org/2677233002/
Cool, I'm happy to review when you've added tests.
hbos: can you give it another look? More eyes :-)
better to test https://jsfiddle.net/bgqdskjL/3/ -- /5/ does an ice restart immediately afterwards which leads to weird behavior in Firefox.
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0a734fd400a160bdc1ea1e2eaff4bc44299d523a commit 0a734fd400a160bdc1ea1e2eaff4bc44299d523a Author: philipp.hancke <philipp.hancke@googlemail.com> Date: Wed Mar 08 19:46:42 2017 Exposes RTCPeerConnection.icegatheringstatechange. Spec: https://w3c.github.io/webrtc-pc/#event-icegatheringstatechange Intent to Implement & Ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/pjwzh5m-gYI Chrome Status feature: https://www.chromestatus.com/feature/5738839242964992 BUG= 681083 Review-Url: https://codereview.chromium.org/2677233002 Cr-Commit-Position: refs/heads/master@{#455518} [modify] https://crrev.com/0a734fd400a160bdc1ea1e2eaff4bc44299d523a/chrome/browser/media/webrtc/webrtc_browsertest.cc [modify] https://crrev.com/0a734fd400a160bdc1ea1e2eaff4bc44299d523a/chrome/test/data/webrtc/peerconnection.js [modify] https://crrev.com/0a734fd400a160bdc1ea1e2eaff4bc44299d523a/third_party/WebKit/LayoutTests/external/wpt/webrtc/rtcpeerconnection/rtcpeerconnection-idl-expected.txt [modify] https://crrev.com/0a734fd400a160bdc1ea1e2eaff4bc44299d523a/third_party/WebKit/LayoutTests/platform/mac/virtual/stable/webexposed/global-interface-listing-expected.txt [modify] https://crrev.com/0a734fd400a160bdc1ea1e2eaff4bc44299d523a/third_party/WebKit/LayoutTests/platform/win/virtual/stable/webexposed/global-interface-listing-expected.txt [modify] https://crrev.com/0a734fd400a160bdc1ea1e2eaff4bc44299d523a/third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt [modify] https://crrev.com/0a734fd400a160bdc1ea1e2eaff4bc44299d523a/third_party/WebKit/Source/core/events/EventTypeNames.json5 [modify] https://crrev.com/0a734fd400a160bdc1ea1e2eaff4bc44299d523a/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp [modify] https://crrev.com/0a734fd400a160bdc1ea1e2eaff4bc44299d523a/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.h [modify] https://crrev.com/0a734fd400a160bdc1ea1e2eaff4bc44299d523a/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl
Comment 1 by guidou@chromium.org
, Jan 19 2017Status: Assigned (was: Untriaged)