Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2677233002:
Expose RTCPeerConnection.icegatheringstatechange. (Closed)
DescriptionExposes 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}
Committed: https://chromium.googlesource.com/chromium/src/+/0a734fd400a160bdc1ea1e2eaff4bc44299d523a
Patch Set 1 #Patch Set 2 : add onicegatheringstatechange test #
Total comments: 5
Patch Set 3 : codereview feedback #
Total comments: 1
Patch Set 4 : fix virtual expectations #Patch Set 5 : fix virtual expectations #Patch Set 6 : dont remove oniceconnectionstatechange #Patch Set 7 : fix number of passing tests #Messages
Total messages: 47 (27 generated)
Description was changed from ========== expose icegatheringstatechange exposes icegatheringstatechange BUG=681083 ========== to ========== expose icegatheringstatechange exposes icegatheringstatechange BUG=681083 ==========
philipp.hancke@googlemail.com changed reviewers: + hbos@chromium.org
The CQ bit was checked by hbos@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2677233002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp (right): https://codereview.chromium.org/2677233002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:1392: if (m_iceConnectionState != ICEConnectionStateClosed && 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
https://codereview.chromium.org/2677233002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp (right): https://codereview.chromium.org/2677233002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:1392: if (m_iceConnectionState != ICEConnectionStateClosed && On 2017/02/08 12:21:24, hbos_chromium wrote: > 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. SGTM -- change the state but not emit an event. > 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? That is done in https://codesearch.chromium.org/chromium/src/content/renderer/media/rtc_peer_... > > [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
https://codereview.chromium.org/2677233002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp (right): https://codereview.chromium.org/2677233002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:1392: if (m_iceConnectionState != ICEConnectionStateClosed && 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2017/02/08 12:50:35, fippo wrote: > https://codereview.chromium.org/2677233002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp > (right): > > https://codereview.chromium.org/2677233002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:1392: if > (m_iceConnectionState != ICEConnectionStateClosed && > 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... The spec rule of thumb is that "when you call close(), the PC stops wriggling". That means no more state changes in the future (there may be some as the result of the close() call itself; a number of changes in state are listed there). Bernard went over the spec and inserted "if closed, do nothing" in quite a number of places after we had a discussion about it. I don't think we need to change the state at close(). Now to take a look at the code....
hta@chromium.org changed reviewers: + hta@chromium.org
Looks good, wonder why we didn't do that before :-) I always ask for more tests - can you add checking that the event is fired to the layout test clone or expand https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/peer... ? (recommending that because it's a testharness.js based test, which is the New Hotness). According to the New Ideal, this should really land in https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/... - which accepts testharness.js-driven tests and is two-way synchronized automagically with the W3C WPT repository. And just to make your life more bureaucratic - have you onsidered whether we need to post an Intent to Implement and Ship on this feature? If you decide "nah, it's too small" (which I think is reasonable), sending a note to blink-dev saying that you're planning to land this would be a Good Thing anyway. https://codereview.chromium.org/2677233002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp (right): https://codereview.chromium.org/2677233002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:1395: scheduleDispatchEvent(Event::create(EventTypeNames::icegatheringstatechange)); Remember to run "git cl format" in your client and submit the result - that parenthesis looks a little out of touch.
On 2017/02/08 13:35:22, hta - Chromium wrote: > Looks good, wonder why we didn't do that before :-) > > I always ask for more tests - can you add checking that the event is fired to > the layout test clone or expand > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/peer... > > ? (recommending that because it's a testharness.js based test, which is the New > Hotness). > > According to the New Ideal, this should really land in > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/... > - which accepts testharness.js-driven tests and is two-way synchronized > automagically with the W3C WPT repository. Ideally yes, but it's a LayoutTest, so (I think) it uses a mock peer connection handler. https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebRT... So I think those tests rely on this happening when doing addICECandidate: https://cs.chromium.org/chromium/src/components/test_runner/mock_webrtc_peer_... I'm not sure if an event such as icegatheringstatechange is as easily mockable, and we probably want a non-mock based test anyway. Not in this CL - I think we should consider getting rid of the mocking so that we can utilize LayoutTests for everything...? https://codereview.chromium.org/2677233002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp (right): https://codereview.chromium.org/2677233002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:1392: if (m_iceConnectionState != ICEConnectionStateClosed && On 2017/02/08 12:50:35, fippo wrote: > 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... 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.
On 2017/02/08 13:22:30, hta - Chromium wrote: > I don't think we need to change the state at close(). I agree.
On 2017/02/08 13:54:07, hbos_chromium wrote: > On 2017/02/08 13:22:30, hta - Chromium wrote: > > I don't think we need to change the state at close(). > > I agree. Yeah. The only important transition here is the one to complete. Well, if you don't do trickle-ice at least. Removed the state change from close()
The CQ bit was checked by hta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
To run what the bots are running and failing: Build blink_tests (example ninja -C out/gn blink_tests) third_party/WebKit/Tools/Scripts/run-webkit-tests -t gn virtual/stable/webexposed/global-interface-listing.html (or replace "gn" with name of your out/foo-directory) To make the test pass run-webkit-tests with --reset-results flag to replace the -expected.txt file. On 2017/02/08 20:45:21, fippo wrote: > On 2017/02/08 13:54:07, hbos_chromium wrote: > > On 2017/02/08 13:22:30, hta - Chromium wrote: > > > I don't think we need to change the state at close(). > > > > I agree. > > Yeah. The only important transition here is the one to complete. Well, if you > don't do trickle-ice at least. > Removed the state change from close() This made me think of this discussion, "pc.close() is always user-invoked now, and by design we don't fire events on user-invoked actions." https://github.com/w3c/webrtc-pc/issues/1020
lgtm https://codereview.chromium.org/2677233002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/webrtc/rtcpeerconnection/rtcpeerconnection-idl-expected.txt (right): https://codereview.chromium.org/2677233002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/webrtc/rtcpeerconnection/rtcpeerconnection-idl-expected.txt:106: FAIL RTCPeerConnection interface: pc must inherit property "onicegatheringstatechange" with the proper type (22) Unrecognized type EventHandler I *think* these errors mean that the test is somehow broken in that it doesn't define EventHandler in its IDL imports. Seems like a separable problem; don't worry about it here.
The CQ bit was checked by philipp.hancke@googlemail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by philipp.hancke@googlemail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hbos@chromium.org
lgtm
The patchset sent to the CQ was uploaded after l-g-t-m from hta@chromium.org Link to the patchset: https://codereview.chromium.org/2677233002/#ps120001 (title: "fix number of passing tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2017/03/08 10:20:38, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) This will need a review from an API owner since it's a web-exposed change. Can we add a link to an intent-to-ship to the CL description?
hbos@chromium.org changed reviewers: + jochen@chromium.org
Please take a look jochen for owning: third_party/WebKit/LayoutTests/platform/win/virtual/stable/webexposed/global-interface-listing-expected.txt third_party/WebKit/Source/core/events/EventTypeNames.json5
Description was changed from ========== expose icegatheringstatechange exposes icegatheringstatechange BUG=681083 ========== to ========== 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 ==========
On 2017/03/08 10:23:10, haraken wrote: > On 2017/03/08 10:20:38, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > This will need a review from an API owner since it's a web-exposed change. Can > we add a link to an intent-to-ship to the CL description? Done.
lgtm
The CQ bit was checked by hbos@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1489000218869070,
"parent_rev": "2c6d2707d8ea850c862f04ac066724273981e88f", "commit_rev":
"0a734fd400a160bdc1ea1e2eaff4bc44299d523a"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} Committed: https://chromium.googlesource.com/chromium/src/+/0a734fd400a160bdc1ea1e2eaff4... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/0a734fd400a160bdc1ea1e2eaff4... |
