[webkit-reviews] review canceled: [Bug 221312] Dispatch start and end event in pair for SpeechRecognition : [Attachment 419113] Patch

[bugzilla-daemon at webkit.org]

Sihui Liu <sihui_liu at apple.com> has canceled Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 221312: Dispatch start and end event in pair for SpeechRecognition
https://bugs.webkit.org/show_bug.cgi?id=221312

Attachment 419113: Patch

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




--- Comment #3 from Sihui Liu <sihui_liu at apple.com> ---
Comment on attachment 419113
  --> https://bugs.webkit.org/attachment.cgi?id=419113
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