Bug 221312 – Dispatch start and end event in pair for SpeechRecognition

• Status: NEW
• Component: WebCore Misc.

[Sihui Liu]

...

[Sihui Liu]

Created attachment 419113 [details]
Patch

[youenn fablet]

Comment on attachment 419113 [details]
Patch

Some questions below.
Also, there is no test for it. Can we write some with the existing infra to validate we are firing events appropriately?

If it is not possible right now, I would suggest that we introduce a MockSpeechRecognitionConnection.
And an internals API to set the connection of a SpeechRecognition object to a MockSpeechRecognitionConnection.
Then we can write tests to validate this state handling.
 

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

> Source/WebCore/Modules/speech/SpeechRecognition.cpp:122
> +    m_hasReceivedStart = true;

Should we assert ASSERT(!m_hasReceivedStart);

> Source/WebCore/Modules/speech/SpeechRecognition.cpp:124
>          m_state = State::Running;

If we are not in State::Starting, should we still dispatch the start event?

> Source/WebCore/Modules/speech/SpeechRecognition.cpp:196
> +        reset();

If we received a start, why is it ok to not set back m_state to inactive?
For instance, if access is denied by user, we get an error, but it seems we will keep with a starting state, except if we also get a didEnd.
In that case, calling start will fail as long as we do not reset the state.

[Sihui Liu]

Comment on attachment 419113 [details]
Patch

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

>> Source/WebCore/Modules/speech/SpeechRecognition.cpp:122
>> +    m_hasReceivedStart = true;
> 
> Should we assert ASSERT(!m_hasReceivedStart);

I think we can.

>> Source/WebCore/Modules/speech/SpeechRecognition.cpp:124
>>          m_state = State::Running;
> 
> If we are not in State::Starting, should we still dispatch the start event?

I think the start event is just a signal that server/speech recognition service is connected and starts processing the request, which can happen after use asks to abort/stop because this is an async process; so this is Okay as long as we also dispatch the end event to signal the end.

>> Source/WebCore/Modules/speech/SpeechRecognition.cpp:196
>> +        reset();
> 
> If we received a start, why is it ok to not set back m_state to inactive?
> For instance, if access is denied by user, we get an error, but it seems we will keep with a starting state, except if we also get a didEnd.
> In that case, calling start will fail as long as we do not reset the state.

That's right. The idea here is if we receive a start, there must be an end event from the server to signal that it finishes handling that request. 

Server will automatically abort the request if there is an error, but that does not mean the recognition session ends immediately.
For example, if something is wrong due to audio capture, server can still try sending a recognition result with already captured audio (or sending other events like audio end, error, etc).

In that case if we allow start, it may cause confusion which session the events belong to, since the eventhandlers are reused for sessions/requests

[Radar WebKit Bug Importer]

<rdar://problem/74173731>