Extend PlatformSpeechSynthesizerMock with an utterance queue.

Extend PlatformSpeechSynthesizerMock with an utterance queue.

Clusterfuzz repeatedly runs up against a known limitation of this mock
implementation of PlatformSpeechSynthesizer, that of not being able
to handle the addition of new utterances while one is underway.

Lift the limitation and keep a queue.

R=dmazzoni,haraken
BUG=450142
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189298

[sof, 2015-01-30 16:56:57]

sigbjornf@opera.com changed reviewers:
+ dmazzoni@chromium.org, haraken@chromium.org

[sof, 2015-01-30 16:56:58]

Please take a look.

[haraken, 2015-01-31 01:38:03]

LGTM, but I want to have dmazzoni confirm.

https://codereview.chromium.org/884973003/diff/1/Source/modules/speech/testin...
File Source/modules/speech/testing/PlatformSpeechSynthesizerMock.cpp (right):

https://codereview.chromium.org/884973003/diff/1/Source/modules/speech/testin...
Source/modules/speech/testing/PlatformSpeechSynthesizerMock.cpp:46: ,
m_speakingNextTimer(this, &PlatformSpeechSynthesizerMock::speakingNext)

Why do we need to speakNow here? In the previous code, we didn't schedule
speaking in PlatformSpeechSynthesizerMock's constructor.

https://codereview.chromium.org/884973003/diff/1/Source/modules/speech/testin...
Source/modules/speech/testing/PlatformSpeechSynthesizerMock.cpp:88:
m_speakingNextTimer.startOneShot(.2, FROM_HERE);

I don't know if .2 is good or not though :)

[sof, 2015-01-31 06:06:19]

https://codereview.chromium.org/884973003/diff/1/Source/modules/speech/testin...
File Source/modules/speech/testing/PlatformSpeechSynthesizerMock.cpp (right):

https://codereview.chromium.org/884973003/diff/1/Source/modules/speech/testin...
Source/modules/speech/testing/PlatformSpeechSynthesizerMock.cpp:46: ,
m_speakingNextTimer(this, &PlatformSpeechSynthesizerMock::speakingNext)
On 2015/01/31 01:38:03, haraken wrote:
> 
> Why do we need to speakNow here? In the previous code, we didn't schedule
> speaking in PlatformSpeechSynthesizerMock's constructor.

I don't understand, the ctor just initializes the timers.

[dmazzoni, 2015-01-31 06:51:11]

lgtm

https://codereview.chromium.org/884973003/diff/1/Source/modules/speech/testin...
File Source/modules/speech/testing/PlatformSpeechSynthesizerMock.cpp (right):

https://codereview.chromium.org/884973003/diff/1/Source/modules/speech/testin...
Source/modules/speech/testing/PlatformSpeechSynthesizerMock.cpp:88:
m_speakingNextTimer.startOneShot(.2, FROM_HERE);
On 2015/01/31 01:38:03, haraken wrote:
> 
> I don't know if .2 is good or not though :)

Do we need a delay between the end of one utterance and the beginning of the
next? In the real impl we'd start the next one immediately.

https://codereview.chromium.org/884973003/diff/1/Source/modules/speech/testin...
Source/modules/speech/testing/PlatformSpeechSynthesizerMock.cpp:122: void
PlatformSpeechSynthesizerMock::cancel()
This should remove all utterances from the queue, too.

[sof, 2015-01-31 07:24:24]

https://codereview.chromium.org/884973003/diff/1/Source/modules/speech/testin...
File Source/modules/speech/testing/PlatformSpeechSynthesizerMock.cpp (right):

https://codereview.chromium.org/884973003/diff/1/Source/modules/speech/testin...
Source/modules/speech/testing/PlatformSpeechSynthesizerMock.cpp:88:
m_speakingNextTimer.startOneShot(.2, FROM_HERE);
On 2015/01/31 06:51:11, dmazzoni wrote:
> On 2015/01/31 01:38:03, haraken wrote:
> > 
> > I don't know if .2 is good or not though :)
> 
> Do we need a delay between the end of one utterance and the beginning of the
> next? In the real impl we'd start the next one immediately.

ok, will align with that.

https://codereview.chromium.org/884973003/diff/1/Source/modules/speech/testin...
Source/modules/speech/testing/PlatformSpeechSynthesizerMock.cpp:122: void
PlatformSpeechSynthesizerMock::cancel()
On 2015/01/31 06:51:11, dmazzoni wrote:
> This should remove all utterances from the queue, too.

It should, but doesn't it suffice to do it in speakingErrorOccurred() (as it now
does) ? That would then also include whatever gets added before we dispatch the
'onerror' event when that error timer fires.

[sof, 2015-01-31 07:36:19]

Thanks for the quick reviewing.

Will await clarification on cancel() issue.

https://codereview.chromium.org/884973003/diff/1/Source/modules/speech/testin...
File Source/modules/speech/testing/PlatformSpeechSynthesizerMock.cpp (right):

https://codereview.chromium.org/884973003/diff/1/Source/modules/speech/testin...
Source/modules/speech/testing/PlatformSpeechSynthesizerMock.cpp:88:
m_speakingNextTimer.startOneShot(.2, FROM_HERE);
On 2015/01/31 07:24:24, sof wrote:
> On 2015/01/31 06:51:11, dmazzoni wrote:
> > On 2015/01/31 01:38:03, haraken wrote:
> > > 
> > > I don't know if .2 is good or not though :)
> > 
> > Do we need a delay between the end of one utterance and the beginning of the
> > next? In the real impl we'd start the next one immediately.
> 
> ok, will align with that.

Done, removing the "next" timer that was previously added.

[dmazzoni, 2015-01-31 07:43:31]

https://codereview.chromium.org/884973003/diff/1/Source/modules/speech/testin...
File Source/modules/speech/testing/PlatformSpeechSynthesizerMock.cpp (right):

https://codereview.chromium.org/884973003/diff/1/Source/modules/speech/testin...
Source/modules/speech/testing/PlatformSpeechSynthesizerMock.cpp:122: void
PlatformSpeechSynthesizerMock::cancel()
On 2015/01/31 07:24:24, sof wrote:
> On 2015/01/31 06:51:11, dmazzoni wrote:
> > This should remove all utterances from the queue, too.
> 
> It should, but doesn't it suffice to do it in speakingErrorOccurred() (as it
now
> does) ? That would then also include whatever gets added before we dispatch
the
> 'onerror' event when that error timer fires.

I don't think that's right. If I call cancel() and then immediately call speak()
on a new utterance, it should speak the new utterance - even if it takes time
for the cancel() to succeed, when it does, it should move onto whatever was
enqueued after the cancel.

Perhaps we should have a separate function to force an error, rather than
abusing cancel() for that.

[sof, 2015-01-31 07:56:50]

https://codereview.chromium.org/884973003/diff/1/Source/modules/speech/testin...
File Source/modules/speech/testing/PlatformSpeechSynthesizerMock.cpp (right):

https://codereview.chromium.org/884973003/diff/1/Source/modules/speech/testin...
Source/modules/speech/testing/PlatformSpeechSynthesizerMock.cpp:122: void
PlatformSpeechSynthesizerMock::cancel()
On 2015/01/31 07:43:30, dmazzoni wrote:
> On 2015/01/31 07:24:24, sof wrote:
> > On 2015/01/31 06:51:11, dmazzoni wrote:
> > > This should remove all utterances from the queue, too.
> > 
> > It should, but doesn't it suffice to do it in speakingErrorOccurred() (as it
> now
> > does) ? That would then also include whatever gets added before we dispatch
> the
> > 'onerror' event when that error timer fires.
> 
> I don't think that's right. If I call cancel() and then immediately call
speak()
> on a new utterance, it should speak the new utterance - even if it takes time
> for the cancel() to succeed, when it does, it should move onto whatever was
> enqueued after the cancel.
> 

That makes good sense - switched to that behavior + adjusted the test to verify
that this happens.

> Perhaps we should have a separate function to force an error, rather than
> abusing cancel() for that.

ok, but will not do that here.

[sof, 2015-01-31 08:17:03]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-01-31 08:17:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/884973003/60001

[commit-bot: I haz the power, 2015-01-31 09:30:00]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189298