Remove onconnection and onclosedconnection from PC

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: drno, Assigned: drno)

Tracking

(Blocks 1 bug, {dev-doc-needed, site-compat})

Trunk
mozilla32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Assignee

Description

5 years ago
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.
Assignee

Comment 1

5 years ago
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).
Attachment #8426729 - Flags: review?(rjesup)
Attachment #8426729 - Flags: review?(jib)
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
Attachment #8426729 - Flags: review?(jib) → review+
Assignee

Comment 3

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

Carrying forward r+=jib
Attachment #8426729 - Attachment is obsolete: true
Attachment #8426729 - Flags: review?(rjesup)
Attachment #8426849 - Flags: review?(rjesup)
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
Attachment #8426849 - Flags: review?(rjesup)
Attachment #8426849 - Flags: review?(bugs)
Attachment #8426849 - Flags: review+
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.
Attachment #8426849 - Flags: review?(bugs)
(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.
Assignee

Comment 7

5 years ago
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.
Assignee

Comment 8

5 years ago
This should hopefully remove all implementation and unit testing code.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=5bd72913cc86
Attachment #8426849 - Attachment is obsolete: true
Attachment #8429583 - Flags: review?(rjesup)
Attachment #8429583 - Flags: review?(jib)
Comment on attachment 8429583 [details] [diff] [review]
remove_onconnection_event.patch

Since the .webidl changes need a dom peer review, r=smaug for those.
Attachment #8429583 - Flags: review+
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.
Attachment #8429583 - Flags: review?(jib) → review+
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;"
Attachment #8429583 - Flags: review?(rjesup) → review+
Assignee

Comment 12

5 years ago
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
Attachment #8429583 - Attachment is obsolete: true
Assignee

Comment 13

5 years ago
Try build looks green, requesting checkin.
Assignee: nobody → drno
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a476db2df3a3
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Blocks: 1050930
You need to log in before you can comment on or make changes to this bug.