1014304 - Remove onconnection and onclosedconnection from PC

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Nils Ohlmeier [:drno]]

According to this 
http://dxr.mozilla.org/mozilla-central/source/dom/webidl/RTCPeerConnection.webidl#142
our RTCPeerConnection has events for 'onconnection' and 'onclosedconnection'. But these events are nowhere to be found in the standard 
http://dev.w3.org/2011/webrtc/editor/webrtc.html
and should therefore be removed.

Comment 1

[Nils Ohlmeier [:drno]]

Attachment: remove_onconnection_event.patch

This builds cleanly and passes tests (which is not surprising as the tests don't cover these at all).
But I'm not sure if there are more implementation parts which need to get removed (I was not able to locate any).

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8426729 [details] [diff] [review]
remove_onconnection_event.patch

Review of attachment 8426729 [details] [diff] [review]:
-----------------------------------------------------------------

r+ provided you remove this code as well http://mxr.mozilla.org/mozilla-central/source/dom/media/PeerConnection.js#310

Comment 3

[Nils Ohlmeier [:drno]]

Attachment: remove_onconnection_event.patch

Added removal of 'onconnection' and 'onclosedconnection' getter/setter from PeerConnection.js according to :jib's hint.

Carrying forward r+=jib

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8426849 [details] [diff] [review]
remove_onconnection_event.patch

Review of attachment 8426849 [details] [diff] [review]:
-----------------------------------------------------------------

Touched webidl, so you need a DOM reviewer

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

But we dispatch those events.
Shouldn't we remove http://mxr.mozilla.org/mozilla-central/source/dom/media/PeerConnection.js?rev=50116088c244&mark=1285-1285,1289-1289#1284 ?
The spec doesn't talk about those events at all.

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Olli Pettay [:smaug] from comment #5)
> But we dispatch those events. Shouldn't we remove

Yes there is more plumbing to pull out here. http://mxr.mozilla.org/mozilla-central/search?string=notifyClosedConnection and the same for notifyConnection - Thanks for spotting it!

drno, let me know if you need help.

Comment 7

[Nils Ohlmeier [:drno]]

Thanks for finding this. I overlooked it because I was only looking for onclosedconnection.
I'll go through the remaining plumbing, remove it and submit another patch later.

Comment 8

[Nils Ohlmeier [:drno]]

Attachment: remove_onconnection_event.patch

This should hopefully remove all implementation and unit testing code.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=5bd72913cc86

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8429583 [details] [diff] [review]
remove_onconnection_event.patch

Since the .webidl changes need a dom peer review, r=smaug for those.

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8429583 [details] [diff] [review]
remove_onconnection_event.patch

Review of attachment 8429583 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nit.

::: netwerk/sctp/datachannel/DataChannel.h
@@ +474,5 @@
>  
>    // for ON_CONNECTION/ON_DISCONNECTED
>    DataChannelOnMessageAvailable(int32_t     aType,
>                                  DataChannelConnection *aConnection,
>                                  bool aResult = true)

aResult can go away too, I think.

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8429583 [details] [diff] [review]
remove_onconnection_event.patch

Review of attachment 8429583 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/sctp/datachannel/DataChannel.h
@@ -540,5 @@
> -            // FIX - on mResult false (failure) we should do something.  Needs spec work here
> -            break;
> -          case ON_DISCONNECTED:
> -            mConnection->mListener->NotifyClosedConnection();
> -            break;

perhaps to avoid compiler warnings add "default: break;"

Comment 12

[Nils Ohlmeier [:drno]]

Attachment: remove_onconnection_event.patch

Addressed jesup's and jib's comments (removed aResult and added default switch statement).

Carrying forward r+=:jib,:jesup

Try run: https://tbpl.mozilla.org/?tree=Try&rev=a40f26cc6e62

Comment 13

[Nils Ohlmeier [:drno]]

Try build looks green, requesting checkin.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a476db2df3a3

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a476db2df3a3