New issue
Advanced search Search tips

Issue 681083 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Implement RTCPeerConnection.onicegatheringstatechange

Project Member Reported by deadbeef@chromium.org, Jan 13 2017

Issue description

It looks like only very small changes are needed in RTCPeerConnection.cpp to implement this (just copying the pattern of oniceconnectionstatechange).
 

Comment 1 by guidou@chromium.org, Jan 19 2017

Owner: hbos@chromium.org
Status: Assigned (was: Untriaged)
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?

Comment 3 by hbos@chromium.org, Feb 6 2017

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.

Comment 4 by fi...@appear.in, Feb 6 2017

I'll go for a WebRtcBrowserTest, thanks!

Until then I uploaded the patch itself in https://codereview.chromium.org/2677233002/

Comment 5 by hbos@chromium.org, Feb 6 2017

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.
Project Member

Comment 8 by bugdroid1@chromium.org, Mar 8 2017

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 9 by hbos@chromium.org, Mar 21 2017

Status: Fixed (was: Assigned)

Sign in to add a comment