Bug 106847 - Stub out WebSpeech synthesis
Summary: Stub out WebSpeech synthesis
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: chris fleizach
URL:
Keywords:
Depends on:
Blocks: 106742
  Show dependency treegraph
 
Reported: 2013-01-14 17:42 PST by chris fleizach
Modified: 2013-01-22 13:25 PST (History)
5 users (show)

See Also:


Attachments
initial stubbing out of JS objects and code (77.69 KB, patch)
2013-01-15 23:46 PST, chris fleizach
abarth: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
updates based on review (76.00 KB, patch)
2013-01-16 12:45 PST, chris fleizach
no flags Details | Formatted Diff | Diff
patch fixing style warnings (75.96 KB, patch)
2013-01-16 12:50 PST, chris fleizach
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chris fleizach 2013-01-14 17:42:24 PST
The first part will be to stub out the API and put it behind the right feature flag.
Comment 1 chris fleizach 2013-01-15 23:46:17 PST
Created attachment 182923 [details]
initial stubbing out of JS objects and code
Comment 2 chris fleizach 2013-01-15 23:56:56 PST
Spec is here for reference
https://dvcs.w3.org/hg/speech-api/raw-file/9a0075d25326/speechapi.html#tts-section
Comment 3 Adam Barth 2013-01-16 00:09:05 PST
@Chris: Do you have someone in mind to review this patch?  If not, I can take a look tomorrow.
Comment 4 chris fleizach 2013-01-16 00:11:45 PST
(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.
Comment 5 Build Bot 2013-01-16 01:39:34 PST
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
Comment 6 Zan Dobersek 2013-01-16 01:41:11 PST
(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.
Comment 7 Adam Barth 2013-01-16 10:42:40 PST
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.
Comment 8 chris fleizach 2013-01-16 10:47:55 PST
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?
Comment 9 Adam Barth 2013-01-16 11:04:17 PST
> 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.
Comment 10 chris fleizach 2013-01-16 12:45:21 PST
Created attachment 183020 [details]
updates based on review
Comment 11 WebKit Review Bot 2013-01-16 12:48:21 PST
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.
Comment 12 chris fleizach 2013-01-16 12:50:36 PST
Created attachment 183022 [details]
patch fixing style warnings
Comment 13 Adam Barth 2013-01-16 13:39:27 PST
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.
Comment 14 chris fleizach 2013-01-16 13:47:14 PST
https://trac.webkit.org/changeset/139918
Comment 15 chris fleizach 2013-01-16 13:48:12 PST
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
Comment 16 Joseph Pecoraro 2013-01-22 13:25:42 PST
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.