[webkit-reviews] review canceled: [Bug 221296] SpeechRecognitionPermissionManager should not handle requests that are already cancelled : [Attachment 419099] Patch

[bugzilla-daemon at webkit.org]

Sihui Liu <sihui_liu at apple.com> has canceled Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 221296: SpeechRecognitionPermissionManager should not handle requests that
are already cancelled
https://bugs.webkit.org/show_bug.cgi?id=221296

Attachment 419099: Patch

https://bugs.webkit.org/attachment.cgi?id=419099&action=review




--- Comment #5 from Sihui Liu <sihui_liu at apple.com> ---
Comment on attachment 419099
  --> https://bugs.webkit.org/attachment.cgi?id=419099
Patch

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

>> Source/WebCore/ChangeLog:9
>> +	    user permission granting.
> 
> We should be able to write a permission delegate that delays the user
permission fo ever.
> Either through test runner or an API test.

Hmm, after considering about this I think it's possible. Let me try

>> Source/WebKit/UIProcess/SpeechRecognitionPermissionManager.cpp:95
>> +	if (requestIterator == m_requests.begin() || requestIterator ==
m_requests.end())
> 
> Why do we need both, I would think if (requestIterator == m_requests.end())
should work?

If requestIterator == m_requests.begin(), it means the request already starts
(request being queued gets processed immediately if there is no request ahead).
During requesting handling process, SpeechRecognitionPermissionManager would
just look at the first request in queue and think it's the request being
handled, so here we just let started request run to the end and it will fail in
completion handler.

>> Source/WebKit/UIProcess/SpeechRecognitionServer.cpp:75
>> +	auto permissionRequestIdentifier =
permissionManager->request(request.lang(), request.clientOrigin(),
request.frameIdentifier(), [this, weakThis = makeWeakPtr(this), weakRequest =
makeWeakPtr(request)](auto error) mutable {
> 
> If we pass request directly to permissionManager,
SpeechRecognitionPermissionRequest could take a
WeakPtr<SpeechRecognitionRequest>.
> If its WeakPtr<SpeechRecognitionRequest> is null at the start of processing
the permission request, we could just bail out.
> In that case, SpeechRecognitionServer::cancel would just need to destroy the
request and would not need to call permissionManager->cancelRequest.

That should work too, will change.