WebRTC: Update RTCPeerConnection API and introduce PeerConnectionBackend

[Adam Bergkvist]

This change introduces PeerConnectionBackend which is a WebCore interface that allows RTCPeerConnection to have platform abstractions at different levels. For example, the MediaEndpoint interface [1] is a lower level WebRTC backend abstraction where a big part of the WebRTC specification is implemented in WebCore to be reusable. RTCPeerConnectionHandler (in the repo today) is on the other hand a higher level WebRTC backend abstraction where the calls are mostly forwarded directly to the backend. The PeerConnectionBackend interface facilitates both approaches.

RTCPeerConnection
 | (has)
 |
PeerConnectionBackend
 |   |
 |   | (realizes)
 |  MediaEndpointPeerConnection
 |   | (has)
 |   |
 |  MediaEndpoint (platform interface)
 |
 | (realizes)
RTCPeerHandlerPeerConnection
 | (has)
 |
RCPeerConnectionHandler (existing platform interface)

Notable changes:

* Overloaded methods on RTCPeerConnection (Promise + legacy callback signatures) are implemented with JSBuiltins.

* "Queued operations" ([1] Section 4.3.1) is implemented in JS bindings with Promises. Issue: Non-queued operations are still exposed to the script (see FIXME in RTCPeerConnection.idl).

* New RTCPeerConnection features
 - add/removeTrack()
 - pending/currentLocal/RemoteDescription attributes
 - RTCRtpSender/Receiver

* Information carrying objects like RTCSessionDescription and RTCCandidate don't encapsulate a "mirrored" platform object anymore.

* Remove callback implementations (not used with JS bindings)

[1] http://webkit.org/b/150165
[2] https://w3c.github.io/webrtc-pc/archives/20151006/webrtc.html

[Adam Bergkvist]

Created attachment 263151 [details]
Proposed patch

[WebKit Commit Bot]

Attachment 263151 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:34:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/WebCore/ChangeLog:37:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/WebCore/ChangeLog:41:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/WebCore/ChangeLog:46:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/WebCore/ChangeLog:49:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 5 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Bergkvist]

Created attachment 263153 [details]
Updated patch (style in change log)

[Build Bot]

Comment on attachment 263153 [details]
Updated patch (style in change log)

Attachment 263153 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/291006

New failing tests:
js/dom/global-constructors-attributes.html

[Build Bot]

Created attachment 263156 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 263153 [details]
Updated patch (style in change log)

Attachment 263153 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/291007

New failing tests:
js/dom/global-constructors-attributes.html

[Build Bot]

Created attachment 263157 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Adam Bergkvist]

Created attachment 263263 [details]
Proposed patch

Fixed expected results for mac

[Build Bot]

Comment on attachment 263263 [details]
Proposed patch

Attachment 263263 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/295568

New failing tests:
js/dom/global-constructors-attributes.html

[Build Bot]

Created attachment 263266 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 263263 [details]
Proposed patch

Attachment 263263 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/295572

New failing tests:
js/dom/global-constructors-attributes.html

[Build Bot]

Created attachment 263267 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Adam Bergkvist]

Created attachment 263292 [details]
Updated patch

Updated more mac expected results

[Eric Carlson]

Created attachment 264278 [details]
Rebased patch

[Eric Carlson]

The Mac test crash (abbreviated) is:

* thread #1: tid = 0x75d24, 0x0000000108c124a7, queue = 'com.apple.main-thread, stop reason = EXC_BAD_ACCESS (code=1, addre

[Eric Carlson]

Make that:

* thread #1: tid = 0x75d24, 0x0000000108c124a7, queue = 'com.apple.main-thread, stop reason = EXC_BAD_ACCESS (code=1, addre

[Eric Carlson]

Hmm, bugzilla apparently doesn't like characters in the backtrace. One more time?

* thread #1: tid = 0x75d24, 0x0000000108c124a7, queue = 'com.apple.main-thread, stop reason = EXC_BAD_ACCESS 
    frame #0: 0x0000000108c124a7 JavaScriptCore`::WTFCrash() + 39 at Assertions.cpp:321
    frame #1: 0x0000000108098173 JavaScriptCore`std::__1::unique_ptr<JSC::ProgramNode, std::__1::default_delete<JSC::ProgramNode> > JSC::parse<JSC::ProgramNode>(vm=0x0000000112802000, source=0x0000000112813b90, name=0x00007fff5f4da690, builtinMode=Builtin, strictMode=NotStrict, parseMode=ProgramMode, error=0x00007fff5f4da6b8, positionBeforeLastNewline=0x00007fff5f4da718, defaultConstructorKind=None, thisTDZMode=CheckIfNeeded) + 691 at Parser.h:1386
    frame #2: 0x0000000108090eab JavaScriptCore`JSC::createExecutableInternal(vm=0x0000000112802000, source=0x0000000112813b90, name=0x0000000112813b50, constructorKind=None, constructAbility=CannotConstruct) + 411 at BuiltinExecutables.cpp:83
    frame #3: 0x00000001080915f0 JavaScriptCore`JSC::createBuiltinExecutable(vm=0x0000000112802000, code=0x0000000112813b90, name=0x0000000112813b50, constructAbility=CannotConstruct) + 48 at BuiltinExecutables.cpp:72
    frame #4: 0x000000010c9fe978 WebCore`WebCore::RTCPeerConnectionBuiltinsWrapper::rTCPeerConnectionCreateOfferCodeExecutable(this=0x0000000112813b20) + 120 at RTCPeerConnectionBuiltinsWrapper.h:82
    frame #5: 0x000000010c9fa834 WebCore`WebCore::rTCPeerConnectionCreateOfferCodeGenerator(vm=0x0000000112802000) + 52 at RTCPeerConnectionBuiltins.cpp:259
    frame #6: 0x000000010ba033cc WebCore`void JSC::reifyStaticProperties<40u>(vm=0x0000000112802000, values=0x000000010d7fd6e0, thisObj=0x000000011281d620) [40u], JSC::JSObject&) + 396 at Lookup.h:314
    frame #7: 0x000000010ba01a0e WebCore`WebCore::JSRTCPeerConnectionPrototype::finishCreation(this=0x000000011281d620, vm=0x0000000112802000) + 62 at JSRTCPeerConnection.cpp:197
    frame #8: 0x000000010ba037e5 WebCore`WebCore::JSRTCPeerConnectionPrototype::create(vm=0x0000000112802000, globalObject=0x000000011287a900, structure=0x00000001175e8300) + 101 at JSRTCPeerConnection.cpp:111
    frame #9: 0x000000010ba01ca9 WebCore`WebCore::JSRTCPeerConnection::createPrototype(vm=0x0000000112802000, globalObject=0x000000011287a900) + 105 at JSRTCPeerConnection.cpp:209
    frame #10: 0x000000010ba04900 WebCore`JSC::Structure* WebCore::getDOMStructure<WebCore::JSRTCPeerConnection>(vm=0x0000000112802000, globalObject=0x000000011287a900) + 112 at JSDOMBinding.h:145
    frame #11: 0x000000010ba038a0 WebCore`JSC::JSObject* WebCore::getDOMPrototype<WebCore::JSRTCPeerConnection>(vm=0x0000000112802000, globalObject=0x000000011287a900) + 48 at JSDOMBinding.h:156

[Eric Carlson]

The assert happen in parser.parse returns an "Unrecognized token 'Function'" error

[Eric Carlson]

When parsing s_rTCPeerConnectionCreateOfferCode

[Build Bot]

Comment on attachment 264278 [details]
Rebased patch

Attachment 264278 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/350999

New failing tests:
inspector/console/messageRepeatCountUpdated.html
js/dom/global-constructors-attributes-idb.html
js/dom/global-constructors-attributes.html

[Build Bot]

Created attachment 264285 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[youenn fablet]

Comment on attachment 264278 [details]
Rebased patch

Some comments below mostly related to js builtins.

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

> Source/WebCore/Modules/mediastream/RTCPeerConnection.js:31
> +// @optional=MEDIA_STREAM

The annotation scheme changed and is now more inline with other generators.
One can use @conditional=ENABLE(MEDIA_STREAM)

> Source/WebCore/Modules/mediastream/RTCPeerConnection.js:40
> +    if (arguments.length > 1 && arguments[0] instanceof Function && arguments[1] instanceof Function) {

If arguments[0] is not a function, the function switches to promise mode, even if arguments[1] is a function and options a dictionary.
It may be easy to make an error when using that function, and switching from callback to promise version.
Just wondering whether that is a good pattern for users.

On another point, most built-ins use (typeof arg === "function").

> Source/WebCore/Modules/mediastream/RTCPeerConnection.js:48
> +                throw new @TypeError("Argument 3 ('options') to RTCPeerConnection.createOffer must be a Dictionary");

As discussed with adam in another bug patch, it might be good to add routines for this js binding kind of error throwing.

> Source/WebCore/Modules/mediastream/RTCPeerConnection.js:52
> +            return peerConnection.internalCreateOffer(options).then(successCallback, errorCallback);

Might want to use [ Private ] keyword to have @internalCreateOffer.
Might also want to use ImplementedAs so that the C++ createOffer function is called directly.

> Source/WebCore/Modules/mediastream/RTCPeerConnection.js:69
> +function createAnswer()

It seems that all these functions share a lot of code.
It would be good if this could be factored out in some way.

> Source/WebCore/Modules/mediastream/RTCPeerConnectionInternals.js:35
> +function runQueuedOperation(peerConnection, operation)

Rename to runQueuedOperations?
Also would it make sense to take directly @operations as input parameter?
That would allow this function to be reusable in other contexts in the future.

> Source/WebCore/Modules/mediastream/RTCPeerConnectionInternals.js:62
> +    return typeof object == "undefined" || object == null || @isObject(object);

Might want to use ===

Also, this function is generic.
In the future, we should ensure to reuse existing JS built-ins, so probably move these two functions in some other place.

> Source/WebCore/Modules/mediastream/RTCRtpReceiver.cpp:38
> +Ref<RTCRtpReceiver> RTCRtpReceiver::create(RefPtr<MediaStreamTrack>&& track)

Would it make sense to move these methods to the header file, or are you expecting to add more code in them later on?

> Source/WebCore/Modules/mediastream/RTCRtpReceiver.cpp:50
> +}

Is this destructor needed?

> Source/WebCore/Modules/mediastream/RTCRtpReceiver.h:46
> +    RTCRtpReceiver(RefPtr<MediaStreamTrack>&&);

Use explicit?

[Eric Carlson]

(In reply to comment #22)
> Comment on attachment 264278 [details]
> Rebased patch
> 
> Some comments below mostly related to js builtins.
> 
[ Lots of good questions and comments removed ]

I wanted to see where Adam's patch was crashing on the Mac bots, so I rebased it to run locally. I only re-uploaded it didn't because apply cleanly to TOT.

[Adam Bergkvist]

Thanks guys for giving this patch some attention. I'll update it shortly.

[Adam Bergkvist]

Created attachment 265211 [details]
Updated patch

Fixed Youenn's comments

[Adam Bergkvist]

(In reply to comment #22)
> Comment on attachment 264278 [details]
> Rebased patch
> 
> Some comments below mostly related to js builtins.

A lot of good points below. Thanks for reviewing.

> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264278&action=review
> 
> > Source/WebCore/Modules/mediastream/RTCPeerConnection.js:31
> > +// @optional=MEDIA_STREAM
> 
> The annotation scheme changed and is now more inline with other generators.
> One can use @conditional=ENABLE(MEDIA_STREAM)

Updated

> > Source/WebCore/Modules/mediastream/RTCPeerConnection.js:40
> > +    if (arguments.length > 1 && arguments[0] instanceof Function && arguments[1] instanceof Function) {
> 
> If arguments[0] is not a function, the function switches to promise mode,
> even if arguments[1] is a function and options a dictionary.
> It may be easy to make an error when using that function, and switching from
> callback to promise version.
> Just wondering whether that is a good pattern for users.

This is not ideal indeed. The promise and callback versions are specified as overloaded functions so I've rewritten the bindings to behave as such.

> On another point, most built-ins use (typeof arg === "function").

Fixed. At first glance I thought it would be safe to always assume a string return value from this native operator, but it seems that it can return "implementation-dependent" values. So === is safest.

> > Source/WebCore/Modules/mediastream/RTCPeerConnection.js:48
> > +                throw new @TypeError("Argument 3 ('options') to RTCPeerConnection.createOffer must be a Dictionary");
> 
> As discussed with adam in another bug patch, it might be good to add
> routines for this js binding kind of error throwing.
> 
> > Source/WebCore/Modules/mediastream/RTCPeerConnection.js:52
> > +            return peerConnection.internalCreateOffer(options).then(successCallback, errorCallback);
> 
> Might want to use [ Private ] keyword to have @internalCreateOffer.

Fixed. Great to have [ Private ] in place now.

> Might also want to use ImplementedAs so that the C++ createOffer function is
> called directly.

I don't think that can be used in this case since the C++ function never gets called directly (but always via the operations queue)

> > Source/WebCore/Modules/mediastream/RTCPeerConnection.js:69
> > +function createAnswer()
> 
> It seems that all these functions share a lot of code.
> It would be good if this could be factored out in some way.

True. The createOffer/Answer() and setLocal/RemoteDescription() "pairs" now share more code.

> > Source/WebCore/Modules/mediastream/RTCPeerConnectionInternals.js:35
> > +function runQueuedOperation(peerConnection, operation)
> 
> Rename to runQueuedOperations?

This function is called for every call to a "queued operation". I renamed it to enqueueOperation().

> Also would it make sense to take directly @operations as input parameter?
> That would allow this function to be reusable in other contexts in the
> future.

Right now I ensure the existence of @operations at a single point inside this function. Passing the operations array to the function would require ensuring @operation before the call and that's done from multiple locations. I'm not sure how reusable this function is since it implements the quite specific "operations queue" from the WebRTC 1.0 spec.

> > Source/WebCore/Modules/mediastream/RTCPeerConnectionInternals.js:62
> > +    return typeof object == "undefined" || object == null || @isObject(object);
> 
> Might want to use ===

Fixed.

> Also, this function is generic.
> In the future, we should ensure to reuse existing JS built-ins, so probably
> move these two functions in some other place.

I totally agree. Leaving it as is for this change though.

> > Source/WebCore/Modules/mediastream/RTCRtpReceiver.cpp:38
> > +Ref<RTCRtpReceiver> RTCRtpReceiver::create(RefPtr<MediaStreamTrack>&& track)
> 
> Would it make sense to move these methods to the header file, or are you
> expecting to add more code in them later on?

Moved. This makes the cpp file rather slim, but I'm keeping since it will get more content before it's properly implemented.

> > Source/WebCore/Modules/mediastream/RTCRtpReceiver.cpp:50
> > +}
> 
> Is this destructor needed?

Fixed.

> > Source/WebCore/Modules/mediastream/RTCRtpReceiver.h:46
> > +    RTCRtpReceiver(RefPtr<MediaStreamTrack>&&);
> 
> Use explicit?

Fixed

[Build Bot]

Comment on attachment 265211 [details]
Updated patch

Attachment 265211 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/411387

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 265219 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Adam Bergkvist]

Created attachment 265283 [details]
Updated patch

Attempt to fix mac-debug test failures

[Eric Carlson]

Comment on attachment 265283 [details]
Updated patch

Thank you Adam!

[youenn fablet]

Comment on attachment 265283 [details]
Updated patch

This looks like a great move forward :)

Two general comments come to mind:
- There is some refactoring as part of this patch that could have been splitted so as to ease the review. This may be good for future developments.
- I am surprised that no test is added/few tests are rebased. How is WebRTC tested? It would be great to add/activate tests, even if they are mostly failing at the beginning.
Without tests, it might be so easy to inadvertently break some behavior (like the promise/legacy switch for instance).
There are a few tests available at https://github.com/w3c/web-platform-tests/tree/master/webrtc/ that can be easily imported (see http://trac.webkit.org/wiki/WebKitW3CTesting), maybe some other tests can be imported from Blink/Mozilla.

Please find some additional comments below.

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

> Source/WebCore/ChangeLog:34
> +        Notable changes:

Maybe for future developments, you could split each notable change in its own bug/patch?

> Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:40
> +    return nullptr;

I guess this function is not yet implemented.
Would it be useful to use notImplemented()?

> Source/WebCore/Modules/mediastream/PeerConnectionBackend.h:71
> +    virtual ScriptExecutionContext* context() const = 0;

XHR and others are naming it scriptExecutionContext().
Would it be useful to rename it that way?

> Source/WebCore/Modules/mediastream/PeerConnectionBackend.h:89
> +    virtual void setLocalDescription(RTCSessionDescription*, PeerConnection::VoidPromise&&) = 0;

Is there a specific reason to using a pointer here?
Also true for setRemoteDescription, addIceCandidate and getStats.

> Source/WebCore/Modules/mediastream/PeerConnectionStates.h:40
> +enum class SignalingState {

It seems SignalingState is mainly used in RTCPeerConnection.
Would it make sense to move the definition within RTCPeerConnection as a public enum class?
Or is it supposed to be used in other contexts?

Might apply to other enum classes as well.

> Source/WebCore/Modules/mediastream/RTCIceCandidate.cpp:102
> +

Remove empty line here.
You might also consider moving those getters/setters to header.

> Source/WebCore/Modules/mediastream/RTCOfferAnswerOptions.cpp:37
> +    : m_voiceActivityDetection(true)

Move to header if that stays simple?

> Source/WebCore/Modules/mediastream/RTCOfferAnswerOptions.h:57
> +    virtual ~RTCOfferOptions() { }

Is it needed?

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:138
> +    if (!streams.size()) {

Is it possible for streams to have a null MediaStream pointer?

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:223
> +    return m_backend->localDescription();

Move it to header?

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:228
> +    return m_backend->currentLocalDescription();

Ditto?

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:231
> +RefPtr<RTCSessionDescription> RTCPeerConnection::pendingLocalDescription() const

Ditto?

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:411
> +    scriptExecutionContext()->postTask([=](ScriptExecutionContext&) {

First time I see [=]() use in WebKit. It is nice to see it :)

> Source/WebCore/Modules/mediastream/RTCPeerConnection.js:36
> +

For all these functions, we should somehow check that "this" has the right type.
We should work on that in the future.

> Source/WebCore/Modules/mediastream/RTCPeerConnection.js:74
> +    if (arguments.length == 1) {

It would be good to have a test for that behavior (one arg -> promise, more than one arg, legacy callbacks).
It may happen that future patches break that behavior without noticing it.

> Source/WebCore/Modules/mediastream/RTCPeerConnection.js:102
> +        if (selector != null && !(selector instanceof MediaStreamTrack))

Why checking selector != null?
Having a test might be good to validate the behavior.

> Source/WebCore/Modules/mediastream/RTCPeerConnection.js:118
> +    peerConnection.@privateGetStats(selector).then(successCallback, errorCallback);

You might want to check whether the direct use of then is appropriate.
A user script may modify Promise prototype and its then function.
@Promise.prototype.@then.@call is (unfortunately less readable but) safer.

> Source/WebCore/Modules/mediastream/RTCPeerConnectionInternals.js:32
> +// @internal

How do you expose these internal functions to JS built-ins?
I would expect changes within JSDOMWindowBase::finishCreation to do that explicitly.

In the future, tooling should be improved to not require that.

> Source/WebCore/Modules/mediastream/RTCPeerConnectionInternals.js:50
> +    return new Promise(function (resolve, reject) {

"return new @Promise" would be safer.

> Source/WebCore/Modules/mediastream/RTCPeerConnectionInternals.js:72
> +            return targetFunction.@apply(peerConnection, [options]);

or @call(peerConnection, options)?

> Source/WebCore/Modules/mediastream/RTCPeerConnectionInternals.js:84
> +        return targetFunction.@apply(peerConnection, [options]).then(successCallback, errorCallback);

Ditto?

> Source/WebCore/Modules/mediastream/RTCPeerConnectionInternals.js:114
> +        return targetFunction.@apply(peerConnection, [description]).then(successCallback, errorCallback);

Ditto?

> Source/WebCore/Modules/mediastream/RTCPeerConnectionInternals.js:129
> +    return typeof object === "undefined" || object == null || (typeof object === "object");

This function is generic.
Maybe we should move it to GlobalObject.js

> Source/WebCore/Modules/mediastream/RTCRtpSenderReceiverBase.cpp:45
> +RTCRtpSenderReceiverBase::~RTCRtpSenderReceiverBase()

Move to header?

> Source/WebCore/Modules/mediastream/RTCRtpSenderReceiverBase.cpp:49
> +MediaStreamTrack* RTCRtpSenderReceiverBase::track() const

Ditto?

> Source/WebCore/Modules/mediastream/RTCSessionDescription.cpp:-35
> -

Not sure about the style, but usually there is an empty line between #if and #include

> Source/WebCore/Modules/mediastream/RTCSessionDescription.cpp:73
> +    return adoptRef(*new RTCSessionDescription(type, sdp));

Move to header?

> Source/WebCore/Modules/mediastream/RTCSessionDescription.cpp:88
> +    return m_type;

Ditto?

> Source/WebCore/Modules/mediastream/RTCSessionDescription.cpp:101
> +    return m_sdp;

Ditto?

> Source/WebCore/Modules/mediastream/RTCSessionDescription.cpp:106
> +    m_sdp = sdp;

Ditto?

> Source/WebCore/Modules/mediastream/RTCSessionDescription.h:36
>  #include "ExceptionBase.h"

IIRC, if ExceptionCode is the only one used, it is preferred to remove that #include and add a typedef int ExceptionCode instead.

> Source/WebCore/Modules/mediastream/RTCTrackEvent.cpp:63
> +RTCTrackEvent::RTCTrackEvent()

Move to header?

> Source/WebCore/Modules/mediastream/RTCTrackEvent.cpp:81
> +RTCTrackEvent::~RTCTrackEvent()

Is it needed? Move to header?

[Adam Bergkvist]

(In reply to comment #31)
> Comment on attachment 265283 [details]
> Updated patch

Thanks for reviewing. I'll respond to your general comments now and fix the rest later today.

> This looks like a great move forward :)
> 
> Two general comments come to mind:
> - There is some refactoring as part of this patch that could have been
> splitted so as to ease the review. This may be good for future developments.

I agree that this patch could have been split up and some pure refactoring changes could have been in stand-alone patches. I was a bit lazy when I extracted it from the downstream fork. This patch contains the foundation and it will be much easier to make smaller patches that incrementally adds functionality from now on.

> - I am surprised that no test is added/few tests are rebased. How is WebRTC
> tested? It would be great to add/activate tests, even if they are mostly
> failing at the beginning.
> Without tests, it might be so easy to inadvertently break some behavior
> (like the promise/legacy switch for instance).
> There are a few tests available at
> https://github.com/w3c/web-platform-tests/tree/master/webrtc/ that can be
> easily imported (see http://trac.webkit.org/wiki/WebKitW3CTesting), maybe
> some other tests can be imported from Blink/Mozilla.

The reason why this patch doesn't add or enable any tests is that it's not possible to construct an RTCPeerConnection yet. The missing piece is an PeerConnectionBackend implementation and it's coming in a follow up patch. I didn't plan to make a mock object for it since it's a WebCore component.

[Adam Bergkvist]

Created attachment 265482 [details]
Candidate for landing

Addressed Youenn's comments

[Adam Bergkvist]

(In reply to comment #31)
> Comment on attachment 265283 [details]
> Updated patch
> 
> [...]
>
> Please find some additional comments below.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=265283&action=review
> 
> > Source/WebCore/ChangeLog:34
> > +        Notable changes:
> 
> Maybe for future developments, you could split each notable change in its
> own bug/patch?

Yes. The size of this patch is far from ideal. I plan to do follow-up patches on a more per-feature basis.

> > Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:40
> > +    return nullptr;
> 
> I guess this function is not yet implemented.
> Would it be useful to use notImplemented()?

This is the create function used when no proper implementation is available. The nullptr will result in an exception being thrown when an RTCPeerConnection is created. I borrowed this pattern from RTCPeerConnectionHandler.

> > Source/WebCore/Modules/mediastream/PeerConnectionBackend.h:71
> > +    virtual ScriptExecutionContext* context() const = 0;
> 
> XHR and others are naming it scriptExecutionContext().
> Would it be useful to rename it that way?
> 

The scriptExecutionContext() inherited from ActiveDOMObject seems to satisfy the interface, so let's use that one.

> > Source/WebCore/Modules/mediastream/PeerConnectionBackend.h:89
> > +    virtual void setLocalDescription(RTCSessionDescription*, PeerConnection::VoidPromise&&) = 0;
> 
> Is there a specific reason to using a pointer here?
> Also true for setRemoteDescription, addIceCandidate and getStats.

Null shouldn't get passed the bindings so we can use refs here. Added ASSERTs as well.
 
> > Source/WebCore/Modules/mediastream/PeerConnectionStates.h:40
> > +enum class SignalingState {
> 
> It seems SignalingState is mainly used in RTCPeerConnection.
> Would it make sense to move the definition within RTCPeerConnection as a
> public enum class?
> Or is it supposed to be used in other contexts?
> 
> Might apply to other enum classes as well.

These states used to be part of a platform interface to be accessible by the WebRTC backend. They are not used that way currently but other backends may want to access them in the future.

> > Source/WebCore/Modules/mediastream/RTCIceCandidate.cpp:102
> > +
> 
> Remove empty line here.

Fixed.

> You might also consider moving those getters/setters to header.

Moved.

> > Source/WebCore/Modules/mediastream/RTCOfferAnswerOptions.cpp:37
> > +    : m_voiceActivityDetection(true)
> 
> Move to header if that stays simple?

I think the cpp is fine in this case since the other constructor with an init list is there.

> > Source/WebCore/Modules/mediastream/RTCOfferAnswerOptions.h:57
> > +    virtual ~RTCOfferOptions() { }
> 
> Is it needed?

Nope. Same goes for the one in RTCAnswerOptions.

> 
> > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:138
> > +    if (!streams.size()) {
> 
> Is it possible for streams to have a null MediaStream pointer?

The binding catches that.

> > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:223
> > +    return m_backend->localDescription();
> 
> Move it to header?

I think the header looks nicer without these longer expressions inlined. I don't have a strong opinion though.

> > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:228
> > +    return m_backend->currentLocalDescription();
> 
> Ditto?
> 
> > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:231
> > +RefPtr<RTCSessionDescription> RTCPeerConnection::pendingLocalDescription() const
> 
> Ditto?
>
> > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:411
> > +    scriptExecutionContext()->postTask([=](ScriptExecutionContext&) {
> 
> First time I see [=]() use in WebKit. It is nice to see it :)

I believe it was in this file already so I can't take credit for introducing it. :)

> > Source/WebCore/Modules/mediastream/RTCPeerConnection.js:36
> > +
> 
> For all these functions, we should somehow check that "this" has the right
> type.
> We should work on that in the future.

Would it be sufficient to do @isObject() and check for one of the [Private] function symbols? For example @queuedCreateOffer? Perhaps we can do this is a follow-up patch?

> > Source/WebCore/Modules/mediastream/RTCPeerConnection.js:74
> > +    if (arguments.length == 1) {
> 
> It would be good to have a test for that behavior (one arg -> promise, more
> than one arg, legacy callbacks).
> It may happen that future patches break that behavior without noticing it.

We can check the return value to see which version was called. I can make a bug on it so it's not forgotten.

> > Source/WebCore/Modules/mediastream/RTCPeerConnection.js:102
> > +        if (selector != null && !(selector instanceof MediaStreamTrack))
> 
> Why checking selector != null?
> Having a test might be good to validate the behavior.

Null is allowed. We should be able to test this rather soon.

> > Source/WebCore/Modules/mediastream/RTCPeerConnection.js:118
> > +    peerConnection.@privateGetStats(selector).then(successCallback, errorCallback);
> 
> You might want to check whether the direct use of then is appropriate.
> A user script may modify Promise prototype and its then function.
> @Promise.prototype.@then.@call is (unfortunately less readable but) safer.

This is indeed a problem. However I suspect using @Promise.prototype.then.@call only helps if the user has modifies then() on an instance, not if the actual prototype function is modified. And since the bindings only calls then() on internal promise instances created inside the binding, using the prototype-call only complicates syntax without any real benefit. We should have a general solution for this.

> > Source/WebCore/Modules/mediastream/RTCPeerConnectionInternals.js:32
> > +// @internal
> 
> How do you expose these internal functions to JS built-ins?
> I would expect changes within JSDOMWindowBase::finishCreation to do that
> explicitly.
 
The plan is to have that done in a follow-up patch. The license of JSDOMWindowBase prevents me from changing it. :/

> In the future, tooling should be improved to not require that.

That would be great.

> > Source/WebCore/Modules/mediastream/RTCPeerConnectionInternals.js:50
> > +    return new Promise(function (resolve, reject) {
> 
> "return new @Promise" would be safer.

Fixed.

> > Source/WebCore/Modules/mediastream/RTCPeerConnectionInternals.js:72
> > +            return targetFunction.@apply(peerConnection, [options]);
> 
> or @call(peerConnection, options)?

That's better (not having to create the arguments list). Fixed.

> > Source/WebCore/Modules/mediastream/RTCPeerConnectionInternals.js:84
> > +        return targetFunction.@apply(peerConnection, [options]).then(successCallback, errorCallback);
> 
> Ditto?
> 
> > Source/WebCore/Modules/mediastream/RTCPeerConnectionInternals.js:114
> > +        return targetFunction.@apply(peerConnection, [description]).then(successCallback, errorCallback);
> 
> Ditto?
> 
> > Source/WebCore/Modules/mediastream/RTCPeerConnectionInternals.js:129
> > +    return typeof object === "undefined" || object == null || (typeof object === "object");
> 
> This function is generic.
> Maybe we should move it to GlobalObject.js

Fixed.

> 
> > Source/WebCore/Modules/mediastream/RTCRtpSenderReceiverBase.cpp:45
> > +RTCRtpSenderReceiverBase::~RTCRtpSenderReceiverBase()
> 
> Move to header?

Lets skip the cpp file all together here.

> > Source/WebCore/Modules/mediastream/RTCRtpSenderReceiverBase.cpp:49
> > +MediaStreamTrack* RTCRtpSenderReceiverBase::track() const
> 
> Ditto?
>
>
> > Source/WebCore/Modules/mediastream/RTCSessionDescription.cpp:-35
> > -
> 
> Not sure about the style, but usually there is an empty line between #if and
> #include
>

Seems to be more common with an empty line. Seems to work both ways for style checker.

> > Source/WebCore/Modules/mediastream/RTCSessionDescription.cpp:73
> > +    return adoptRef(*new RTCSessionDescription(type, sdp));
> 
> Move to header?

There are three create functions and one is pretty big. My thinking was to keep the all in the cpp even though two are one-liners.

> > Source/WebCore/Modules/mediastream/RTCSessionDescription.cpp:88
> > +    return m_type;
> 
> Ditto?
>

Fixed (and 2 more)

> > Source/WebCore/Modules/mediastream/RTCSessionDescription.cpp:101
> > +    return m_sdp;
> 
> Ditto?
> 
> > Source/WebCore/Modules/mediastream/RTCSessionDescription.cpp:106
> > +    m_sdp = sdp;
> 
> Ditto?
> 
> > Source/WebCore/Modules/mediastream/RTCSessionDescription.h:36
> >  #include "ExceptionBase.h"
> 
> IIRC, if ExceptionCode is the only one used, it is preferred to remove that
> #include and add a typedef int ExceptionCode instead.

Fixed.

> > Source/WebCore/Modules/mediastream/RTCTrackEvent.cpp:63
> > +RTCTrackEvent::RTCTrackEvent()
> 
> Move to header?

I can't keep my forward declaration of RTCRtpReceiver if I move it.

> > Source/WebCore/Modules/mediastream/RTCTrackEvent.cpp:81
> > +RTCTrackEvent::~RTCTrackEvent()
> 
> Is it needed? Move to header?

Removed

[Adam Bergkvist]

Created attachment 265483 [details]
Candidate for landing (rebased)

Rebased patch

[Adam Bergkvist]

Created attachment 265487 [details]
Candidate for landing (rebased)

[WebKit Commit Bot]

Comment on attachment 265487 [details]
Candidate for landing (rebased)

Clearing flags on attachment: 265487

Committed r192464: <http://trac.webkit.org/changeset/192464>

[youenn fablet]

Nice to see this patch landed.

> > > Source/WebCore/Modules/mediastream/RTCPeerConnection.js:36
> > > +
> > 
> > For all these functions, we should somehow check that "this" has the right
> > type.
> > We should work on that in the future.
> 
> Would it be sufficient to do @isObject() and check for one of the [Private]
> function symbols? For example @queuedCreateOffer? Perhaps we can do this is
> a follow-up patch?

Checking that @queuedCreateOffer exists currently means that the object prototype is not modified by user scripts.
I think C++ implemented APIs work on instances even if their prototypes are changed.
I am not sure whether we should move these private functions to instance slots or not.

> > > Source/WebCore/Modules/mediastream/RTCPeerConnection.js:74
> > > +    if (arguments.length == 1) {
> > 
> > It would be good to have a test for that behavior (one arg -> promise, more
> > than one arg, legacy callbacks).
> > It may happen that future patches break that behavior without noticing it.
> 
> We can check the return value to see which version was called. I can make a
> bug on it so it's not forgotten.

Sounds good, especially if it ends up being part of WebRTC W3C test suite.
This would ensure every browser has the same understanding.

> > > Source/WebCore/Modules/mediastream/RTCPeerConnection.js:118
> > > +    peerConnection.@privateGetStats(selector).then(successCallback, errorCallback);
> > 
> > You might want to check whether the direct use of then is appropriate.
> > A user script may modify Promise prototype and its then function.
> > @Promise.prototype.@then.@call is (unfortunately less readable but) safer.
> 
> This is indeed a problem. However I suspect using
> @Promise.prototype.then.@call only helps if the user has modifies then() on
> an instance, not if the actual prototype function is modified. And since the
> bindings only calls then() on internal promise instances created inside the
> binding, using the prototype-call only complicates syntax without any real
> benefit. We should have a general solution for this.

A user script may decide to change the Promise prototype, in which case all promises instances will be affected, hence the unsafe use of then.

We could use InternalPromise for private methods.
But we need to ensure that we never expose any InternalPromise instance to user scripts.

Agreed on the need for a better more general solution.

> 
> > > Source/WebCore/Modules/mediastream/RTCPeerConnectionInternals.js:32
> > > +// @internal
> > 
> > How do you expose these internal functions to JS built-ins?
> > I would expect changes within JSDOMWindowBase::finishCreation to do that
> > explicitly.
>  
> The plan is to have that done in a follow-up patch. The license of
> JSDOMWindowBase prevents me from changing it. :/

If that is stopping you from progressing, please drop me a word.

[youenn fablet]

Comment on attachment 265487 [details]
Candidate for landing (rebased)

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

> LayoutTests/ChangeLog:14
> +        * platform/mac-yosemite/js/dom/global-constructors-attributes-expected.txt:

Forgot to mention that some additional rebasing might be needed, the default one and win flavour at least.

[Ryan Haddad]

Rebaselined js/dom/global-constructors-attributes for El Cap in r192473.