Bug 107014 – WebSpeech: implement voices list

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

Implement the voices list to return the right data.

[chris fleizach]

Created attachment 183044 [details]
patch

[Adam Barth]

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.)

[chris fleizach]

(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?

[Adam Barth]

> 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.

[chris fleizach]

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