Support multiple voices for all available modules (text-to-speech engines) on Linux.

Support multiple voices for all available modules (text-to-speech engines) on Linux.

This adds appropriate calls to retrieve voices from SpeechDispatcher and make it available through Chrome.

We combine all voices across all available modules into one list in order to conform to the way Chrome exposes voices.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202665

[dmazzoni, 2013-05-28 07:30:02]

https://codereview.chromium.org/15846006/diff/5001/chrome/browser/speech/tts_...
File chrome/browser/speech/tts_linux.cc (right):

https://codereview.chromium.org/15846006/diff/5001/chrome/browser/speech/tts_...
chrome/browser/speech/tts_linux.cc:25: std::string name;
nit: indent struct members

https://codereview.chromium.org/15846006/diff/5001/chrome/browser/speech/tts_...
chrome/browser/speech/tts_linux.cc:169: all_native_voices_->find(voice.name);
nit: indent just 4

https://codereview.chromium.org/15846006/diff/5001/chrome/browser/speech/tts_...
chrome/browser/speech/tts_linux.cc:209: char** modules =
libspeechd_loader_.spd_list_modules(conn_);
Technically I think this array and each string within it needs to be freed with
free.
The libspeechd docs are really unclear, but I looked at the source
(see spd_execute_command_with_list_reply in particular) and these are
all malloced and returned.

https://codereview.chromium.org/15846006/diff/5001/chrome/browser/speech/tts_...
chrome/browser/speech/tts_linux.cc:210: if (!modules) {
nit: normally blocks with a single-line body don't need braces in chromium C++
code

https://codereview.chromium.org/15846006/diff/5001/chrome/browser/speech/tts_...
chrome/browser/speech/tts_linux.cc:215:
libspeechd_loader_.spd_list_synthesis_voices(conn_);
This doesn't seem to depend on the for loop index - does it belong outside the
loop?

[David Tseng, 2013-05-28 17:13:45]

On Tue, May 28, 2013 at 12:30 AM, <dmazzoni@chromium.org> wrote:

>
> https://codereview.chromium.**org/15846006/diff/5001/chrome/**
>
browser/speech/tts_linux.cc<https://codereview.chromium.org/15846006/diff/5001/chrome/browser/speech/tts_linux.cc>
> File chrome/browser/speech/tts_**linux.cc (right):
>
> https://codereview.chromium.**org/15846006/diff/5001/chrome/**
>
browser/speech/tts_linux.cc#**newcode25<https://codereview.chromium.org/15846006/diff/5001/chrome/browser/speech/tts_linux.cc#newcode25>
> chrome/browser/speech/tts_**linux.cc:25: std::string name;
> nit: indent struct members
> Done.
>


> https://codereview.chromium.**org/15846006/diff/5001/chrome/**
>
browser/speech/tts_linux.cc#**newcode169<https://codereview.chromium.org/15846006/diff/5001/chrome/browser/speech/tts_linux.cc#newcode169>
> chrome/browser/speech/tts_**linux.cc:169:
> all_native_voices_->find(voice**.name <http://voice.name>);
> nit: indent just 4
> Done.
>


> https://codereview.chromium.**org/15846006/diff/5001/chrome/**
>
browser/speech/tts_linux.cc#**newcode209<https://codereview.chromium.org/15846006/diff/5001/chrome/browser/speech/tts_linux.cc#newcode209>
> chrome/browser/speech/tts_**linux.cc:209: char** modules =
> libspeechd_loader_.spd_list_**modules(conn_);
> Technically I think this array and each string within it needs to be
> freed with free.
> The libspeechd docs are really unclear, but I looked at the source
> (see spd_execute_command_with_list_**reply in particular) and these are
> all malloced and returned.
> Is that within the Chrome process? Not very familiar with how dbus
> messaging worksand how arguments get marshalled.


Done.



> https://codereview.chromium.**org/15846006/diff/5001/chrome/**
>
browser/speech/tts_linux.cc#**newcode210<https://codereview.chromium.org/15846006/diff/5001/chrome/browser/speech/tts_linux.cc#newcode210>
> chrome/browser/speech/tts_**linux.cc:210: if (!modules) {
> nit: normally blocks with a single-line body don't need braces in
> chromium C++ code
>
> I knew that ;); too much js made this a habit.

> https://codereview.chromium.**org/15846006/diff/5001/chrome/**
>
browser/speech/tts_linux.cc#**newcode215<https://codereview.chromium.org/15846006/diff/5001/chrome/browser/speech/tts_linux.cc#newcode215>
> chrome/browser/speech/tts_**linux.cc:215:
> libspeechd_loader_.spd_list_**synthesis_voices(conn_);
> This doesn't seem to depend on the for loop index - does it belong
> outside the loop?
>
> Good catch; I actually need to switch to each module before being able to
enumerate the voices for the selected module.
Done.

>
https://codereview.chromium.**org/15846006/<https://codereview.chromium.org/1...
>

[dmazzoni, 2013-05-28 18:03:57]

lgtm

On 2013/05/28 17:13:45, David Tseng wrote:
> > https://codereview.chromium.**org/15846006/diff/5001/chrome/**
> >
>
browser/speech/tts_linux.cc#**newcode209<https://codereview.chromium.org/15846006/diff/5001/chrome/browser/speech/tts_linux.cc#newcode209>
> > chrome/browser/speech/tts_**linux.cc:209: char** modules =
> > libspeechd_loader_.spd_list_**modules(conn_);
> > Technically I think this array and each string within it needs to be
> > freed with free.
> > The libspeechd docs are really unclear, but I looked at the source
> > (see spd_execute_command_with_list_**reply in particular) and these are
> > all malloced and returned.
> > Is that within the Chrome process? Not very familiar with how dbus
> > messaging worksand how arguments get marshalled.

Yes, this is in the client library that makes the dbus calls.

https://codereview.chromium.org/15846006/diff/9001/chrome/browser/speech/tts_...
File chrome/browser/speech/tts_linux.cc (right):

https://codereview.chromium.org/15846006/diff/9001/chrome/browser/speech/tts_...
chrome/browser/speech/tts_linux.cc:218: continue;
Nit: should free modules[i] even if you continue here.

[David Tseng, 2013-05-28 19:08:47]

On Tue, May 28, 2013 at 11:03 AM, <dmazzoni@chromium.org> wrote:

> lgtm
>
> On 2013/05/28 17:13:45, David Tseng wrote:
>
>> > https://codereview.chromium.****org/15846006/diff/5001/chrome/****
>> >
>>
>
> browser/speech/tts_linux.cc#****newcode209<https://codereview.**
> chromium.org/15846006/diff/**5001/chrome/browser/speech/**
>
tts_linux.cc#newcode209<https://codereview.chromium.org/15846006/diff/5001/chrome/browser/speech/tts_linux.cc#newcode209>
> >
>
>> > chrome/browser/speech/tts_****linux.cc:209: char** modules =
>> > libspeechd_loader_.spd_list_****modules(conn_);
>>
>> > Technically I think this array and each string within it needs to be
>> > freed with free.
>> > The libspeechd docs are really unclear, but I looked at the source
>> > (see spd_execute_command_with_list_****reply in particular) and these
>> are
>>
>> > all malloced and returned.
>> > Is that within the Chrome process? Not very familiar with how dbus
>> > messaging worksand how arguments get marshalled.
>>
>
> Yes, this is in the client library that makes the dbus calls.
>
>
>
> https://codereview.chromium.**org/15846006/diff/9001/chrome/**
>
browser/speech/tts_linux.cc<https://codereview.chromium.org/15846006/diff/9001/chrome/browser/speech/tts_linux.cc>
> File chrome/browser/speech/tts_**linux.cc (right):
>
> https://codereview.chromium.**org/15846006/diff/9001/chrome/**
>
browser/speech/tts_linux.cc#**newcode218<https://codereview.chromium.org/15846006/diff/9001/chrome/browser/speech/tts_linux.cc#newcode218>
> chrome/browser/speech/tts_**linux.cc:218: continue;
> Nit: should free modules[i] even if you continue here.
> Done.
>


>
https://codereview.chromium.**org/15846006/<https://codereview.chromium.org/1...
>

[commit-bot: I haz the power, 2013-05-28 19:10:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtseng@chromium.org/15846006/14001

[commit-bot: I haz the power, 2013-05-28 22:08:45]

Change committed as 202665