Bug 107014

Summary: WebSpeech: implement voices list
Product: WebKit Reporter: chris fleizach <cfleizach>
Component: AccessibilityAssignee: chris fleizach <cfleizach>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dmazzoni, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 106742    
Attachments:
Description Flags
patch abarth: review+, abarth: commit-queue-

Description chris fleizach 2013-01-16 08:14:34 PST
Implement the voices list to return the right data.
Comment 1 chris fleizach 2013-01-16 15:33:01 PST
Created attachment 183044 [details]
patch
Comment 2 Adam Barth 2013-01-16 23:09:35 PST
Comment on attachment 183044 [details]
patch

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

I don't read objective-c that well, but the objective-c code looks plausible.  If you feel you need an objective-c expert, we might want to ask someone to double-check that part of the patch.

> Source/WebCore/Modules/speech/mac/SpeechSynthesisMac.mm:56
> +        RefPtr<SpeechSynthesisVoice> voice = SpeechSynthesisVoice::create(voiceURI, name, language, true, isDefault);
> +        m_voiceList.append(voice);

I would combine these lines both for readability and to save a ref churn.  You can also save the ref churn by calling voice.release() on the last line.

> LayoutTests/platform/mac/TestExpectations:192
> +# Speech synthesis is not yet enabled.
> +platform/mac/fast/speechsynthesis

Should these tests be in platform/mac, or should they be in fast/speechsynthesis outside the "mac" directory?  The test you wrote doesn't seem particularly mac-specific.  (I realize this is going to make you edit a bunch of other TestExpectations files, but it's probably the right thing to do.)
Comment 3 chris fleizach 2013-01-16 23:23:46 PST
(In reply to comment #2)
> (From update of attachment 183044 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183044&action=review
> 
> I don't read objective-c that well, but the objective-c code looks plausible.  If you feel you need an objective-c expert, we might want to ask someone to double-check that part of the patch.
> 
> > Source/WebCore/Modules/speech/mac/SpeechSynthesisMac.mm:56
> > +        RefPtr<SpeechSynthesisVoice> voice = SpeechSynthesisVoice::create(voiceURI, name, language, true, isDefault);
> > +        m_voiceList.append(voice);
> 
> I would combine these lines both for readability and to save a ref churn.  You can also save the ref churn by calling voice.release() on the last line.
> 
> > LayoutTests/platform/mac/TestExpectations:192
> > +# Speech synthesis is not yet enabled.
> > +platform/mac/fast/speechsynthesis
> 
> Should these tests be in platform/mac, or should they be in fast/speechsynthesis outside the "mac" directory?  The test you wrote doesn't seem particularly mac-specific.  (I realize this is going to make you edit a bunch of other TestExpectations files, but it's probably the right thing to do.)

I thought about that, but given that the data for the voices is going to be platform specific I wasn't sure that i could make a test that would work on all platforms. i figured when the next platform started to implement this we could move the tests that make sense. what do you think?
Comment 4 Adam Barth 2013-01-16 23:50:00 PST
> I thought about that, but given that the data for the voices is going to be platform specific I wasn't sure that i could make a test that would work on all platforms. i figured when the next platform started to implement this we could move the tests that make sense. what do you think?

It's hard for me to know without being able to see the tests that we haven't written yet.  We can certainly start out this way if you're open to moving the tests in a future patch if they seem useful cross-port.
Comment 5 chris fleizach 2013-01-17 00:16:08 PST
https://trac.webkit.org/changeset/139970