webrtc-internals: filter some events after close [chromium/src : main]
Philipp Hancke (Gerrit)
Attention is currently required from: Henrik Boström.
Philipp Hancke would like Henrik Boström to review this change.
webrtc-internals: filter some events after close
for consistency with how the JS behaves
BUG=chromium:1124234
Change-Id: Ib5fc2b726ad81ed00cd59af2afc75fb27f85e2c2
---
M third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler.cc
M third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler_test.cc
2 files changed, 55 insertions(+), 5 deletions(-)
To view, visit change 3883820. To unsubscribe, or for help writing mail filters, visit settings.
Philipp Hancke (Gerrit)
Attention is currently required from: Henrik Boström.
1 comment:
Patchset:
any idea how to get rid of the low coverage warning? It is not quite clear to me where that originates from :-(
To view, visit change 3883820. To unsubscribe, or for help writing mail filters, visit settings.
Henrik Boström (Gerrit)
Attention is currently required from: Harald Alvestrand, Philipp Hancke.
Henrik Boström would like Harald Alvestrand to review this change authored by Philipp Hancke.
webrtc-internals: filter some events after close
for consistency with how the JS behaves
BUG=chromium:1124234
Change-Id: Ib5fc2b726ad81ed00cd59af2afc75fb27f85e2c2
---
M third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler.cc
M third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler_test.cc
2 files changed, 55 insertions(+), 5 deletions(-)
To view, visit change 3883820. To unsubscribe, or for help writing mail filters, visit settings.
Henrik Boström (Gerrit)
Attention is currently required from: Harald Alvestrand, Philipp Hancke.
2 comments:
Patchset:
+Harald for second opinion on "close"
File third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler.cc:
Patch Set #7, Line 2107:
TRACE_EVENT0("webrtc", "RTCPeerConnectionHandler::TrackSignalingChange");pc.onsignalingstatechange does not fire on close(), the philosophy is that if it did not happen asynchronously then there is no need to fire an event; the caller knows we are closing and can act accordingly without the event handler.
Whether or not you like that philosophy, one could argue that because the chrome://webrtc-internals/ page is not the person that performed pc.close(), having "TrackSignalingChange" expose the fact that the peer connection did close is still relevant information, and there is no need to pretend that "TrackSignalingStateChange" is the same thing as pc.onsignalingstatechange.
With this CL, it is no longer possible to tell if/when the pc closed? Other than stats no longer updating I guess.
That being said, I don't have a strong preference whether to include or exclude this event on close...
To view, visit change 3883820. To unsubscribe, or for help writing mail filters, visit settings.
Philipp Hancke (Gerrit)
Attention is currently required from: Harald Alvestrand, Henrik Boström.
1 comment:
File third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler.cc:
Patch Set #7, Line 2107:
TRACE_EVENT0("webrtc", "RTCPeerConnectionHandler::TrackSignalingChange");
pc. […]
close() is still traced. I thought signaling state change was still emitted because of the "lid close" hack relying on it? See https://bugs.chromium.org/p/chromium/issues/detail?id=699036
To view, visit change 3883820. To unsubscribe, or for help writing mail filters, visit settings.
Henrik Boström (Gerrit)
Attention is currently required from: Harald Alvestrand, Philipp Hancke.
1 comment:
File third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler.cc:
Patch Set #7, Line 2107:
TRACE_EVENT0("webrtc", "RTCPeerConnectionHandler::TrackSignalingChange");
close() is still traced. […]
We don't fire the onsignalingstatechange JS event on pc.close(), but AFAIK we do still fire if "close" happens asynchronously (e.g. due to lid close).
When I check webrtc-internals on Chrome stable then I already don't see any webrtc-internals event on explicit pc.close(), however I am still able to tell the PC closed due to connection state: "Connection state: new => closed".
Uhm, so does that mean that this change here really only has a change in behavior for webrtc-internals when laptop lid close happens?
To view, visit change 3883820. To unsubscribe, or for help writing mail filters, visit settings.
Henrik Boström (Gerrit)
File third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler.cc:
Patch Set #7, Line 2107:
TRACE_EVENT0("webrtc", "RTCPeerConnectionHandler::TrackSignalingChange");
We don't fire the onsignalingstatechange JS event on pc. […]
TLDR:
- ICE connection state is already suppressed for both JS and webrtc-internals when pc.close() is called.
- But not if closed due to async event (lid close? context destroyed maybe?).
Today the reliable way to tell if PC closed in webrtc-internals is with the connection state change, not the iceConnectionState, and this CL does not appear to change that.
To view, visit change 3883820. To unsubscribe, or for help writing mail filters, visit settings.
Henrik Boström (Gerrit)
File third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler.cc:
Patch Set #7, Line 2107:
TRACE_EVENT0("webrtc", "RTCPeerConnectionHandler::TrackSignalingChange");
TLDR: […]
No idea if lid affects both iceConnectionState and connectionState or just one of the two though
To view, visit change 3883820. To unsubscribe, or for help writing mail filters, visit settings.
Philipp Hancke (Gerrit)
Attention is currently required from: Harald Alvestrand, Henrik Boström.
1 comment:
File third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler.cc:
Patch Set #7, Line 2107:
TRACE_EVENT0("webrtc", "RTCPeerConnectionHandler::TrackSignalingChange");
No idea if lid affects both iceConnectionState and connectionState or just one of the two though
well, if you close the lid how do you view webrtc-internals? :-)
To view, visit change 3883820. To unsubscribe, or for help writing mail filters, visit settings.
Henrik Boström (Gerrit)
Attention is currently required from: Harald Alvestrand, Philipp Hancke.
1 comment:
File third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler.cc:
Patch Set #7, Line 2107:
TRACE_EVENT0("webrtc", "RTCPeerConnectionHandler::TrackSignalingChange");
well, if you close the lid how do you view webrtc-internals? :-)
you open the lid again :)
To view, visit change 3883820. To unsubscribe, or for help writing mail filters, visit settings.
Harald Alvestrand (Gerrit)
Attention is currently required from: Philipp Hancke.
1 comment:
Patchset:
will rubberstamp once hbos is OK with this.
To view, visit change 3883820. To unsubscribe, or for help writing mail filters, visit settings.
Henrik Boström (Gerrit)
Attention is currently required from: Philipp Hancke.
Patch set 7:Code-Review +1
1 comment:
File third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler.cc:
Patch Set #7, Line 2107:
TRACE_EVENT0("webrtc", "RTCPeerConnectionHandler::TrackSignalingChange");
you open the lid again :)
I think we can still tell if close happened so I'll let you decide, but if you have the ability to test lid closing, do check if webrtc-internals is updated accordingly (check that there is info about it being closed after you open the lid again)
To view, visit change 3883820. To unsubscribe, or for help writing mail filters, visit settings.