Add support for onvoiceschanged event.

Add support for onvoiceschanged event.

This is a recent addition to the spec: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22003

BUG=171887
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=159914

[eseidel, 2013-10-16 06:44:52]

https://codereview.chromium.org/27477002/diff/1/Source/modules/speech/SpeechS...
File Source/modules/speech/SpeechSynthesis.cpp (right):

https://codereview.chromium.org/27477002/diff/1/Source/modules/speech/SpeechS...
Source/modules/speech/SpeechSynthesis.cpp:64:
dispatchEvent(Event::create(EventTypeNames::voiceschanged));
This will cause synchronous event dispatch, right?  Callers may have the frame
navigated or othewise things deleted out from under them.

[dmazzoni, 2013-10-16 07:08:00]

https://codereview.chromium.org/27477002/diff/1/Source/modules/speech/SpeechS...
File Source/modules/speech/SpeechSynthesis.cpp (right):

https://codereview.chromium.org/27477002/diff/1/Source/modules/speech/SpeechS...
Source/modules/speech/SpeechSynthesis.cpp:64:
dispatchEvent(Event::create(EventTypeNames::voiceschanged));
On 2013/10/16 06:44:53, eseidel wrote:
> This will cause synchronous event dispatch, right?  Callers may have the frame
> navigated or othewise things deleted out from under them.

That's only a problem if the user is triggering this function, right? The only
caller of voicesDidChange is the platform, and that's only in response to IPC
messages from the browser process, handled in the main loop.

[abarth-chromium, 2013-10-16 19:25:06]

https://codereview.chromium.org/27477002/diff/1/Source/modules/speech/SpeechS...
File Source/modules/speech/SpeechSynthesis.cpp (right):

https://codereview.chromium.org/27477002/diff/1/Source/modules/speech/SpeechS...
Source/modules/speech/SpeechSynthesis.cpp:61: void
SpeechSynthesis::voicesDidChange()
When can this function be called?  Why don't we need SpeechSynthesis to be an
ActiveDOMObject so that it keeps its JS wrapper alive?

https://codereview.chromium.org/27477002/diff/1/Source/modules/speech/SpeechS...
File Source/modules/speech/SpeechSynthesis.h (right):

https://codereview.chromium.org/27477002/diff/1/Source/modules/speech/SpeechS...
Source/modules/speech/SpeechSynthesis.h:67: using
RefCounted<SpeechSynthesis>::deref;
There's a macro that does this work now, I believe.

https://codereview.chromium.org/27477002/diff/1/Source/modules/speech/SpeechS...
Source/modules/speech/SpeechSynthesis.h:72: SpeechSynthesis(ExecutionContext*);
Please mark one-argument constructors explicit.

[dmazzoni, 2013-10-16 22:09:06]

https://codereview.chromium.org/27477002/diff/1/Source/modules/speech/SpeechS...
File Source/modules/speech/SpeechSynthesis.cpp (right):

https://codereview.chromium.org/27477002/diff/1/Source/modules/speech/SpeechS...
Source/modules/speech/SpeechSynthesis.cpp:61: void
SpeechSynthesis::voicesDidChange()
On 2013/10/16 19:25:06, abarth wrote:
> When can this function be called?  Why don't we need SpeechSynthesis to be an
> ActiveDOMObject so that it keeps its JS wrapper alive?

Good question.

This function is only called from the platform, in response to an IPC from the
browser, on the main event loop. So I don't think we have to worry about it
being in a weird state.

SpeechSynthesis is owned by the DOMWindow, and it doesn't have any state that
needs to be preserved if the window goes away. The global speech queue state is
kept by Chromium in the browser process; it keeps everything running smoothly
even when a window disappears.

https://codereview.chromium.org/27477002/diff/1/Source/modules/speech/SpeechS...
File Source/modules/speech/SpeechSynthesis.h (right):

https://codereview.chromium.org/27477002/diff/1/Source/modules/speech/SpeechS...
Source/modules/speech/SpeechSynthesis.h:67: using
RefCounted<SpeechSynthesis>::deref;
On 2013/10/16 19:25:06, abarth wrote:
> There's a macro that does this work now, I believe.

I can't seem to find it from a code search and a search of the blink-reviews
list - any idea what I'm looking for?

https://codereview.chromium.org/27477002/diff/1/Source/modules/speech/SpeechS...
Source/modules/speech/SpeechSynthesis.h:72: SpeechSynthesis(ExecutionContext*);
On 2013/10/16 19:25:06, abarth wrote:
> Please mark one-argument constructors explicit.

Done.

[abarth-chromium, 2013-10-17 02:00:53]

LGTM w/ the nits below addressed

https://codereview.chromium.org/27477002/diff/1/Source/modules/speech/SpeechS...
File Source/modules/speech/SpeechSynthesis.cpp (right):

https://codereview.chromium.org/27477002/diff/1/Source/modules/speech/SpeechS...
Source/modules/speech/SpeechSynthesis.cpp:61: void
SpeechSynthesis::voicesDidChange()
On 2013/10/16 22:09:06, Dominic Mazzoni wrote:
> On 2013/10/16 19:25:06, abarth wrote:
> > When can this function be called?  Why don't we need SpeechSynthesis to be
an
> > ActiveDOMObject so that it keeps its JS wrapper alive?
> 
> Good question.
> 
> This function is only called from the platform, in response to an IPC from the
> browser, on the main event loop. So I don't think we have to worry about it
> being in a weird state.
> 
> SpeechSynthesis is owned by the DOMWindow, and it doesn't have any state that
> needs to be preserved if the window goes away. The global speech queue state
is
> kept by Chromium in the browser process; it keeps everything running smoothly
> even when a window disappears.

I should write a design doc that explains how all this works in more detail. 
What you've written is almost correct, but there is one tiny detail: we need to
make sure not to dispatch any events after the DOMWindow detaches from the
Frame.  The easiest way to handle that is to check whether
executionContext()->activeDOMObjectsAreStopped() before dispatching the event.

https://codereview.chromium.org/27477002/diff/1/Source/modules/speech/SpeechS...
File Source/modules/speech/SpeechSynthesis.h (right):

https://codereview.chromium.org/27477002/diff/1/Source/modules/speech/SpeechS...
Source/modules/speech/SpeechSynthesis.h:67: using
RefCounted<SpeechSynthesis>::deref;
On 2013/10/16 22:09:06, Dominic Mazzoni wrote:
> On 2013/10/16 19:25:06, abarth wrote:
> > There's a macro that does this work now, I believe.
> 
> I can't seem to find it from a code search and a search of the blink-reviews
> list - any idea what I'm looking for?

http://src.chromium.org/viewvc/blink?view=revision&revision=159796

https://codereview.chromium.org/27477002/diff/1/Source/modules/speech/SpeechS...
File Source/modules/speech/SpeechSynthesis.idl (right):

https://codereview.chromium.org/27477002/diff/1/Source/modules/speech/SpeechS...
Source/modules/speech/SpeechSynthesis.idl:28:
ConstructorCallWith=ExecutionContext,
Given that there's NoInterfaceObject and this object is held off the DOMWindow,
you don't need this attribute.  There is no constructor call!

[dmazzoni, 2013-10-17 15:28:58]

Feedback addressed, but please confirm that the change I made to
SpeechSynthesis::fireEvent is correct too, thanks.

https://codereview.chromium.org/27477002/diff/1/Source/modules/speech/SpeechS...
File Source/modules/speech/SpeechSynthesis.cpp (right):

https://codereview.chromium.org/27477002/diff/1/Source/modules/speech/SpeechS...
Source/modules/speech/SpeechSynthesis.cpp:61: void
SpeechSynthesis::voicesDidChange()
On 2013/10/17 02:00:53, abarth wrote:
> On 2013/10/16 22:09:06, Dominic Mazzoni wrote:
> > On 2013/10/16 19:25:06, abarth wrote:
> > > When can this function be called?  Why don't we need SpeechSynthesis to be
> an
> > > ActiveDOMObject so that it keeps its JS wrapper alive?
> > 
> > Good question.
> > 
> > This function is only called from the platform, in response to an IPC from
the
> > browser, on the main event loop. So I don't think we have to worry about it
> > being in a weird state.
> > 
> > SpeechSynthesis is owned by the DOMWindow, and it doesn't have any state
that
> > needs to be preserved if the window goes away. The global speech queue state
> is
> > kept by Chromium in the browser process; it keeps everything running
smoothly
> > even when a window disappears.
> 
> I should write a design doc that explains how all this works in more detail. 
> What you've written is almost correct, but there is one tiny detail: we need
to
> make sure not to dispatch any events after the DOMWindow detaches from the
> Frame.  The easiest way to handle that is to check whether
> executionContext()->activeDOMObjectsAreStopped() before dispatching the event.

Got it, that makes sense.

I made the same change to SpeechSynthesis::fireEvent, does that look right?

https://codereview.chromium.org/27477002/diff/1/Source/modules/speech/SpeechS...
File Source/modules/speech/SpeechSynthesis.h (right):

https://codereview.chromium.org/27477002/diff/1/Source/modules/speech/SpeechS...
Source/modules/speech/SpeechSynthesis.h:67: using
RefCounted<SpeechSynthesis>::deref;
On 2013/10/17 02:00:53, abarth wrote:
> On 2013/10/16 22:09:06, Dominic Mazzoni wrote:
> > On 2013/10/16 19:25:06, abarth wrote:
> > > There's a macro that does this work now, I believe.
> > 
> > I can't seem to find it from a code search and a search of the blink-reviews
> > list - any idea what I'm looking for?
> 
> http://src.chromium.org/viewvc/blink?view=revision&revision=159796

Thanks. No wonder I missed it, it hadn't even rolled yet.

https://codereview.chromium.org/27477002/diff/1/Source/modules/speech/SpeechS...
File Source/modules/speech/SpeechSynthesis.idl (right):

https://codereview.chromium.org/27477002/diff/1/Source/modules/speech/SpeechS...
Source/modules/speech/SpeechSynthesis.idl:28:
ConstructorCallWith=ExecutionContext,
On 2013/10/17 02:00:53, abarth wrote:
> Given that there's NoInterfaceObject and this object is held off the
DOMWindow,
> you don't need this attribute.  There is no constructor call!

Oh duh - I should have realized this since the constructor is private. Thanks.

[abarth-chromium, 2013-10-17 19:43:48]

lgtm

LGTM

Thanks!

[commit-bot: I haz the power, 2013-10-17 19:54:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/27477002/12001

[commit-bot: I haz the power, 2013-10-17 22:52:17]

Change committed as 159914