Bug 95734

Summary: MediaStream API: Add the async createOffer functionality to RTCPeerConnection
Product: WebKit Reporter: Tommy Widenflycht <tommyw>
Component: WebCore Misc.Assignee: Tommy Widenflycht <tommyw>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, bashi, dglazkov, eric.carlson, feature-media-reviews, fishd, gyuyoung.kim, jamesr, ojan, rakuco, tkent+wkapi, webkit.review.bot
Priority: P2 Keywords: WebExposed
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 80589    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Tommy Widenflycht 2012-09-04 04:39:33 PDT
As before this is an entire slice of functionality.
Comment 1 Tommy Widenflycht 2012-09-04 04:51:05 PDT
Created attachment 162012 [details]
Patch
Comment 2 Tommy Widenflycht 2012-09-04 04:54:34 PDT
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.
Comment 3 WebKit Review Bot 2012-09-04 04:54:40 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 4 Tommy Widenflycht 2012-09-04 05:59:05 PDT
Created attachment 162026 [details]
Patch
Comment 5 Adam Barth 2012-09-04 11:38:35 PDT
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?
Comment 6 Adam Barth 2012-09-04 11:39:40 PDT
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.
Comment 7 Tommy Widenflycht 2012-09-04 12:44:03 PDT
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.
Comment 8 Tommy Widenflycht 2012-09-04 12:51:17 PDT
Created attachment 162083 [details]
Patch
Comment 9 Adam Barth 2012-09-04 13:02:43 PDT
Comment on attachment 162083 [details]
Patch

Thanks.
Comment 10 WebKit Review Bot 2012-09-04 14:01:45 PDT
Comment on attachment 162083 [details]
Patch

Clearing flags on attachment: 162083

Committed r127501: <http://trac.webkit.org/changeset/127501>
Comment 11 WebKit Review Bot 2012-09-04 14:01:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Kenichi Ishibashi 2012-09-04 17:17:13 PDT
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?
Comment 13 Tommy Widenflycht 2012-09-05 00:39:41 PDT
(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?