Summary: | WebSpeech: implement voices list | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | chris fleizach <cfleizach> | ||||
Component: | Accessibility | Assignee: | 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
chris fleizach
2013-01-16 08:14:34 PST
Created attachment 183044 [details]
patch
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.) (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? > 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.
|