[webkit-reviews] review denied: [Bug 218216] Set up basic infrastructure for SpeechRecognition : [Attachment 412553] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Oct 29 07:32:35 PDT 2020

Alex Christensen <achristensen at apple.com> has denied Sihui Liu
<sihui_liu at apple.com>'s request for review:
Bug 218216: Set up basic infrastructure for SpeechRecognition

Attachment 412553: Patch


--- Comment #12 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 412553
  --> https://bugs.webkit.org/attachment.cgi?id=412553

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

> Source/WebCore/ChangeLog:12
> +	   one. SpeechRecogntionConnection is currently one per process.

Is there a reason we can only have one per process?  If not, it would probably
be better to have one per Page.

> Source/WebCore/Modules/speech/SpeechRecognition.cpp:164
> +	      
a.transcript), alternativeData.confidence));

reserveInitialCapacity and uncheckedAppend removes some branches.

> Source/WebCore/Modules/speech/SpeechRecognitionConnection.h:60
> +    HashMap<SpeechRecognitionClientIdentifier, SpeechRecognitionClient*>

Could this be WeakPtr<SpeechRecognitionClient> instead of storing raw pointers?

> Source/WebCore/Modules/speech/SpeechRecognitionError.h:32
> +enum class SpeechRecognitionErrorType {

: uint8_t

> Source/WebCore/Modules/speech/SpeechRecognitionError.h:67
> +    return result;

I have a preference that we consistently use the Optional based decoding.


Optional<String> message;
decoder >> message;
if (!message)
    return WTF::nullopt;

return {{

> Source/WebCore/Modules/speech/SpeechRecognizer.h:46
> +    uint64_t m_maxAlternatives;

This and the booleans should have a default initializer because they aren't
initialized in the constructor.

More information about the webkit-reviews mailing list