Bug 106847 – Stub out WebSpeech synthesis

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

The first part will be to stub out the API and put it behind the right feature flag.

[chris fleizach]

Created attachment 182923 [details]
initial stubbing out of JS objects and code

[chris fleizach]

Spec is here for reference
https://dvcs.w3.org/hg/speech-api/raw-file/9a0075d25326/speechapi.html#tts-section

[Adam Barth]

@Chris: Do you have someone in mind to review this patch?  If not, I can take a look tomorrow.

[chris fleizach]

(In reply to comment #3)
> @Chris: Do you have someone in mind to review this patch?  If not, I can take a look tomorrow.

Thanks that would be great.

[Build Bot]

Comment on attachment 182923 [details]
initial stubbing out of JS objects and code

Attachment 182923 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15903306

New failing tests:
fast/js/integer-division-neg2tothe32-by-neg1.html

[Zan Dobersek]

(In reply to comment #5)
> (From update of attachment 182923 [details])
> Attachment 182923 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/15903306
> 
> New failing tests:
> fast/js/integer-division-neg2tothe32-by-neg1.html

Not relevant, that's a current failure of different origin.

[Adam Barth]

Comment on attachment 182923 [details]
initial stubbing out of JS objects and code

View in context: https://bugs.webkit.org/attachment.cgi?id=182923&action=review

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:26207
>  				FD537353137B651800008DCE /* ZeroPole.h in Headers */,
> +				AA2A5ACE16A485FD00975A25 /* SpeechSynthesisVoice.h in Headers */,

Would you be willing to sort the xcodeproj file when you land this patch?  We try to keep the file sorted so that folks have fewer merge conflicts.

> Source/WebCore/Configurations/FeatureDefines.xcconfig:157
> +ENABLE_WEB_SPEECH_SYNTHESIS = ;

nit: I probably would have just called this ENABLE_SPEECH_SYNTHESIS (the "web" doesn't really mean much in this context).

> Source/WebCore/Modules/speech/DOMWindowSpeechSynthesis.idl:29
> +] interface DOMWindowSpeechSynthesis {

It's interesting that you've created a new interface for synthesis.  I would have expected you to add this code to DOMWindowSpeech.idl, but I agree that this approach is cleaner.

> Source/WebCore/Modules/speech/DOMWindowSpeechSynthesis.idl:33
> +    readonly attribute SpeechSynthesis speechSynthesis;
> +    attribute SpeechSynthesisEventConstructor SpeechSynthesisEvent;
> +    attribute SpeechSynthesisUtteranceConstructor SpeechSynthesisUtterance;

Here you've chosen to use unprefixed names.  That's different from what we did with ENABLE(SCRIPTED_SPEECH), but it's also a fine approach.

> Source/WebCore/Modules/speech/SpeechSynthesisUtterance.h:39
> +    static PassRefPtr<SpeechSynthesisUtterance> create(String text);

String -> const String&

There are many occurrences of this issue in this patch.  Generally speaking, when passing a String as a parameter to a function, you almost always want to use const String& to avoid ref churn.

> Source/WebCore/Modules/speech/SpeechSynthesisUtterance.h:60
> +    EventListener* onstart() { return getAttributeEventListener(eventNames().startEvent); }
> +    void setOnstart(PassRefPtr<EventListener> listener) { setAttributeEventListener(eventNames().startEvent, listener); }

I believe we have a macro that does this work for you.  See, for example, http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Document.h#L264

DEFINE_ATTRIBUTE_EVENT_LISTENER(abort);

> Source/WebCore/Modules/speech/SpeechSynthesisUtterance.h:86
> +    SpeechSynthesisUtterance(String text);

Please mark one-argument constructors "explicit".

> Source/WebCore/Modules/speech/SpeechSynthesisUtterance.h:97
> +    ScriptExecutionContext* m_scriptExecutionContext;

This is very unlikely to the the right thing to do.  Instead, you probably want to inherit from ContextDestructionObserver so that your ScriptExecutionContext pointer won't be left dangling when the ScriptExecutionContext is destroyed.

> Source/WebCore/Modules/speech/SpeechSynthesisVoice.h:43
> +    String voiceURI() const { return m_voiceURI; }
> +    String name() const { return m_name; }
> +    String lang() const { return m_lang; }

These should all return const String& because this object hold the underlying String object (again to avoid ref churn)

> Source/WebCore/Modules/speech/DOMWindowSpeechSynthesis.h:51
> +    DOMWindow* m_window;

It's unlikely that you should hold onto a DOMWindow*.  Again, there's a risk that the DOMWindow pointer will be left dangling.  Given that you're inheriting from DOMWindowProperty, you already have an m_associatedDOMWindow member variable whose lifetime is managed automatically.

> Source/WebCore/Modules/speech/SpeechSynthesisVoiceList.idl:28
> +] interface SpeechSynthesisVoiceList {

Rather than creating a fake JavaScript array, we should probably change the spec to use sequence<SpeechSynthesisVoice>.  The platform as a whole is moving away from these fake arrays because don't work correctly from the perspective of a JavaScript developer.

[chris fleizach]

Thanks for the review. Just a few comments inline and I'll address the rest of the issues with the next patch

(In reply to comment #7)
> (From update of attachment 182923 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182923&action=review
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:26207
> >  				FD537353137B651800008DCE /* ZeroPole.h in Headers */,
> > +				AA2A5ACE16A485FD00975A25 /* SpeechSynthesisVoice.h in Headers */,
> 
> Would you be willing to sort the xcodeproj file when you land this patch?  We try to keep the file sorted so that folks have fewer merge conflicts.

Yea, i'll fix this in the next upload

> 
> > Source/WebCore/Configurations/FeatureDefines.xcconfig:157
> > +ENABLE_WEB_SPEECH_SYNTHESIS = ;
> 
> nit: I probably would have just called this ENABLE_SPEECH_SYNTHESIS (the "web" doesn't really mean much in this context).

The spec is called "WebSpeech", but I can rename to just SPEECH_SYN

> 
> > Source/WebCore/Modules/speech/DOMWindowSpeechSynthesis.idl:29
> > +] interface DOMWindowSpeechSynthesis {
> 
> It's interesting that you've created a new interface for synthesis.  I would have expected you to add this code to DOMWindowSpeech.idl, but I agree that this approach is cleaner.
> 
> > Source/WebCore/Modules/speech/DOMWindowSpeechSynthesis.idl:33
> > +    readonly attribute SpeechSynthesis speechSynthesis;
> > +    attribute SpeechSynthesisEventConstructor SpeechSynthesisEvent;
> > +    attribute SpeechSynthesisUtteranceConstructor SpeechSynthesisUtterance;
> 
> Here you've chosen to use unprefixed names.  That's different from what we did with ENABLE(SCRIPTED_SPEECH), but it's also a fine approach.
> 
> > Source/WebCore/Modules/speech/SpeechSynthesisUtterance.h:39
> > +    static PassRefPtr<SpeechSynthesisUtterance> create(String text);
> 
> String -> const String&
> 
> There are many occurrences of this issue in this patch.  Generally speaking, when passing a String as a parameter to a function, you almost always want to use const String& to avoid ref churn.
> 
> > Source/WebCore/Modules/speech/SpeechSynthesisUtterance.h:60
> > +    EventListener* onstart() { return getAttributeEventListener(eventNames().startEvent); }
> > +    void setOnstart(PassRefPtr<EventListener> listener) { setAttributeEventListener(eventNames().startEvent, listener); }
> 
> I believe we have a macro that does this work for you.  See, for example, http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Document.h#L264
> 
> DEFINE_ATTRIBUTE_EVENT_LISTENER(abort);
> 
> > Source/WebCore/Modules/speech/SpeechSynthesisUtterance.h:86
> > +    SpeechSynthesisUtterance(String text);
> 
> Please mark one-argument constructors "explicit".
> 
> > Source/WebCore/Modules/speech/SpeechSynthesisUtterance.h:97
> > +    ScriptExecutionContext* m_scriptExecutionContext;
> 
> This is very unlikely to the the right thing to do.  Instead, you probably want to inherit from ContextDestructionObserver so that your ScriptExecutionContext pointer won't be left dangling when the ScriptExecutionContext is destroyed.
> 
> > Source/WebCore/Modules/speech/SpeechSynthesisVoice.h:43
> > +    String voiceURI() const { return m_voiceURI; }
> > +    String name() const { return m_name; }
> > +    String lang() const { return m_lang; }
> 
> These should all return const String& because this object hold the underlying String object (again to avoid ref churn)
> 
> > Source/WebCore/Modules/speech/DOMWindowSpeechSynthesis.h:51
> > +    DOMWindow* m_window;
> 
> It's unlikely that you should hold onto a DOMWindow*.  Again, there's a risk that the DOMWindow pointer will be left dangling.  Given that you're inheriting from DOMWindowProperty, you already have an m_associatedDOMWindow member variable whose lifetime is managed automatically.
> 
> > Source/WebCore/Modules/speech/SpeechSynthesisVoiceList.idl:28
> > +] interface SpeechSynthesisVoiceList {
> 
> Rather than creating a fake JavaScript array, we should probably change the spec to use sequence<SpeechSynthesisVoice>.  The platform as a whole is moving away from these fake arrays because don't work correctly from the perspective of a JavaScript developer.

So do you recommend going with sequence<Speec..> now and asking spec developers to change?

[Adam Barth]

> So do you recommend going with sequence<Speec..> now and asking spec developers to change?

Yeah, that's probably fine.  We should just make sure we end up converging with the spec before enabling the feature.

[chris fleizach]

Created attachment 183020 [details]
updates based on review

[WebKit Review Bot]

Attachment 183020 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/Modules/speech/SpeechSynthesisUtterance.h:72:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/Modules/speech/SpeechSynthesis.cpp:41:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 2 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Created attachment 183022 [details]
patch fixing style warnings

[Adam Barth]

Comment on attachment 183022 [details]
patch fixing style warnings

View in context: https://bugs.webkit.org/attachment.cgi?id=183022&action=review

Thanks for updating the patch.  This looks much better!

> Source/WebCore/Modules/speech/SpeechSynthesis.h:55
> +    Vector<RefPtr<SpeechSynthesisVoice> > getVoices() { return m_voiceList; };

Here you probably want an const Vector<RefPtr<SpeechSynthesisVoice> >& to avoid making an extra copy of the vector.

[chris fleizach]

https://trac.webkit.org/changeset/139918

[chris fleizach]

Thanks for the review. 

I'll do the rest of the parts in much smaller chunks in bugs off the main bug
https://bugs.webkit.org/show_bug.cgi?id=106742

[Joseph Pecoraro]

Comment on attachment 183022 [details]
patch fixing style warnings

View in context: https://bugs.webkit.org/attachment.cgi?id=183022&action=review

> Source/WebCore/Configurations/FeatureDefines.xcconfig:157
> +ENABLE_SPEECH_SYNTHESIS = ;

Just a heads up for the future, please keep all the FeatureDefines.xcconfig files in sync across all the OS X projects (JavaScriptCore, WebCore, WebKit, and WebKit2). The files are meant to stay identical. I'll drive by fix this with my FeatureDefines changes.