[webkit-reviews] review denied: [Bug 219459] Implement recognizer for SpeechRecognition : [Attachment 415646] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 219459: Implement recognizer for SpeechRecognition
https://bugs.webkit.org/show_bug.cgi?id=219459

Attachment 415646: Patch

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




--- Comment #9 from Darin Adler <darin at apple.com> ---
Comment on attachment 415646
  --> https://bugs.webkit.org/attachment.cgi?id=415646
Patch

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

> Source/WebCore/Configurations/WebCore.xcconfig:128
> +WK_SPEECH_LDFLAGS = $(WK_SPEECH_LDFLAGS_$(WK_PLATFORM_NAME));
> +WK_SPEECH_LDFLAGS_macosx = -framework Speech;
> +WK_SPEECH_LDFLAGS_iphoneos = -framework Speech;
> +WK_SPEECH_LDFLAGS_iphonesimulator = -framework Speech;
> +WK_SPEECH_LDFLAGS_maccatalyst = -framework Speech;

Is this OK for the base system on macOS? Is the Speech framework included in
that reduced system?

Does this affect launch time? Adding another framework that mostly isn’t used
can slow down launch if that framework is not part of the shared cache, I
believe.

> Source/WebCore/Modules/speech/SpeechRecognizer.cpp:46
> +	   auto error = WebCore::SpeechRecognitionError {
WebCore::SpeechRecognitionErrorType::ServiceNotAllowed, "Failed to start
recognition" };

Should use _s on the string constant here since it’s going to be used to make a
WTF::String.

> Source/WebCore/Modules/speech/SpeechRecognizer.cpp:48
> +	   auto update =
WebCore::SpeechRecognitionUpdate::createError(identifier, error);
> +	   invokeDelegateCallback(update);

Seems like this could be collapsed into a single line like the other similar
cases below.

> Source/WebCore/Modules/speech/SpeechRecognizerCocoa.mm:89
> +	   return nil;

This is missing a call to either "[self release]" or "[self dealloc]".

> Source/WebCore/Modules/speech/SpeechRecognizerCocoa.mm:91
> +    if (![_recognizer.get() isAvailable])

The "get()" here is not needed.

> Source/WebCore/Modules/speech/SpeechRecognizerCocoa.mm:92
> +	   return nil;

This is missing a call to either "[self release]" or "[self dealloc]".

> Source/WebCore/Modules/speech/SpeechRecognizerCocoa.mm:104
> +    [_request _setMaximumRecognitionDuration:60 * 60];

Where does this magic constant come from? Maybe a named constant at the top of
the file would be better. We could also comment how we chose that value.

> Source/WebCore/Modules/speech/SpeechRecognizerCocoa.mm:107
> +    _task = [_recognizer.get() recognitionTaskWithRequest:_request.get()
delegate:self];

The first "get()" here isn't needed.

> Source/WebCore/Modules/speech/SpeechRecognizerCocoa.mm:119
> +	   data.alternatives.append(WebCore::SpeechRecognitionAlternativeData {
[[segment substring] UTF8String], [segment confidence] });

There’s no reason to call UTF8String here. We’re doing to store this into a
WTF::String, and it can do it with NSString. Calling UTF8String makes the
operation less efficient, and I don’t think provides any value.

> Source/WebCore/Modules/speech/SpeechRecognizerCocoa.mm:124
> +	      
data.alternatives.append(WebCore::SpeechRecognitionAlternativeData {
[segmentAlternative UTF8String], 0.0 });

There’s no reason to call UTF8String here. We’re doing to store this into a
WTF::String, and it can do it with NSString. Calling UTF8String makes the
operation less efficient, and I don’t think provides any value.

> Source/WebCore/Modules/speech/SpeechRecognizerCocoa.mm:193
> +    // FIXME: This is never called.

This is a confusing comment. Why is this never called? We say FIXME: What
should someone be fixing?

> Source/WebCore/Modules/speech/SpeechRecognizerCocoa.mm:214
> +    // FIXME: This is never called.

Ditto.

> Source/WebCore/Modules/speech/SpeechRecognizerCocoa.mm:231
> +    auto error = WebCore::SpeechRecognitionError {
WebCore::SpeechRecognitionErrorType::Aborted, [task.error.localizedDescription
UTF8String] };

There’s no reason to call UTF8String here. We’re doing to store this into a
WTF::String, and it can do it with NSString. Calling UTF8String makes the
operation less efficient, and I don’t think provides any value.

> Source/WebCore/Modules/speech/SpeechRecognizerCocoa.mm:233
> +    auto update = WebCore::SpeechRecognitionUpdate::createError(_identifier,
error);
> +    _delegateCallback(update);

I think we could collapse these into a single line.

> Source/WebCore/Modules/speech/SpeechRecognizerCocoa.mm:285
> +    [m_task.get() audioSamplesAvailable:buffer.get()];

The first "get()" here isn’t needed.

> Source/WebCore/Modules/speech/SpeechRecognizerCocoa.mm:304
> +    [m_task.get() stop];

I don’t think this get() is needed.

> Source/WebCore/Modules/speech/SpeechRecognizerCocoa.mm:310
> +    [m_task.get() abort];

Ditto.

> Source/WebCore/Modules/speech/SpeechRecognizerCocoa.mm:319
> +    [task.get() abort];

Ditto.

> Source/WebKit/UIProcess/WebProcessProxy.cpp:1728
> +	   if (!weakPage ||
!weakPage->preferences().mockCaptureDevicesEnabled())
> +	       return false;
> +	   return true;

I would have written this:

    return weakPage && weakPage->preferences().mockCaptureDevicesEnabled();