Bug 217780 – Add stubs for SpeechRecognition

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Sihui Liu]

...

[Radar WebKit Bug Importer]

<rdar://problem/70350727>

[Sihui Liu]

Created attachment 411483 [details]
Patch

[Sihui Liu]

Created attachment 411484 [details]
Patch

[Sihui Liu]

Created attachment 411485 [details]
Patch

[Sihui Liu]

Created attachment 411497 [details]
Patch

[Sihui Liu]

Created attachment 411524 [details]
Patch

[Sihui Liu]

Created attachment 411528 [details]
Patch

[Sihui Liu]

Created attachment 411532 [details]
Patch

[Sihui Liu]

Comment on attachment 411532 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +

Based on https://wicg.github.io/speech-api/#speechreco-section.

[youenn fablet]

Comment on attachment 411532 [details]
Patch

Overall looks good.
Two main comments:
- We might want to add a setting to disable the feature until ready.
- We might want to add some tests for this patch. It seems we could already do a lot of checks like WebIDL.

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

> Source/WebCore/Modules/speech/DOMWindow+SpeechRecognition.idl:31
> +    [ImplementedAs=speechRecognition] readonly attribute SpeechRecognition webkitSpeechRecognition;

Why webkitSpeechRecognition and not speechRecognition?
This is probably not as per spec.

Also, it might be good to add a runtime flag.

> Source/WebCore/Modules/speech/DOMWindowSpeechRecognition.cpp:81
> +    if (page->isClosing())

Could be if (!page || page->isClosing())
I do not see those checks though in some other DOMWindow supplements.
Why are we adding them?

> Source/WebCore/Modules/speech/DOMWindowSpeechRecognition.h:31
> +#include "SpeechRecognition.h"

It seems we could forward declare SpeechRecognition

> Source/WebCore/Modules/speech/SpeechRecognition.cpp:46
> +    return adoptRef(*new SpeechRecognition(document));

Should we add a call to suspendIfNeeded?

> Source/WebCore/Modules/speech/SpeechRecognition.h:58
> +    void stop();

There might be a name conflict with ActiveDOMObject stop() that we might want to implement in the future.
Maybe we should name it stopRecognition and startRecognition as well.

> Source/WebCore/Modules/speech/SpeechRecognition.h:86
> +    const char* activeDOMObjectName() const;

I would add // ActiveDOMObject.
Should be final.
What is our plan for suspend/resume?

> Source/WebCore/Modules/speech/SpeechRecognitionAlternative.cpp:37
> +Ref<SpeechRecognitionAlternative> SpeechRecognitionAlternative::create(const String& transcript, double confidence)

Could be String&& here and constructor

> Source/WebCore/Modules/speech/SpeechRecognitionAlternative.h:31
> +#include <wtf/RefCounted.h>

Might require <wtf/text/WTFString.h> ?

> Source/WebCore/Modules/speech/SpeechRecognitionError.h:46
> +    Optional<SpeechRecognitionErrorType> type;

Why Optional? Should we add a general/unknown error instead?

> Source/WebCore/Modules/speech/SpeechRecognitionRequest.h:39
> +    WEBCORE_EXPORT static Ref<SpeechRecognitionRequest> create(SpeechRecognitionClient*, const String& lang, bool continuous, bool interimResults, uint64_t maxAlternatives);

Can we pass a SpeechRecognitionClient&?

> Source/WebCore/Modules/speech/SpeechRecognitionRequest.h:42
> +    SpeechRecognitionClient* client() const { return m_client.get(); }
> +    SpeechRecognitionRequestData data() const { return m_data; }

const SpeechRecognitionRequestData&

> Source/WebCore/Modules/speech/SpeechRecognitionRequestData.h:32
> +#include "SpeechRecognitionClient.h"

Is this one needed?

> Source/WebCore/Modules/speech/SpeechRecognitionRequestData.h:39
> +*/

To remove?

> Source/WebCore/page/SpeechRecognitionConnection.h:40
> +    virtual void abort(SpeechRecognitionClient*) = 0;

SpeechRecognitionClient& would be better.

> Source/WebKit/WebProcess/WebCoreSupport/WebSpeechRecognitionConnection.cpp:42
> +void WebSpeechRecognitionConnection::start(WebCore::SpeechRecognitionClient* client, const String& lang, bool continuous, bool interimResults, uint64_t maxAlternatives)

SpeechRecognitionClient& here and below

> Source/WebKit/WebProcess/WebCoreSupport/WebSpeechRecognitionConnection.cpp:46
> +    processPendingRequest();

There is a SpeechRecognition/Client per document.
Does this mean that requests should be processed one-by-one on a per page scope or on a per document scope?
It seems this is per page now but I want to confirm this is the right approach.

> Source/WebKit/WebProcess/WebCoreSupport/WebSpeechRecognitionConnection.h:42
> +class WebSpeechRecognitionConnection : public WebCore::SpeechRecognitionConnection {

final?

> Source/WebKit/WebProcess/WebCoreSupport/WebSpeechRecognitionConnection.h:45
> +    WebSpeechRecognitionConnection(WebPage&);

explicit

> Source/WebKit/WebProcess/WebCoreSupport/WebSpeechRecognitionConnection.h:49
> +    void abort(WebCore::SpeechRecognitionClient*) final;

Can be all private?

> Source/WebKit/WebProcess/WebCoreSupport/WebSpeechRecognitionConnection.h:60
> +    Deque<RefPtr<WebCore::SpeechRecognitionRequest>> m_pendingRequests;

Deque<Ref<>>

[Sihui Liu]

Comment on attachment 411532 [details]
Patch

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

>> Source/WebCore/Modules/speech/DOMWindow+SpeechRecognition.idl:31
>> +    [ImplementedAs=speechRecognition] readonly attribute SpeechRecognition webkitSpeechRecognition;
> 
> Why webkitSpeechRecognition and not speechRecognition?
> This is probably not as per spec.
> 
> Also, it might be good to add a runtime flag.

Some sites are using webkit prefix version as it's what originally supported by Blink, so I just added it here. Will add a comment.

Sure, will add a runtime flag.

>> Source/WebCore/Modules/speech/DOMWindowSpeechRecognition.cpp:81
>> +    if (page->isClosing())
> 
> Could be if (!page || page->isClosing())
> I do not see those checks though in some other DOMWindow supplements.
> Why are we adding them?

I may get this from storage code. Taking a deeper look now we may not need this, as long as we stop processing requests correctly when objects are destroyed, so will remove.

>> Source/WebCore/Modules/speech/DOMWindowSpeechRecognition.h:31
>> +#include "SpeechRecognition.h"
> 
> It seems we could forward declare SpeechRecognition

Sure, will move.

>> Source/WebCore/Modules/speech/SpeechRecognition.cpp:46
>> +    return adoptRef(*new SpeechRecognition(document));
> 
> Should we add a call to suspendIfNeeded?

Yes based on the test failure. Will add.

>> Source/WebCore/Modules/speech/SpeechRecognition.h:58
>> +    void stop();
> 
> There might be a name conflict with ActiveDOMObject stop() that we might want to implement in the future.
> Maybe we should name it stopRecognition and startRecognition as well.

Okay, will rename.

>> Source/WebCore/Modules/speech/SpeechRecognition.h:86
>> +    const char* activeDOMObjectName() const;
> 
> I would add // ActiveDOMObject.
> Should be final.
> What is our plan for suspend/resume?

I did not consider about this. Looks like we can abort outstanding request when suspending.

>> Source/WebCore/Modules/speech/SpeechRecognitionAlternative.cpp:37
>> +Ref<SpeechRecognitionAlternative> SpeechRecognitionAlternative::create(const String& transcript, double confidence)
> 
> Could be String&& here and constructor

Okay.

>> Source/WebCore/Modules/speech/SpeechRecognitionAlternative.h:31
>> +#include <wtf/RefCounted.h>
> 
> Might require <wtf/text/WTFString.h> ?

Why?

>> Source/WebCore/Modules/speech/SpeechRecognitionError.h:46
>> +    Optional<SpeechRecognitionErrorType> type;
> 
> Why Optional? Should we add a general/unknown error instead?

Optional here is to decide if this is an error or not... no type means no error. 
As I consider about this now, maybe I should use Optional<SpeechRecognitionError> to check if there is an error, instead of asking SpeechRecognitionError.

>> Source/WebCore/Modules/speech/SpeechRecognitionRequest.h:39
>> +    WEBCORE_EXPORT static Ref<SpeechRecognitionRequest> create(SpeechRecognitionClient*, const String& lang, bool continuous, bool interimResults, uint64_t maxAlternatives);
> 
> Can we pass a SpeechRecognitionClient&?

Sure.

>> Source/WebCore/Modules/speech/SpeechRecognitionRequest.h:42
>> +    SpeechRecognitionRequestData data() const { return m_data; }
> 
> const SpeechRecognitionRequestData&

Okay.

>> Source/WebCore/Modules/speech/SpeechRecognitionRequestData.h:32
>> +#include "SpeechRecognitionClient.h"
> 
> Is this one needed?

Will check and remove if possible.

>> Source/WebCore/Modules/speech/SpeechRecognitionRequestData.h:39
>> +*/
> 
> To remove?

oh I was about to add identifier to request so server and client can verify they are processing the same request.
This may not be necessary but nice to have. Will add.

>> Source/WebCore/page/SpeechRecognitionConnection.h:40
>> +    virtual void abort(SpeechRecognitionClient*) = 0;
> 
> SpeechRecognitionClient& would be better.

Okay.

>> Source/WebKit/WebProcess/WebCoreSupport/WebSpeechRecognitionConnection.cpp:42
>> +void WebSpeechRecognitionConnection::start(WebCore::SpeechRecognitionClient* client, const String& lang, bool continuous, bool interimResults, uint64_t maxAlternatives)
> 
> SpeechRecognitionClient& here and below

Okay.

>> Source/WebKit/WebProcess/WebCoreSupport/WebSpeechRecognitionConnection.cpp:46
>> +    processPendingRequest();
> 
> There is a SpeechRecognition/Client per document.
> Does this mean that requests should be processed one-by-one on a per page scope or on a per document scope?
> It seems this is per page now but I want to confirm this is the right approach.

ah you've pointed out a mistake I made! I revisited the document and found SpeechRecognition is not [SameObject]; it should be multiple SpeechRecognition/Clients per document. Will update that.

I think ultimately an app can only handle one speechrecognition request at a time (like mediarecorder with specified stream?), so maybe it's more appropriate to handle it one-by-one per process. 
I just followed SpeechSynthesis and make it per page. What do you think?

>> Source/WebKit/WebProcess/WebCoreSupport/WebSpeechRecognitionConnection.h:42
>> +class WebSpeechRecognitionConnection : public WebCore::SpeechRecognitionConnection {
> 
> final?

Okay.

>> Source/WebKit/WebProcess/WebCoreSupport/WebSpeechRecognitionConnection.h:45
>> +    WebSpeechRecognitionConnection(WebPage&);
> 
> explicit

sure.

>> Source/WebKit/WebProcess/WebCoreSupport/WebSpeechRecognitionConnection.h:49
>> +    void abort(WebCore::SpeechRecognitionClient*) final;
> 
> Can be all private?

Will move.

>> Source/WebKit/WebProcess/WebCoreSupport/WebSpeechRecognitionConnection.h:60
>> +    Deque<RefPtr<WebCore::SpeechRecognitionRequest>> m_pendingRequests;
> 
> Deque<Ref<>>

Okay.

[Sihui Liu]

(In reply to youenn fablet from comment #10)
> Comment on attachment 411532 [details]
> Patch
> 
> Overall looks good.
> Two main comments:
> - We might want to add a setting to disable the feature until ready.
> - We might want to add some tests for this patch. It seems we could already
> do a lot of checks like WebIDL.
> 
Will add feature flag and tests.

[Sihui Liu]

Created attachment 411727 [details]
Patch

[Sihui Liu]

Created attachment 411754 [details]
Patch

[Sihui Liu]

Created attachment 411764 [details]
Patch

[Sam Weinig]

Comment on attachment 411764 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Based on https://wicg.github.io/speech-api/#speechreco-section.

I think it would be helpful to write up the major design decisions that when into what are implementing here, such as a high level overview of the design, where the binding to platform speech recognition will go, etc.

> Source/WebCore/Modules/speech/SpeechRecognitionAlternative.idl:30
> +] interface SpeechRecognitionAlternative {

Should this also be guarded by EnabledBySetting=SpeechRecognition?

> Source/WebCore/Modules/speech/SpeechRecognitionErrorEvent.idl:29
> +] interface SpeechRecognitionErrorEvent : Event {

Should this also be guarded by EnabledBySetting=SpeechRecognition?

> Source/WebCore/Modules/speech/SpeechRecognitionEvent.idl:29
> +] interface SpeechRecognitionEvent : Event {

Should this also be guarded by EnabledBySetting=SpeechRecognition?

> Source/WebCore/Modules/speech/SpeechRecognitionProgressUpdate.h:69
> +    Optional<SpeechRecognitionError> m_error;
> +    Optional<Vector<SpeechRecognitionResultData>> m_results;

If these are mutually exclusive, this should probably be something like Variant<WTF::Monostate, SpeechRecognitionError, Vector<SpeechRecognitionResultData>>

> Source/WebCore/Modules/speech/SpeechRecognitionResult.idl:30
> +] interface SpeechRecognitionResult {

Should this also be guarded by EnabledBySetting=SpeechRecognition?

> Source/WebCore/Modules/speech/SpeechRecognitionResultList.idl:30
> +] interface SpeechRecognitionResultList {

Should this also be guarded by EnabledBySetting=SpeechRecognition?

> Source/WebKitLegacy/mac/WebView/WebPreferencesPrivate.h:351
> +@property (nonatomic) BOOL speechRecognitionEnabled;

It doesn't look like you hooked up a provider for WebKitLegacy, why add preferences for it?

> LayoutTests/TestExpectations:79
> +speech [ Skip ]

Why skip these by default? If we can keep these passing on all platforms that would be much better.

[Sam Weinig]

There other thing it would be good to mention / figure out up front is how you intended to test this. It seems like a MockSpeechRecognitionProvider of some form might be the right strategy?

[Sihui Liu]

(In reply to Sam Weinig from comment #17)
> There other thing it would be good to mention / figure out up front is how
> you intended to test this. It seems like a MockSpeechRecognitionProvider of
> some form might be the right strategy?

(In reply to Sam Weinig from comment #16)
> Comment on attachment 411764 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411764&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Based on https://wicg.github.io/speech-api/#speechreco-section.
> 
> I think it would be helpful to write up the major design decisions that when
> into what are implementing here, such as a high level overview of the
> design, where the binding to platform speech recognition will go, etc.
> 

Sure, will add. I am going to break this patch down to smaller ones to make the design more clear.

> > Source/WebCore/Modules/speech/SpeechRecognitionAlternative.idl:30
> > +] interface SpeechRecognitionAlternative {
> 
> Should this also be guarded by EnabledBySetting=SpeechRecognition?
> 

Yes!

> > Source/WebCore/Modules/speech/SpeechRecognitionErrorEvent.idl:29
> > +] interface SpeechRecognitionErrorEvent : Event {
> 
> Should this also be guarded by EnabledBySetting=SpeechRecognition?
> 

Yes!

> > Source/WebCore/Modules/speech/SpeechRecognitionEvent.idl:29
> > +] interface SpeechRecognitionEvent : Event {
> 
> Should this also be guarded by EnabledBySetting=SpeechRecognition?
> 

Yes!

> > Source/WebCore/Modules/speech/SpeechRecognitionProgressUpdate.h:69
> > +    Optional<SpeechRecognitionError> m_error;
> > +    Optional<Vector<SpeechRecognitionResultData>> m_results;
> 
> If these are mutually exclusive, this should probably be something like
> Variant<WTF::Monostate, SpeechRecognitionError,
> Vector<SpeechRecognitionResultData>>
> 

Will try using Variant.

> > Source/WebCore/Modules/speech/SpeechRecognitionResult.idl:30
> > +] interface SpeechRecognitionResult {
> 
> Should this also be guarded by EnabledBySetting=SpeechRecognition?
> 

Yes!

> > Source/WebCore/Modules/speech/SpeechRecognitionResultList.idl:30
> > +] interface SpeechRecognitionResultList {
> 
> Should this also be guarded by EnabledBySetting=SpeechRecognition?
> 

Yes!

> > Source/WebKitLegacy/mac/WebView/WebPreferencesPrivate.h:351
> > +@property (nonatomic) BOOL speechRecognitionEnabled;
> 
> It doesn't look like you hooked up a provider for WebKitLegacy, why add
> preferences for it?
> 

I think this will be implemented later in WebKitLegacy so I added this preference. I will move the implementation to followup patches and only do bindings & feature flag in this patch.

> > LayoutTests/TestExpectations:79
> > +speech [ Skip ]
> 
> Why skip these by default? If we can keep these passing on all platforms
> that would be much better.

It seems we only have speech service on Cocoa platforms now, so I only enable the tests on on Cocoa platforms and skip on the others.

[Sihui Liu]

(In reply to Sam Weinig from comment #17)
> There other thing it would be good to mention / figure out up front is how
> you intended to test this. It seems like a MockSpeechRecognitionProvider of
> some form might be the right strategy?

Yes, that's how we did for SpeechSynthesis.

[Sihui Liu]

Created attachment 411820 [details]
Patch

[Sam Weinig]

(In reply to Sihui Liu from comment #18)

> 
> > > LayoutTests/TestExpectations:79
> > > +speech [ Skip ]
> > 
> > Why skip these by default? If we can keep these passing on all platforms
> > that would be much better.
> 
> It seems we only have speech service on Cocoa platforms now, so I only
> enable the tests on on Cocoa platforms and skip on the others.

It would be much better if you made the tests work for all platforms, mocking out the actual speech recognition. We don't really need to test the platform's speech recognition at all, that's the job of the platform library, but rather you need to test the binding within WebKit and WebCore, and that should almost entirely be cross-platform, so there shouldn't be any reason not to make the tests cross platform from the start.

[Sihui Liu]

(In reply to Sam Weinig from comment #21)
> (In reply to Sihui Liu from comment #18)
> 
> > 
> > > > LayoutTests/TestExpectations:79
> > > > +speech [ Skip ]
> > > 
> > > Why skip these by default? If we can keep these passing on all platforms
> > > that would be much better.
> > 
> > It seems we only have speech service on Cocoa platforms now, so I only
> > enable the tests on on Cocoa platforms and skip on the others.
> 
> It would be much better if you made the tests work for all platforms,
> mocking out the actual speech recognition. We don't really need to test the
> platform's speech recognition at all, that's the job of the platform
> library, but rather you need to test the binding within WebKit and WebCore,
> and that should almost entirely be cross-platform, so there shouldn't be any
> reason not to make the tests cross platform from the start.

I see, you are talking about the test for API existence. We did not compile ( ENABLE_SPEECH_SYNTHESIS is 0) or expose SpeechSynthesis or run corresponding tests on non-Cocoa platforms, so I made SpeechRecognition to match that for consistency. We can remove the macros together later.

Or do you think it's better to not add the macro for SpeechRecognition since it's newly added?

[Sihui Liu]

Created attachment 411840 [details]
Patch

[Sihui Liu]

Created attachment 411843 [details]
Patch

[Sam Weinig]

(In reply to Sihui Liu from comment #22)
> (In reply to Sam Weinig from comment #21)
> > (In reply to Sihui Liu from comment #18)
> > 
> > > 
> > > > > LayoutTests/TestExpectations:79
> > > > > +speech [ Skip ]
> > > > 
> > > > Why skip these by default? If we can keep these passing on all platforms
> > > > that would be much better.
> > > 
> > > It seems we only have speech service on Cocoa platforms now, so I only
> > > enable the tests on on Cocoa platforms and skip on the others.
> > 
> > It would be much better if you made the tests work for all platforms,
> > mocking out the actual speech recognition. We don't really need to test the
> > platform's speech recognition at all, that's the job of the platform
> > library, but rather you need to test the binding within WebKit and WebCore,
> > and that should almost entirely be cross-platform, so there shouldn't be any
> > reason not to make the tests cross platform from the start.
> 
> I see, you are talking about the test for API existence. We did not compile
> ( ENABLE_SPEECH_SYNTHESIS is 0) or expose SpeechSynthesis or run
> corresponding tests on non-Cocoa platforms, so I made SpeechRecognition to
> match that for consistency. We can remove the macros together later.
> 
> Or do you think it's better to not add the macro for SpeechRecognition since
> it's newly added?

Yeah, if you have a runtime flag, I think we should avoid ENABLE_SPEECH_SYNTHESIS style macros where we can. The fewer differences in testing across the platforms we can have, the better.

[Sihui Liu]

Created attachment 411882 [details]
Patch

[Sihui Liu]

(In reply to Sam Weinig from comment #25)
> (In reply to Sihui Liu from comment #22)
> > (In reply to Sam Weinig from comment #21)
> > > (In reply to Sihui Liu from comment #18)
> > > 
> > > > 
> > > > > > LayoutTests/TestExpectations:79
> > > > > > +speech [ Skip ]
> > > > > 
> > > > > Why skip these by default? If we can keep these passing on all platforms
> > > > > that would be much better.
> > > > 
> > > > It seems we only have speech service on Cocoa platforms now, so I only
> > > > enable the tests on on Cocoa platforms and skip on the others.
> > > 
> > > It would be much better if you made the tests work for all platforms,
> > > mocking out the actual speech recognition. We don't really need to test the
> > > platform's speech recognition at all, that's the job of the platform
> > > library, but rather you need to test the binding within WebKit and WebCore,
> > > and that should almost entirely be cross-platform, so there shouldn't be any
> > > reason not to make the tests cross platform from the start.
> > 
> > I see, you are talking about the test for API existence. We did not compile
> > ( ENABLE_SPEECH_SYNTHESIS is 0) or expose SpeechSynthesis or run
> > corresponding tests on non-Cocoa platforms, so I made SpeechRecognition to
> > match that for consistency. We can remove the macros together later.
> > 
> > Or do you think it's better to not add the macro for SpeechRecognition since
> > it's newly added?
> 
> Yeah, if you have a runtime flag, I think we should avoid
> ENABLE_SPEECH_SYNTHESIS style macros where we can. The fewer differences in
> testing across the platforms we can have, the better.

Okay, remove the ENABLE_SPEECH_RECOGNITION macro.

[Sihui Liu]

Created attachment 411883 [details]
Patch

[youenn fablet]

Comment on attachment 411883 [details]
Patch

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

> Source/WebCore/Modules/speech/SpeechRecognition.h:41
> +    void setLang(const String& lang) { m_lang = lang; }

String&& probably and WFMOve

> Source/WebCore/Modules/speech/SpeechRecognition.h:60
> +    SpeechRecognition(Document&);

explicit

> Source/WebCore/Modules/speech/SpeechRecognitionAlternative.cpp:41
> +    : m_transcript(transcript)

WTFMove()

> Source/WebCore/Modules/speech/SpeechRecognitionAlternative.h:39
> +    String transcript() const { return m_transcript; }

const String&?

> Source/WebCore/Modules/speech/SpeechRecognitionAlternative.h:43
> +    explicit SpeechRecognitionAlternative(String&& transcript, double confidence);

explicit not needed

> Source/WebCore/Modules/speech/SpeechRecognitionErrorEvent.cpp:35
> +Ref<SpeechRecognitionErrorEvent> SpeechRecognitionErrorEvent::create(const AtomString& type, const Init& init, IsTrusted isTrusted)

Could probably have Init&& to be able to move m_message

> Source/WebCore/Modules/speech/SpeechRecognitionErrorEvent.h:45
> +    String message() const { return m_message; }

const String&?

> Source/WebCore/Modules/speech/SpeechRecognitionErrorEvent.h:51
> +    EventInterface eventInterface() const override;

final

> Source/WebCore/Modules/speech/SpeechRecognitionEvent.cpp:46
> +SpeechRecognitionEvent::SpeechRecognitionEvent(const AtomString& type, const Init& init, IsTrusted isTrusted)

Init&&

> Source/WebCore/Modules/speech/SpeechRecognitionEvent.cpp:49
> +    , m_results(init.results)

WTFMove()

> Source/WebCore/Modules/speech/SpeechRecognitionEvent.h:52
> +    EventInterface eventInterface() const override;

final

> Source/WebCore/Modules/speech/SpeechRecognitionEvent.h:55
> +    RefPtr<SpeechRecognitionResultList> m_results;

Can it be a Ref<>? It seems results is required so we could mandate it even though the struct is a RefPtr.

> Source/WebCore/Modules/speech/SpeechRecognitionResultList.h:48
> +    SpeechRecognitionResultList(Vector<Ref<SpeechRecognitionResult>>&&);

explicit

> Source/WebCore/Modules/webaudio/AudioWorkletNode.h:35
> +// #include <JavaScriptCore/JSGlobalObject.h>

Not needed

> Source/WebKitLegacy/mac/WebView/WebPreferencesPrivate.h:351
> +@property (nonatomic) BOOL speechRecognitionEnabled;

Should it be _speechRecognitionEnabled?

> Tools/DumpRenderTree/TestOptions.h:67
> +    bool enableSpeechRecognition { true };

Unneeded?

[Sihui Liu]

Comment on attachment 411883 [details]
Patch

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

>> Source/WebCore/Modules/speech/SpeechRecognition.h:41
>> +    void setLang(const String& lang) { m_lang = lang; }
> 
> String&& probably and WFMOve

Will change.

>> Source/WebCore/Modules/speech/SpeechRecognition.h:60
>> +    SpeechRecognition(Document&);
> 
> explicit

Okay.

>> Source/WebCore/Modules/speech/SpeechRecognitionAlternative.cpp:41
>> +    : m_transcript(transcript)
> 
> WTFMove()

Okay.

>> Source/WebCore/Modules/speech/SpeechRecognitionAlternative.h:39
>> +    String transcript() const { return m_transcript; }
> 
> const String&?

Will change.

>> Source/WebCore/Modules/speech/SpeechRecognitionAlternative.h:43
>> +    explicit SpeechRecognitionAlternative(String&& transcript, double confidence);
> 
> explicit not needed

Okay.

>> Source/WebCore/Modules/speech/SpeechRecognitionErrorEvent.cpp:35
>> +Ref<SpeechRecognitionErrorEvent> SpeechRecognitionErrorEvent::create(const AtomString& type, const Init& init, IsTrusted isTrusted)
> 
> Could probably have Init&& to be able to move m_message

Will change.

>> Source/WebCore/Modules/speech/SpeechRecognitionErrorEvent.h:45
>> +    String message() const { return m_message; }
> 
> const String&?

Okay.

>> Source/WebCore/Modules/speech/SpeechRecognitionErrorEvent.h:51
>> +    EventInterface eventInterface() const override;
> 
> final

Will change.

>> Source/WebCore/Modules/speech/SpeechRecognitionEvent.cpp:46
>> +SpeechRecognitionEvent::SpeechRecognitionEvent(const AtomString& type, const Init& init, IsTrusted isTrusted)
> 
> Init&&

Will change.

>> Source/WebCore/Modules/speech/SpeechRecognitionEvent.cpp:49
>> +    , m_results(init.results)
> 
> WTFMove()

Okay.

>> Source/WebCore/Modules/speech/SpeechRecognitionEvent.h:52
>> +    EventInterface eventInterface() const override;
> 
> final

Will change.

>> Source/WebCore/Modules/speech/SpeechRecognitionEvent.h:55
>> +    RefPtr<SpeechRecognitionResultList> m_results;
> 
> Can it be a Ref<>? It seems results is required so we could mandate it even though the struct is a RefPtr.

https://wicg.github.io/speech-api/#eventdef-speechrecognition-nomatch says the results can be null for nomatch event.

>> Source/WebCore/Modules/speech/SpeechRecognitionResultList.h:48
>> +    SpeechRecognitionResultList(Vector<Ref<SpeechRecognitionResult>>&&);
> 
> explicit

Will add.

>> Source/WebCore/Modules/webaudio/AudioWorkletNode.h:35
>> +// #include <JavaScriptCore/JSGlobalObject.h>
> 
> Not needed

Yes.

>> Source/WebKitLegacy/mac/WebView/WebPreferencesPrivate.h:351
>> +@property (nonatomic) BOOL speechRecognitionEnabled;
> 
> Should it be _speechRecognitionEnabled?

hmm, since it's only in DumpRenderTree now so I think yes.

>> Tools/DumpRenderTree/TestOptions.h:67
>> +    bool enableSpeechRecognition { true };
> 
> Unneeded?

Yes.

[Sihui Liu]

Created attachment 411900 [details]
Patch

[Sihui Liu]

Created attachment 411910 [details]
Patch

[EWS]

Committed r268762: <https://trac.webkit.org/changeset/268762>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411910 [details].

[Sihui Liu]

Reopening to attach new patch.

[Sihui Liu]

Created attachment 411943 [details]
Patch for landing

[EWS]

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

[Sihui Liu]

Created attachment 411944 [details]
Patch for landing

[EWS]

Committed r268780: <https://trac.webkit.org/changeset/268780>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411944 [details].

[Darin Adler]

Comment on attachment 411944 [details]
Patch for landing

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

> Source/WebCore/Modules/speech/SpeechRecognitionResultList.h:41
> +    unsigned long length() const { return m_list.size(); }

unsigned makes sense, maybe size_t, but unsigned long does not seem right

[Sihui Liu]

(In reply to Darin Adler from comment #39)
> Comment on attachment 411944 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411944&action=review
> 
> > Source/WebCore/Modules/speech/SpeechRecognitionResultList.h:41
> > +    unsigned long length() const { return m_list.size(); }
> 
> unsigned makes sense, maybe size_t, but unsigned long does not seem right

oops, copied unsigned long from the idl file. Will fix this in next related patch.

[Sam Weinig]

(In reply to Sihui Liu from comment #40)
> (In reply to Darin Adler from comment #39)
> > Comment on attachment 411944 [details]
> > Patch for landing
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=411944&action=review
> > 
> > > Source/WebCore/Modules/speech/SpeechRecognitionResultList.h:41
> > > +    unsigned long length() const { return m_list.size(); }
> > 
> > unsigned makes sense, maybe size_t, but unsigned long does not seem right
> 
> oops, copied unsigned long from the idl file. Will fix this in next related
> patch.

Note, that IDL "long" maps to C "int" - https://heycam.github.io/webidl/#es-long