Bug 230262

Summary:

Migrate to libwebrtc non-racy setRemoteDescription/setLocalDescription variants

Product:

WebKit

Reporter:

youenn fablet <youennf>

Component:

WebRTC

Assignee:

youenn fablet <youennf>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

darin, eric.carlson, ews-watchlist, glenn, hta, jer.noble, philipj, sergio, tommyw, webkit-bug-importer, youennf

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch for landing	none

youenn fablet

Reported 2021-09-14 06:20:48 PDT

Migrate to libwebrtc non-racy setRemoteDescription/setLocalDescription variants

Attachments
Patch (14.33 KB, patch) 2021-09-14 06:35 PDT, youenn fablet	no flags	Details Formatted Diff Diff
Patch (18.00 KB, patch) 2021-09-14 09:22 PDT, youenn fablet	no flags	Details Formatted Diff Diff
Patch for landing (17.75 KB, patch) 2021-09-15 00:20 PDT, youenn fablet	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

youenn fablet

Comment 1 2021-09-14 06:35:29 PDT

Created attachment 438129 [details] Patch

youenn fablet

Comment 2 2021-09-14 09:22:39 PDT

Created attachment 438150 [details] Patch

Darin Adler

Comment 3 2021-09-14 13:49:21 PDT

Comment on attachment 438150 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=438150&action=review > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:148 > + rtc::scoped_refptr<webrtc::SetLocalDescriptionObserverInterface> observer(&m_setLocalSessionDescriptionObserver); I wonder if we can leave out the <> here. If it was std::unique_ptr or WTF::RefPtr we could, but not sure about rtc::scoped_refptr. > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:182 > + rtc::scoped_refptr<webrtc::SetRemoteDescriptionObserverInterface> observer(&m_setRemoteSessionDescriptionObserver); Ditto. > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCObservers.h:69 > + void OnSetLocalDescriptionComplete(webrtc::RTCError error) final Not important, but seems like this can be private. > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCObservers.h:93 > + void OnSetRemoteDescriptionComplete(webrtc::RTCError error) final Not important, but seems like this can be private.

youenn fablet

Comment 4 2021-09-15 00:18:58 PDT

(In reply to Darin Adler from comment #3) > Comment on attachment 438150 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=438150&action=review > > > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:148 > > + rtc::scoped_refptr<webrtc::SetLocalDescriptionObserverInterface> observer(&m_setLocalSessionDescriptionObserver); > > I wonder if we can leave out the <> here. If it was std::unique_ptr or > WTF::RefPtr we could, but not sure about rtc::scoped_refptr. We indeed can do it. Thinking more about it, given the typed parameters, we can probably just pass a pointer to the methods and the scoped_refptr will be created for us. > > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:182 > > + rtc::scoped_refptr<webrtc::SetRemoteDescriptionObserverInterface> observer(&m_setRemoteSessionDescriptionObserver); > > Ditto. > > > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCObservers.h:69 > > + void OnSetLocalDescriptionComplete(webrtc::RTCError error) final > > Not important, but seems like this can be private. OK. > > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCObservers.h:93 > > + void OnSetRemoteDescriptionComplete(webrtc::RTCError error) final > > Not important, but seems like this can be private. OK.

youenn fablet

Comment 5 2021-09-15 00:20:07 PDT

Created attachment 438219 [details] Patch for landing

EWS

Comment 6 2021-09-15 01:19:09 PDT

Committed r282444 (241697@main): <https://commits.webkit.org/241697@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 438219 [details].

Radar WebKit Bug Importer

Comment 7 2021-09-15 01:20:17 PDT

<rdar://problem/83135937>

Note You need to log in before you can comment on or make changes to this bug.