Bug 95734 – MediaStream API: Add the async createOffer functionality to RTCPeerConnection

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Tommy Widenflycht]

As before this is an entire slice of functionality.

[Tommy Widenflycht]

Created attachment 162012 [details]
Patch

[Tommy Widenflycht]

Here's another usage of the "abstract-base-class-in-platform-with-implementation-in-webcore-proper" pattern that I used recently.

The RequestImpl class has the reference to two callbacks which the Request class hides from the platform implementor.

Other than that the rest of the code is pretty much business as usual, nothing I haven't done before several times.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Tommy Widenflycht]

Created attachment 162026 [details]
Patch

[Adam Barth]

Comment on attachment 162026 [details]
Patch

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

> Source/Platform/chromium/public/WebRTCSessionDescriptionDescriptor.h:57
> +/*
> + *  In order to establish the media plane, PeerConnection needs specific
> + *  parameters to indicate what to transmit to the remote side, as well
> + *  as how to handle the media that is received. These parameters are
> + *  determined by the exchange of session descriptions in offers and
> + *  answers, and there are certain details to this process that must be
> + *  handled in the JSEP APIs.
> +
> + *  Whether a session description was sent or received affects the
> + *  meaning of that description. For example, the list of codecs sent to
> + *  a remote party indicates what the local side is willing to decode,
> + *  and what the remote party should send.
> + */

C++ style comments pls.  :-)

> Source/Platform/chromium/public/WebRTCSessionDescriptionDescriptor.h:58
> +class WebRTCSessionDescriptionDescriptor {

"DescriptionDescriptor" :(

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:145
> +    RefPtr<RTCSessionDescriptionCallback> successCallback = prpSuccessCallback;

There's on real point in creating a successCallback local.  All you're using it for is to test for null, which you can do on the PassRefPtr directly.  Once you remove successCallback you can rename prpSuccessCallback to successCallback

> Source/WebCore/Modules/mediastream/RTCSessionDescriptionRequestImpl.cpp:64
> +void RTCSessionDescriptionRequestImpl::requestSucceeded(PassRefPtr<RTCSessionDescriptionDescriptor> sdd)

sdd -> sessionDescriptionDescriptor

Please use complete words in variable names.  I know we use some abbreviations in this code, but we'd like to limit it to abbreviations that appear in the spec.

> Source/WebCore/Modules/mediastream/RTCSessionDescriptionRequestImpl.cpp:76
> +    if (m_errorCallback.get())

No need to call .get() here.  You can test m_errorCallback the same way you've tested m_successCallback on line 66.

> Tools/DumpRenderTree/chromium/MockWebRTCPeerConnectionHandler.cpp:99
> +        request.requestSucceeded(sd);
> +    } else
> +        request.requestFailed("TEST_ERROR");

Do we want to call these back async during testing to make the test environment more like the production environment?

[Adam Barth]

Comment on attachment 162026 [details]
Patch

DescriptionDescriptor is really a goofy name, but I'm not sure what to recommend that's better.  I'm sure someone will make fun of us later for having that name in the code.

[Tommy Widenflycht]

Comment on attachment 162026 [details]
Patch

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

>> Source/Platform/chromium/public/WebRTCSessionDescriptionDescriptor.h:57
>> + */
> 
> C++ style comments pls.  :-)

Fixed.

>> Source/Platform/chromium/public/WebRTCSessionDescriptionDescriptor.h:58
>> +class WebRTCSessionDescriptionDescriptor {
> 
> "DescriptionDescriptor" :(

Fixme added.

>> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:145
>> +    RefPtr<RTCSessionDescriptionCallback> successCallback = prpSuccessCallback;
> 
> There's on real point in creating a successCallback local.  All you're using it for is to test for null, which you can do on the PassRefPtr directly.  Once you remove successCallback you can rename prpSuccessCallback to successCallback

Fixed.

>> Source/WebCore/Modules/mediastream/RTCSessionDescriptionRequestImpl.cpp:64
>> +void RTCSessionDescriptionRequestImpl::requestSucceeded(PassRefPtr<RTCSessionDescriptionDescriptor> sdd)
> 
> sdd -> sessionDescriptionDescriptor
> 
> Please use complete words in variable names.  I know we use some abbreviations in this code, but we'd like to limit it to abbreviations that appear in the spec.

Fixed. Sorry!

>> Source/WebCore/Modules/mediastream/RTCSessionDescriptionRequestImpl.cpp:76
>> +    if (m_errorCallback.get())
> 
> No need to call .get() here.  You can test m_errorCallback the same way you've tested m_successCallback on line 66.

Doh. Fixed.

>> Tools/DumpRenderTree/chromium/MockWebRTCPeerConnectionHandler.cpp:99
>> +        request.requestFailed("TEST_ERROR");
> 
> Do we want to call these back async during testing to make the test environment more like the production environment?

Fixed.

[Tommy Widenflycht]

Created attachment 162083 [details]
Patch

[Adam Barth]

Comment on attachment 162083 [details]
Patch

Thanks.

[WebKit Review Bot]

Comment on attachment 162083 [details]
Patch

Clearing flags on attachment: 162083

Committed r127501: <http://trac.webkit.org/changeset/127501>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kenichi Ishibashi]

This change caused build failures on Win(e.g. http://build.chromium.org/p/chromium.webkit/builders/Win%20Builder/builds/27580)

I removed chromium/public/WebOfferAnswerRequest.h from Platform.gypi to try to fix the failures. http://trac.webkit.org/changeset/127526

Tommy, could you check and do proper fix?

[Tommy Widenflycht]

(In reply to comment #12)
> This change caused build failures on Win(e.g. http://build.chromium.org/p/chromium.webkit/builders/Win%20Builder/builds/27580)
> 
> I removed chromium/public/WebOfferAnswerRequest.h from Platform.gypi to try to fix the failures. http://trac.webkit.org/changeset/127526
> 
> Tommy, could you check and do proper fix?

That was the proper fix. Thanks for that and sorry for leaving that erroneous file in there. I

s there a reason why it is only the cr-win build that has that warning?