Created attachment 162012 [details] Patch

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.

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.

Created attachment 162026 [details] Patch

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 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 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.

Created attachment 162083 [details] Patch

Comment on attachment 162083 [details] Patch Thanks.

Comment on attachment 162083 [details] Patch Clearing flags on attachment: 162083 Committed r127501: <http://trac.webkit.org/changeset/127501>

All reviewed patches have been landed. Closing bug.

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?

(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?