[webkit-reviews] review granted: [Bug 81096] Speech JavaScript API: SpeechRecognition, Controller and Client : [Attachment 131830] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Hans Wennborg <hans at chromium.org>'s
request for review:
Bug 81096: Speech JavaScript API: SpeechRecognition, Controller and Client
https://bugs.webkit.org/show_bug.cgi?id=81096

Attachment 131830: Patch
https://bugs.webkit.org/attachment.cgi?id=131830&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=131830&action=review


I'm marking this patch R+, but you seem to have a bunch of unnecessary
boilerplate in SpeechRecognition.  Maybe there's a reason for it that I don't
understand?  Please consider the comments below before landing.  Thanks!

> Source/WebCore/Modules/speech/SpeechRecognition.cpp:68
> +void SpeechRecognition::audioStartCallback()
> +{
> +    fireAudioStart();
> +}

What's the point of having these thunks?  Why not just have the
SpeechRecognitionClient call dispatchAudioStart directly?

> Source/WebCore/Modules/speech/SpeechRecognition.cpp:135
> +bool SpeechRecognition::canSuspend() const
> +{
> +    // FIXME: Implement!
> +    return false;
> +}
> +
> +void SpeechRecognition::stop()
> +{
> +    // FIXME: Implement!
> +}
> +
> +bool SpeechRecognition::hasPendingActivity() const
> +{
> +    // FIXME: Implement!
> +    return false;
> +}

Do you need to override these?	Why not just inherit?

> Source/WebCore/Modules/speech/SpeechRecognition.cpp:146
> +ScriptExecutionContext* SpeechRecognition::scriptExecutionContext() const
> +{
> +    return ActiveDOMObject::scriptExecutionContext();
> +}
> +

Do you need to override this?  Why not just inherit?

> Source/WebCore/Modules/speech/SpeechRecognition.cpp:167
> +    m_controller->unregisterSpeechRecognition(this);

I was expecting a corresponding registerSpeechRecognition call.  Is that not
needed?

Please consider unregistering yourself in SpeechRecognition::stop() instead. 
SpeechRecognition::stop() is called at a deterministic time, but
SpeechRecognition::~SpeechRecognition is call non-deterministically by the
garbage collector.  Generally, we prefer web-facing APIs to behave as
deterministically as possible so we don't get content that expects the current
quirks of a garbage collector.

> Source/WebCore/Modules/speech/SpeechRecognition.cpp:170
> +void SpeechRecognition::fireAudioStart()

Normally we'd call these functions "dispatchAudioStart".