Bug 158467 – WebRTC: Implement MediaEndpointPeerConnection::setRemoteDescription()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Adam Bergkvist]

Implement the MediaEndponitPeerConnection implementation of RTCPeerConnection.setRemoteDescription [1].

[1] https://w3c.github.io/webrtc-pc/archives/20160513/webrtc.html#dom-rtcpeerconnection-setremotedescription

[Adam Bergkvist]

Created attachment 280807 [details]
Proposed patch

[Eric Carlson]

Comment on attachment 280807 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280807&action=review

> LayoutTests/fast/mediastream/RTCTrackEvent-constructor.html:64
> +                debug("Dictionary members receiver, track and transceiver are not nullable");
> +                shouldThrow("new RTCTrackEvent('eventType', { receiver: null, track: track, transceiver: transceiver})");
> +                shouldThrow("new RTCTrackEvent('eventType', { receiver: receiver, track: null, transceiver: transceiver})");
> +                shouldThrow("new RTCTrackEvent('eventType', { receiver: receiver, track: track, transceiver: null})");
> +                debug("");
> +
> +                debug("Dictionary members receiver, track and transceiver are required");
> +                shouldThrow("new RTCTrackEvent('eventType', { track: track, transceiver: transceiver})");
> +                shouldThrow("new RTCTrackEvent('eventType', { receiver: receiver, transceiver: transceiver})");
> +                shouldThrow("new RTCTrackEvent('eventType', { receiver: receiver, track: track})");
> +                debug("");

Please add a FIXME and a bug # about these failures.

> Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:389
> +

Minor nit: I find the blank line here slightly distracting

> Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:401
> +
> +

Ditto.

> Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:412
> +    for (unsigned i = 0; i < mediaDescriptions.size(); ++i) {
> +        PeerMediaDescription* mediaDescription = mediaDescriptions[i].get();

Nit: can you use a modern for loop here?

> Source/WebCore/Modules/mediastream/RTCTrackEvent.idl:37
> +   [InitializedByEventConstructor] readonly attribute MediaStream[] streams;

Can you use "attribute sequence<MediaStream> streams" instead?

> Source/WebCore/platform/mock/MockMediaEndpoint.cpp:140
> +        for (auto& p : defaultPayloads) {

Nit: a single letter variable name in a modern for loop - such an anachronism :-)

Might as well spell out the variable.

[Adam Bergkvist]

(In reply to comment #2)
> Comment on attachment 280807 [details]
> Proposed patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280807&action=review
> 
> > LayoutTests/fast/mediastream/RTCTrackEvent-constructor.html:64
> > +                debug("Dictionary members receiver, track and transceiver are not nullable");
> > +                shouldThrow("new RTCTrackEvent('eventType', { receiver: null, track: track, transceiver: transceiver})");
> > +                shouldThrow("new RTCTrackEvent('eventType', { receiver: receiver, track: null, transceiver: transceiver})");
> > +                shouldThrow("new RTCTrackEvent('eventType', { receiver: receiver, track: track, transceiver: null})");
> > +                debug("");
> > +
> > +                debug("Dictionary members receiver, track and transceiver are required");
> > +                shouldThrow("new RTCTrackEvent('eventType', { track: track, transceiver: transceiver})");
> > +                shouldThrow("new RTCTrackEvent('eventType', { receiver: receiver, transceiver: transceiver})");
> > +                shouldThrow("new RTCTrackEvent('eventType', { receiver: receiver, track: track})");
> > +                debug("");
> 
> Please add a FIXME and a bug # about these failures.

Fixed.

Bug: https://bugs.webkit.org/show_bug.cgi?id=158536

> > Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:389
> > +
> 
> Minor nit: I find the blank line here slightly distracting

Gone. :)

> > Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:401
> > +
> > +
> 
> Ditto.

Fixed.

> > Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:412
> > +    for (unsigned i = 0; i < mediaDescriptions.size(); ++i) {
> > +        PeerMediaDescription* mediaDescription = mediaDescriptions[i].get();
> 
> Nit: can you use a modern for loop here?

True. Fixed.

> > Source/WebCore/Modules/mediastream/RTCTrackEvent.idl:37
> > +   [InitializedByEventConstructor] readonly attribute MediaStream[] streams;
> 
> Can you use "attribute sequence<MediaStream> streams" instead?

A sequence can't be an interface attribute [1].

[1] http://heycam.github.io/webidl/#idl-sequence

> > Source/WebCore/platform/mock/MockMediaEndpoint.cpp:140
> > +        for (auto& p : defaultPayloads) {
> 
> Nit: a single letter variable name in a modern for loop - such an
> anachronism :-)
> 
> Might as well spell out the variable.

Ops. :) Let's use payload since defaultPayload is taken.

[Adam Bergkvist]

Created attachment 280824 [details]
Updated patch

Thanks for reviewing; updated patch.

[WebKit Commit Bot]

Comment on attachment 280824 [details]
Updated patch

Clearing flags on attachment: 280824

Committed r201851: <http://trac.webkit.org/changeset/201851>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.