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

[bugzilla-daemon at webkit.org]

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 413699: Patch

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




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

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

>> Source/WebCore/Modules/speech/SpeechRecognitionRequest.h:43
>> +	ClientOrigin clientOrigin() const { return m_info.clientOrigin; }
> 
> could be const&

Okay.

>> Source/WebKit/UIProcess/Cocoa/MediaPermissionUtilities.mm:93
>> +	if (type == MediaPermissionType::Audio) {
> 
> switch statement

Okay.

>> Source/WebKit/UIProcess/Cocoa/MediaPermissionUtilities.mm:106
>> +	if (type == MediaPermissionType::Audio) {
> 
> Should be a switch statement

Sure.

>> Source/WebKit/UIProcess/Cocoa/SpeechRecognitionPermissionManagerCocoa.mm:44
>> +void SpeechRecognitionPermissionManager::startProcessingRequest()
> 
> It seems most of SpeechRecognitionPermissionManagerCocoa.mm could go to a cpp
file (either SpeechRecognitionPermissionManagerCocoa.mm or
SpeechRecognitionPermissionManager.cpp).

Okay.

>> Source/WebKit/UIProcess/Cocoa/SpeechRecognitionPermissionManagerCocoa.mm:101
>> +void
SpeechRecognitionPermissionManager::requestSpeechRecognitionServiceAccess()
> 
> I would tend to move checkSpeechRecognitionServiceAccess and
requestSpeechRecognitionServiceAccess as free function into
MediaPermissionUtilities.h/.mm

Okay.

>> Source/WebKit/UIProcess/Cocoa/SpeechRecognitionPermissionManagerCocoa.mm:154
>> +void SpeechRecognitionPermissionManager::requestUserPermission()
> 
> This function in particular would best be in the general cpp file.

Okay.

>> Source/WebKit/UIProcess/SpeechRecognitionPermissionManager.cpp:39
>> +	    request->complete(SpeechRecognitionPermissionDecision::Deny);
> 
> I would expect reset to do that and clear m_requests

Will move to reset

>> Source/WebKit/UIProcess/SpeechRecognitionPermissionManager.cpp:84
>> +	ASSERT(!m_requests.isEmpty());
> 
> If there is a reset in between that clears requests, you might need to do a
check here instead of an assert.

hmm, in this case it seems easier to recreate a new
SpeechRecognitionPermissionManager than reset, because we might: append a
request ->  start the request -> reset -> append a new request(only request in
queue) -> receive decision started request; then a wrong request will be
completed.

>> Source/WebKit/UIProcess/SpeechRecognitionPermissionManager.cpp:93
>> +	m_userPermissionCheck = CheckResult::Unknown;
> 
> If you reset, you might also need to reset all queued requests.

will remove reset() and creating a new SpeechRecognitionPermissionManager for
reseting.

>> Source/WebKit/UIProcess/SpeechRecognitionPermissionManager.h:60
>> +	CheckResult m_speechRecognitionServiceCheck { CheckResult::Unknown };
> 
> I wonder whether a user might be able to change the TCC setting from Granted
to Denied or Denied to Granted during the lifetime of a page.
> In such a case, we might need to do these two checks for every request like
we do for getUserMedia.

Looks like it's possible. There is a tccutil command line tool for resetting.
Will remove these variables.

>> LayoutTests/fast/speechrecognition/permission-error.html:10
>> +	testRunner.setIsSpeechRecognitionPermissionGranted(false);
> 
> We could add some tests for the persistency.
> Say do a request with permission granted, change it to deny but still able to
proceed.
> Maybe a layout test or API test showing that on page reload, we ask again the
delegate.

Okay, will add.