[webkit-reviews] review granted: [Bug 107135] WebSpeech: Implement basic speaking/finished speaking behavior : [Attachment 187496] patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted chris fleizach <cfleizach at apple.com>'s
request for review:
Bug 107135: WebSpeech: Implement basic speaking/finished speaking behavior
https://bugs.webkit.org/show_bug.cgi?id=107135

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

------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=187496&action=review


> Source/WebCore/Modules/speech/SpeechSynthesis.cpp:81
> +    m_isSpeaking = true;

Should this ASSERT(!m_isSpeaking)?

> Source/WebCore/Modules/speech/SpeechSynthesis.cpp:82
> +    utterance->setStartTime(currentTime());

It might make sense for this to use monotonicallyIncreasingTime() instead.

> Source/WebCore/Modules/speech/SpeechSynthesis.cpp:92
> +    if (m_utteranceQueue.size() == 1)
> +	   startSpeakingImmediately(utterance);

One thing that might make this a bit clearer, is if you pull the utterence off
the queue here, and put it in a "currentUtterence" member (then you might be
able to get rid of m_isSpeaking).

> Source/WebCore/Modules/speech/SpeechSynthesis.cpp:116
> +    m_isSpeaking = false;

Should this ASSERT(m_isSpeaking)?

> Source/WebCore/Modules/speech/SpeechSynthesis.cpp:126
> +    if (m_utteranceQueue.size())

You could use isEmpty() here to be a bit clearer.

> Source/WebCore/Modules/speech/SpeechSynthesis.h:75
> +    Vector<RefPtr<SpeechSynthesisUtterance> > m_utteranceQueue;

A Deque might be a bit more efficient.