Bug 219699

<rdar://problem/72392097>

Created attachment 418957 [details]
Patch

Created attachment 418959 [details]
Patch

Created attachment 418967 [details]
Patch

Comment on attachment 418967 [details]
Patch

I am not too clear about what this patch does.
It seems for instance that one thing it can do is to cancel a request that is in the queue to trigger a prompt.
That might indeed be nice to do but it seems like smaller more focused dedicated patches might make things easier.

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

> Source/WebCore/Modules/speech/SpeechRecognitionRequest.h:39
> +    void setPermissionRequestIdentifier(Optional<SpeechRecognitionPermissionRequestIdentifier> permissionRequestIdentifier) { m_permissionRequestIdentifier = permissionRequestIdentifier; }

Why optional? Do we need to set it back to null?

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

const.

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

auto&

> Source/WebCore/Modules/speech/SpeechRecognizer.cpp:43
> +        delegateCallback(update);

Is there a possibility delegateCallback kills 'this'?

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

Optional<>&& or const Optional<>&

> 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.

> Source/WebCore/Modules/speech/SpeechRecognizer.cpp:90
> +    m_request = std::exchange(request, nullptr);

m_request = WTFMove(request)
Maybe ASSERT(!m_request) as well

> Source/WebKit/UIProcess/SpeechRecognitionPermissionManager.cpp:86
> +void SpeechRecognitionPermissionManager::cancelRequest(WebCore::SpeechRecognitionPermissionRequestIdentifier permissionRequestIdentifier)

That seems a lot of work.
I guess you could use removeAllMatching or find/remove instead.

> Source/WebKit/UIProcess/SpeechRecognitionPermissionManager.cpp:95
> +            continue;

This does not seem correct: request completion handler will not be called.

> Source/WebKit/UIProcess/SpeechRecognitionPermissionManager.h:43
> +    Optional<WebCore::SpeechRecognitionPermissionRequestIdentifier> request(const String& lang, const WebCore::ClientOrigin&, WebCore::FrameIdentifier, SpeechRecognitionPermissionRequestCallback&&);

It seems best to provide a WebCore::SpeechRecognitionPermissionRequestIdentifier instead of an Optional.

> Source/WebKit/UIProcess/SpeechRecognitionServer.cpp:91
> +    if (permissionRequestIdentifier)

This if does not seem needed right now.

> Source/WebKit/UIProcess/SpeechRecognitionServer.cpp:95
> +void SpeechRecognitionServer::handleRequest(std::unique_ptr<WebCore::SpeechRecognitionRequest> request)

unique_ptr<>&&, ideally UniqueRef

Comment on attachment 418967 [details]
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<SpeechRecognitionPermissionRequestIdentifier> 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::SpeechRecognitionPermissionRequestIdentifier 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::SpeechRecognitionRequest> request)
> 
> unique_ptr<>&&, ideally UniqueRef

Okay.

Created attachment 419065 [details]
Patch

Created attachment 419066 [details]
Patch

Created attachment 419069 [details]
Patch

Comment on attachment 419069 [details]
Patch

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

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

It might be easier to keep m_delegateCallback as is.
And have this logic in a dedicated method. Something like callDelegate.

> Source/WebCore/Modules/speech/SpeechRecognizer.cpp:48
> +        if (update.type() ==  SpeechRecognitionUpdateType::End)

else if. Or switch

> Source/WebKit/UIProcess/SpeechRecognitionServer.cpp:90
>      }

One liner:
if (m_recognizer)
    m_recognizer->abort(...)

Created attachment 419161 [details]
Patch for landing

Created attachment 419169 [details]
Patch for landing

Patch 419161 does not build

Committed r272337: <https://trac.webkit.org/changeset/272337>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419169 [details].