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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Nov 3 06:08:12 PST 2020


youenn fablet <youennf at gmail.com> has granted 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 412997: Patch

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




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

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

> Source/WebKit/ChangeLog:55
> +2020-11-02  Sihui Liu  <sihui_liu at apple.com>

double log

> Source/WebCore/Modules/speech/SpeechRecognition.cpp:53
> +    if (auto* page = document.page()) {

Maybe we could throw if there is no page and make m_connection a Ref<>.
Do you know what other browsers are doing?

> Source/WebCore/Modules/speech/SpeechRecognition.cpp:69
> +    if (!m_connection)

If we make it a ref, we could remove that check.

> Source/WebCore/Modules/speech/SpeechRecognition.cpp:105
> +    dispatchEvent(Event::create(eventNames().startEvent,
Event::CanBubble::No, Event::IsCancelable::No));

I think we should enqueue a task.
There is always the risk that didStart will be called from receiving an IPC
message from UIProcess but document would be in page cache.
The alternative would be to neuter the object when document goes into page
cache.

> Source/WebCore/Modules/speech/SpeechRecognition.h:63
> +    // SpeechRecognitionConnectionClient

Should be private probably

> Source/WebCore/Modules/speech/SpeechRecognitionConnectionClient.h:34
> +struct SpeechRecognitionError;

reverse the two.

> Source/WebCore/Modules/speech/SpeechRecognitionConnectionClient.h:38
> +    explicit SpeechRecognitionConnectionClient()

No need for explicit

> Source/WebCore/Modules/speech/SpeechRecognitionRequest.h:44
> +    SpeechRecognitionRequest(SpeechRecognitionRequestInfo&&);

explicit

> Source/WebCore/Modules/speech/SpeechRecognitionResultData.h:32
> +    double confidence;

Should we set to 0 by default?

> Source/WebCore/Modules/speech/SpeechRecognitionUpdate.cpp:69
> +    return update;

one liner?

> Source/WebCore/Modules/speech/SpeechRecognitionUpdate.cpp:75
> +    return update;

one liner?

> Source/WebCore/page/SpeechRecognitionProvider.h:37
> +    virtual ~SpeechRecognitionProvider() { };

= default to keep it consistent

> Source/WebCore/page/SpeechRecognitionProvider.h:41
> +class DummySpeechRecognitionProvider final : public
WebCore::SpeechRecognitionProvider {

Maybe move it to its own file?

> Source/WebKit/UIProcess/WebProcessProxy.h:399
> +    void destroySpeechRecognitionServer(SpeechRecognitionServerIdentifier);

Could be private?

> Source/WebKit/WebProcess/WebCoreSupport/WebSpeechRecognitionConnection.h:53
> +    void didReceiveUpdate(const WebCore::SpeechRecognitionUpdate&) final;

private?

> Source/WebKit/WebProcess/WebCoreSupport/WebSpeechRecognitionProvider.h:36
> +    WebSpeechRecognitionProvider(WebPage& page)

explicit.
Do we need to take a page reference.
Maybe we can just keep the page ID?

> Tools/DumpRenderTree/win/DumpRenderTree.cpp:924
> +    prefsPrivate->setSpeechRecognitionEnabled(TRUE);

Do we really want it to be TRUE?


More information about the webkit-reviews mailing list