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

[bugzilla-daemon at webkit.org]

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
https://bugs.webkit.org/show_bug.cgi?id=218216

Attachment 412553: Patch

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




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

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
> +	      
alternatives.append(SpeechRecognitionAlternative::create(WTFMove(alternativeDat
a.transcript), alternativeData.confidence));

reserveInitialCapacity and uncheckedAppend removes some branches.

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

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 {{
    WTFMove(*type),
    WTFMove(*message)
}};

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