[webkit-reviews] review granted: [Bug 221082] Use user media permission prompt for speech recognition : [Attachment 418799] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 221082: Use user media permission prompt for speech recognition
https://bugs.webkit.org/show_bug.cgi?id=221082

Attachment 418799: Patch

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




--- Comment #14 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 418799
  --> https://bugs.webkit.org/attachment.cgi?id=418799
Patch

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

> Source/WebCore/Modules/speech/SpeechRecognition.cpp:79
> +    auto frameIdentifier = optionalFrameIdentifier ?
*optionalFrameIdentifier : FrameIdentifier { };

We probably do not need to check frame.
We could do:
auto frameID = document.frameID();
if (!frameID)
    return Exception { InvalidStateError, "Recognition is not in a valid
frame"_s };
...

> Source/WebKit/UIProcess/SpeechRecognitionPermissionManager.cpp:78
> +void SpeechRecognitionPermissionManager::request(const String& lang, const
WebCore::ClientOrigin& origin, const WebCore::FrameIdentifier frameIdentifier,
SpeechRecognitionPermissionRequestCallback&& completiontHandler)

s/const WebCore::FrameIdentifier/WebCore::FrameIdentifier/
Ditto below.
Or is it to make sure it stays const?

> Source/WebKit/UIProcess/SpeechRecognitionPermissionRequest.h:65
> +    const WebCore::FrameIdentifier m_frameIdentifier;

Ditto here.

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:244
> +	   callback(true);

We will miss device ID exposure which is done in finishGrantingRequest.
I guess we should fix this in a refactoring.

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:616
> +void
UserMediaPermissionRequestManagerProxy::checkUserMediaPermissionForSpeechRecogn
ition(const WebCore::FrameIdentifier frameIdentifier, const
WebCore::SecurityOrigin& requestingOrigin, const WebCore::SecurityOrigin&
topOrigin, const WebCore::CaptureDevice& device,
CompletionHandler<void(bool)>&& completionHandler)

ditto for const WebCore::FrameIdentifier

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:632
> +    }

Can we replace wasRequestDenied and searchForGrantedRequest with
getRequestAction().
We will miss the ability of being able to reprompt user if speech recognition
is made as part of a user gesture.

> Source/WebKit/UIProcess/UserMediaPermissionRequestProxy.cpp:44
> +    , m_decisionCompletionHandler(WTFMove(decisionCompletionHandler))

We probably need to call this completion handler in
UserMediaPermissionRequestManagerProxy::invalidatePendingRequests() or
UserMediaPermissionRequestProxy::invalidate().

> Source/WebKit/UIProcess/WebPageProxy.cpp:10418
> +void WebPageProxy::requestUserMediaPermissionForSpeechRecognition(const
WebCore::FrameIdentifier frameIdentifier, const WebCore::SecurityOrigin&
requestingOrigin, const WebCore::SecurityOrigin& topOrigin,
CompletionHandler<void(bool)>&& completionHandler)

const WebCore::FrameIdentifier?