[webkit-reviews] review denied: [Bug 218476] Implement basic permission check for SpeechRecognition : [Attachment 413782] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Nov 11 09:21:41 PST 2020


Sihui Liu <sihui_liu at apple.com> has denied Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 218476: Implement basic permission check for SpeechRecognition
https://bugs.webkit.org/show_bug.cgi?id=218476

Attachment 413782: Patch

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




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

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

>>> Source/WebKit/UIProcess/Cocoa/UserMediaPermissionRequestManagerProxy.mm:77
>>> +	 if (audioStatus == MediaPermissionResult::Denied) {
>> 
>> videoStatus.
> 
> videoStatus.

NIce catch!

>>> Source/WebKit/UIProcess/Cocoa/UserMediaPermissionRequestManagerProxy.mm:91
>>> +		 });
>> 
>> requestAVCaptureAccessForType(MediaPermissionType::Video,
WTFMove(completionHandler))
> 
> requestAVCaptureAccessForType(MediaPermissionType::Video,
WTFMove(completionHandler))

Sure.

>>> Source/WebKit/UIProcess/Cocoa/UserMediaPermissionRequestManagerProxy.mm:99
>>> +	     });
>> 
>> requestAVCaptureAccessForType(MediaPermissionType::Video, WTFMove(callback))
> 
> requestAVCaptureAccessForType(MediaPermissionType::Video, WTFMove(callback))

Sure.

>>> Source/WebKit/UIProcess/SpeechRecognitionPermissionManager.cpp:84
>>> +	 if (!m_page.preferences().mockCaptureDevicesEnabled()) {
>> 
>> You could simplify this by setting m_microphoneCheck to Granted in case of
m_page.preferences().mockCaptureDevicesEnabled()
> 
> You could simplify this by setting m_microphoneCheck to Granted in case of
m_page.preferences().mockCaptureDevicesEnabled()

Right, since we are resetting it every request now.

>>> Source/WebKit/UIProcess/SpeechRecognitionPermissionManager.cpp:121
>>> +	 if (m_microphoneCheck == CheckResult::Unknown) {
>> 
>> I would probably make this a free function that would return a CheckResult.
>> Then you could write in
SpeechRecognitionPermissionManager::startNextRequest()
>> m_microphoneCheck = computeMicrophoneAccess()
>> Ditto for checkSpeechRecognitionServiceAccess
> 
> I would probably make this a free function that would return a CheckResult.
> Then you could write in
SpeechRecognitionPermissionManager::startNextRequest()
> m_microphoneCheck = computeMicrophoneAccess()
> Ditto for checkSpeechRecognitionServiceAccess

Okay.

>>> Source/WebKit/UIProcess/SpeechRecognitionPermissionManager.h:42
>>> +	 void reset();
>> 
>> No longer defined
> 
> No longer defined

Will remove.

>>> Source/WebKit/UIProcess/SpeechRecognitionPermissionManager.h:62
>>> +	 bool m_isUsingMockedCaptureDevice { false };
>> 
>> m_isUsingMockedCaptureDevice no longer needed
> 
> m_isUsingMockedCaptureDevice no longer needed

will remove.

>>> Source/WebKit/UIProcess/SpeechRecognitionServer.cpp:109
>>> +	 m_permissionChecker(request.clientOrigin(), [this, weakThis =
makeWeakPtr(this), weakRequest =
makeWeakPtr(request)](SpeechRecognitionPermissionDecision decision) mutable {
>> 
>> AIUI, SpeechRecognitionServer is buffering all requests and processing them
one at a time.
>> SpeechRecognitionPermissionManager is also doing the same.
>> One of the buffering might be unneeded if both objects have a one-to-one
mapping.
> 
> AIUI, SpeechRecognitionServer is buffering all requests and processing them
one at a time.
> SpeechRecognitionPermissionManager is also doing the same.
> One of the buffering might be unneeded if both objects have a one-to-one
mapping.

That's true. In that case, we need to make sure either SpeechRecognitionServer
only sends one request to SpeechRecognitionPermissionManager at a time so
SpeechRecognitionPermissionManager does not need to record the check status for
each request (using the queue in SpeechRecognitionPermissionManager), or
SpeechRecognitionServer may run multiple requests at the same time after
permission (using the queue in SpeechRecognitionPermissionManager).

>>> Source/WebKit/UIProcess/WebProcessProxy.cpp:1726
>>> +	     }
>> 
>> It seems like we should not do this search for each permission request.
>> The lambda could take a weakPage for instance.
>> Also, it is a bit weird to compare a pageId and a
SpeechRecognitionServerIdentifier.
>> Maybe the pageId should be also sent through IPC?
> 
> It seems like we should not do this search for each permission request.
> The lambda could take a weakPage for instance.
> Also, it is a bit weird to compare a pageId and a
SpeechRecognitionServerIdentifier.
> Maybe the pageId should be also sent through IPC?

Sure, for simplicity SpeechRecognitionServerIdentifier is actually
WebCore::PageIdentifier right now (using SpeechRecognitionServerIdentifier =
WebCore::PageIdentifier).


More information about the webkit-reviews mailing list