[webkit-reviews] review canceled: [Bug 219699] Create a SpeechRecognizer for each SpeechRecognitionRequest : [Attachment 418967] 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 219699: Create a SpeechRecognizer for each SpeechRecognitionRequest
https://bugs.webkit.org/show_bug.cgi?id=219699

Attachment 418967: Patch

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




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

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

>> Source/WebCore/Modules/speech/SpeechRecognition.h:110
>> +	bool m_hasReceivedStart { false };
> 
> It is not clear to me how this relates with a new recogniser per ask. Might
be food to beef up WebCore change log

This is more related to cancel event I guess, will split this patch into two
patches.

>> Source/WebCore/Modules/speech/SpeechRecognitionRequest.h:39
>> +	void
setPermissionRequestIdentifier(Optional<SpeechRecognitionPermissionRequestIdent
ifier> permissionRequestIdentifier) { m_permissionRequestIdentifier =
permissionRequestIdentifier; }
> 
> Why optional? Do we need to set it back to null?

I guess we don't, since SpeechRecognizer will take the ownership of a request
now. I will split the patch and make the recognizer change first.

>> Source/WebCore/Modules/speech/SpeechRecognitionRequest.h:40
>> +	Optional<SpeechRecognitionPermissionRequestIdentifier>
permissionRequestIdentifier() { return m_permissionRequestIdentifier; }
> 
> const.

Okay.

>> Source/WebCore/Modules/speech/SpeechRecognizer.cpp:41
>> +	m_delegateCallback = [this, delegateCallback = WTFMove(callback)](const
SpeechRecognitionUpdate& update) {
> 
> auto&

Okay.

>> Source/WebCore/Modules/speech/SpeechRecognizer.cpp:43
>> +	    delegateCallback(update);
> 
> Is there a possibility delegateCallback kills 'this'?

No, delegate callback is for sending out update now.

>> Source/WebCore/Modules/speech/SpeechRecognizer.cpp:58
>> +void SpeechRecognizer::abort(Optional<SpeechRecognitionError> error)
> 
> Optional<>&& or const Optional<>&

Okay.

>> Source/WebCore/Modules/speech/SpeechRecognizer.cpp:88
>> +void SpeechRecognizer::start(std::unique_ptr<SpeechRecognitionRequest>
request, Ref<RealtimeMediaSource>&& source, bool mockSpeechRecognitionEnabled)
> 
> unique_ptr<>&&, could be a UniqueRef I guess.

Will change.

>> Source/WebCore/Modules/speech/SpeechRecognizer.cpp:90
>> +	m_request = std::exchange(request, nullptr);
> 
> m_request = WTFMove(request)
> Maybe ASSERT(!m_request) as well

Sure.

>> Source/WebKit/UIProcess/SpeechRecognitionPermissionManager.cpp:86
>> +void
SpeechRecognitionPermissionManager::cancelRequest(WebCore::SpeechRecognitionPer
missionRequestIdentifier permissionRequestIdentifier)
> 
> That seems a lot of work.
> I guess you could use removeAllMatching or find/remove instead.

Okay.

>> Source/WebKit/UIProcess/SpeechRecognitionPermissionManager.cpp:95
>> +		continue;
> 
> This does not seem correct: request completion handler will not be called.

Right, will need to call completion handler.

>> Source/WebKit/UIProcess/SpeechRecognitionServer.cpp:91
>> +	if (permissionRequestIdentifier)
> 
> This if does not seem needed right now.

Will change.

>> Source/WebKit/UIProcess/SpeechRecognitionServer.cpp:95
>> +void
SpeechRecognitionServer::handleRequest(std::unique_ptr<WebCore::SpeechRecogniti
onRequest> request)
> 
> unique_ptr<>&&, ideally UniqueRef

Okay.