Bug 145308

Created attachment 253979 [details] Patch

Comment on attachment 253979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253979&action=review > Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:86 > return UserMediaRequest::create(context, controller, options, WTF::move(resolveCallback), WTF::move(rejectCallback), ec); After looking at current implementation, I think resolveCallback and rejectCallback are currently always called asynchronously, which is necessary for webkitGetUserMedia implementation. But code might change and may break this assumption. An ASSERT would be ideal here. Or we can ensure that handleEvent calls are made asynchronous using postTask. Any thought?

As you say above, the scheduling of calls to handleEvent() must done in the webkitGetUserMedia() case. But I'm a bit confused since this patch removes the "always schedule" case (which is good), but never adds any conditional scheduling.

(In reply to comment #3) > As you say above, the scheduling of calls to handleEvent() must done in the > webkitGetUserMedia() case. But I'm a bit confused since this patch removes > the "always schedule" case (which is good), but never adds any conditional > scheduling. I checked again and it seems I was indeed partially wrong. The success callback is always called asynchronously: the user permission is always requested and it is done inside a callOnMainThread. But it seems that some error cases in the constraints validation may call the failure callback synchronously. A slightly sub-optimal but simple approach may be to call handleEvent for error cases inside a postTask. For HandleEvent success callback, we do not need postTask (although it may be safer if code changes a lot). How about that?

Created attachment 254165 [details] Emulating promise resolution with postTask

(In reply to comment #5) > Created attachment 254165 [details] > Emulating promise resolution with postTask I changed my mind and went the safe way, using postTask for both success and error callbacks. There is no real point in trying to optimize an API (webkitGetUerMedia) that is in the process of being replaced.

Comment on attachment 254165 [details] Emulating promise resolution with postTask Clearing flags on attachment: 254165 Committed r185191: <http://trac.webkit.org/changeset/185191>

All reviewed patches have been landed. Closing bug.

(In reply to comment #6) > (In reply to comment #5) > > Created attachment 254165 [details] > > Emulating promise resolution with postTask > > I changed my mind and went the safe way, using postTask for both success and > error callbacks. Sounds reasonable. :)

Thanks for taking the time to improve the Promise handling here. :) The RTCPeerConnection API will use Promises as well (together with callbacks for legacy interop) so it's great to get these in better shape.

(In reply to comment #10) > Thanks for taking the time to improve the Promise handling here. :) The > RTCPeerConnection API will use Promises as well (together with callbacks for > legacy interop) so it's great to get these in better shape. I worked on some additional Promise support improvement lately. I hope to enable automatic binding generation for AudioContext promise API. Please have a look at it when starting RTCPeerConnection API implementation. The case of "Promise + callbacks-for-legacy" APIs may not be as straightforward though. There is also the case of exceptions rejecting promises that might need some specific support.