Move the code in libcros' chromeos_speech_synthesis.cc into Chrome

Move the code in libcros' chromeos_speech_synthesis.cc into Chrome

Add SpeechSynthesizerClient which consists the code copied from chromeos_speech_synthesis.cc
Remove the test ExtensionApiTest.TtsChromeOs from extension_tts_apitest.cc

BUG=chromium-os:16560
TEST=Enable accessibility feature by pressing Ctrl+Alt+Z on the login screen, ensure that it is speaking as before.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=106246

[hashimoto, 2011-10-18 12:05:23]

SpeechSynthesizerClient is based on the code in libcros'
chromeos_speech_synthesis.cc
(http://git.chromium.org/gitweb/?p=chromiumos/platform/cros.git;a=blob;f=chrom...)

ExtensionApiTest.TtsChromeOs is removed because:
 1. There are already a sufficient number of tests in extension_tts_apitest.cc
to test Extension TTS API itself.
 2. What ExtensionApiTest.TtsChromeOs is doing is only testing
ExtensionTtsPlatformImplChromeOs without actual dbus interactions.
 3. Since SpeechSynthesisLibrary in Cros becomes SpeechSynthesizerClient outside
Cros, it seems costing too much to implement its mock for the one single test.

[satorux1, 2011-10-18 20:49:25]

LGTM. This is awesome! I have minor comments below, but it's up to you.

http://codereview.chromium.org/8334018/diff/1035/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/speech_synthesizer_client.cc (right):

http://codereview.chromium.org/8334018/diff/1035/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/speech_synthesizer_client.cc:41: "Speak");
Can you define a constant for "Speak" in service_constants.h and use it instead?
You can do it in a separate patch. If so, please add TODO a comment here.

http://codereview.chromium.org/8334018/diff/1035/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/speech_synthesizer_client.cc:51: "SetProperties");
ditto.

http://codereview.chromium.org/8334018/diff/1035/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/speech_synthesizer_client.h (right):

http://codereview.chromium.org/8334018/diff/1035/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/speech_synthesizer_client.h:24: typedef
base::Callback<void(bool)> IsSpeakingCallback;
Would be nice to document about the boolean parameter

http://codereview.chromium.org/8334018/diff/1035/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_tts_api_chromeos.cc (right):

http://codereview.chromium.org/8334018/diff/1035/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tts_api_chromeos.cc:115:
speech_synthesizer_client->Speak(utterance);
I was a bit worried about the change here as Speak() no longer returns
true/false. However, looking at the code,  ChromeOSSpeak() always returns true,
so this doesn't change the semantics here. :)

http://codereview.chromium.org/8334018/diff/1035/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tts_api_chromeos.cc:145:
weak_ptr_factory_.GetWeakPtr(), utterance_id));
Nice! It's now asynchronous thus not blocking UI. :)

[hashimoto, 2011-10-19 00:08:47]

New patch set, fixed some comments.

http://codereview.chromium.org/8334018/diff/1035/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/speech_synthesizer_client.cc (right):

http://codereview.chromium.org/8334018/diff/1035/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/speech_synthesizer_client.cc:41: "Speak");
On 2011/10/18 20:49:25, satorux1 wrote:
> Can you define a constant for "Speak" in service_constants.h and use it
instead?
> You can do it in a separate patch. If so, please add TODO a comment here.

Done.

http://codereview.chromium.org/8334018/diff/1035/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/speech_synthesizer_client.h (right):

http://codereview.chromium.org/8334018/diff/1035/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/speech_synthesizer_client.h:24: typedef
base::Callback<void(bool)> IsSpeakingCallback;
On 2011/10/18 20:49:25, satorux1 wrote:
> Would be nice to document about the boolean parameter

Done.

http://codereview.chromium.org/8334018/diff/1035/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_tts_api_chromeos.cc (right):

http://codereview.chromium.org/8334018/diff/1035/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tts_api_chromeos.cc:115:
speech_synthesizer_client->Speak(utterance);
On 2011/10/18 20:49:25, satorux1 wrote:
> I was a bit worried about the change here as Speak() no longer returns
> true/false. However, looking at the code,  ChromeOSSpeak() always returns
true,
> so this doesn't change the semantics here. :)

Yes, this change keeps the semantics, nothing is changed.
If we are going to care about the result, we can add a callback parameter for
Speak later.

http://codereview.chromium.org/8334018/diff/1035/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tts_api_chromeos.cc:145:
weak_ptr_factory_.GetWeakPtr(), utterance_id));
On 2011/10/18 20:49:25, satorux1 wrote:
> Nice! It's now asynchronous thus not blocking UI. :)

I tried to make IsSpeaking synchronous, but gave up because
dbus::ObjectProxy::CallMethodAndBlock must be called from DBus thread, not from
UI thread.
By changing it to asynchronous, however, it becomes not blocking UI thread as
you say.

[satorux1, 2011-10-19 00:23:00]

LGTM++

[satorux1, 2011-10-19 00:29:17]

BTW, once it's checked in, please deprecate functions in
chromeos_speech_synthesis.cc in libcros, and then remove them after a week or
so.

FWIW, I'm doing it for chromeos_loginc.cc and chromeos_brightness.cc:

http://gerrit.chromium.org/gerrit/#change,10269

[commit-bot: I haz the power, 2011-10-19 03:10:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/8334018/11002

[hashimoto, 2011-10-19 03:11:13]

On 2011/10/19 00:29:17, satorux1 wrote:
> BTW, once it's checked in, please deprecate functions in
> chromeos_speech_synthesis.cc in libcros, and then remove them after a week or
> so.
> 
> FWIW, I'm doing it for chromeos_loginc.cc and chromeos_brightness.cc:
> 
> http://gerrit.chromium.org/gerrit/#change%2C10269

OK, I will.  Thank you for the pointer for the CL.

[dmazzoni, 2011-10-19 05:01:15]

LGTM - thanks for doing this!

http://codereview.chromium.org/8334018/diff/11002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tts_api_chromeos.cc (right):

http://codereview.chromium.org/8334018/diff/11002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tts_api_chromeos.cc:48: void
ContinuePollingIfSpeechIsNotFinished(int utterance_id, bool result);
By the way, it would be fantastic if we could implement this with a callback
when speech completes rather than polling. There's already
http://crbug.com/67713 filed for this - could you take a look?

[satorux1, 2011-10-19 05:15:47]

http://codereview.chromium.org/8334018/diff/11002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tts_api_chromeos.cc (right):

http://codereview.chromium.org/8334018/diff/11002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tts_api_chromeos.cc:10: #include
"base/task.h"
I think base/task.h is unnecessary.

http://codereview.chromium.org/8334018/diff/11002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tts_api_chromeos.cc:48: void
ContinuePollingIfSpeechIsNotFinished(int utterance_id, bool result);
On 2011/10/19 05:01:15, Dominic Mazzoni wrote:
> By the way, it would be fantastic if we could implement this with a callback
> when speech completes rather than polling. There's already
> http://crbug.com/67713 filed for this - could you take a look?

Agreed. Polling is uncool. :)

One way to do it is to send a signal from the speech synthesis server once the
speech is done. Chrome can monitor the signal and get notified. This should be
reasonably easy, but not so clean.

Another way, which is cleaner but tricker to implement, is to to change Speak
method implementation in the server to defer returning the response until the
speech is done. This can be tricky to implement. Most D-Bus methods process
requests and return responses as soon as requests are delivered. Returning
responses at a later time would require asynchronous operation, which can be
tricky to implement.

[hashimoto, 2011-10-19 05:18:25]

http://codereview.chromium.org/8334018/diff/11002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tts_api_chromeos.cc (right):

http://codereview.chromium.org/8334018/diff/11002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tts_api_chromeos.cc:48: void
ContinuePollingIfSpeechIsNotFinished(int utterance_id, bool result);
On 2011/10/19 05:01:15, Dominic Mazzoni wrote:
> By the way, it would be fantastic if we could implement this with a callback
> when speech completes rather than polling. There's already
> http://crbug.com/67713 filed for this - could you take a look?

OK, I will.
It looks that currently no signal is sent from speech synthesizer when a speech
is finished.
What is needed is modifying speech synthesizer to send a signal when a speech is
finished, and modifying Chromium to receive the signal and invoke TTS_EVENT_END.
 Is this right?

[dmazzoni, 2011-10-19 05:28:49]

On Tue, Oct 18, 2011 at 10:18 PM, <hashimoto@chromium.org> wrote:

> It looks that currently no signal is sent from speech synthesizer when a
> speech is finished.
> What is needed is modifying speech synthesizer to send a signal when a
> speech is finished, and modifying Chromium to receive the signal and
> invoke TTS_EVENT_END.  Is this right?
>

That sounds right. Thanks!

I guess it would have to listen for a dbus disconnected notice also - my
only other worry is that if the speech synthesis service crashed or
something, we should make sure it doesn't hang Chrome too.

- Dominic

[hashimoto, 2011-10-19 05:40:07]

On 2011/10/19 05:15:47, satorux1 wrote:
>
http://codereview.chromium.org/8334018/diff/11002/chrome/browser/extensions/e...
> File chrome/browser/extensions/extension_tts_api_chromeos.cc (right):
> 
>
http://codereview.chromium.org/8334018/diff/11002/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_tts_api_chromeos.cc:10: #include
> "base/task.h"
> I think base/task.h is unnecessary.
> 
>
http://codereview.chromium.org/8334018/diff/11002/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_tts_api_chromeos.cc:48: void
> ContinuePollingIfSpeechIsNotFinished(int utterance_id, bool result);
> On 2011/10/19 05:01:15, Dominic Mazzoni wrote:
> > By the way, it would be fantastic if we could implement this with a callback
> > when speech completes rather than polling. There's already
> > http://crbug.com/67713 filed for this - could you take a look?
> 
> Agreed. Polling is uncool. :)
> 
> One way to do it is to send a signal from the speech synthesis server once the
> speech is done. Chrome can monitor the signal and get notified. This should be
> reasonably easy, but not so clean.
> 
> Another way, which is cleaner but tricker to implement, is to to change Speak
> method implementation in the server to defer returning the response until the
> speech is done. This can be tricky to implement. Most D-Bus methods process
> requests and return responses as soon as requests are delivered. Returning
> responses at a later time would require asynchronous operation, which can be
> tricky to implement.
Speech Synthesizer is implemented with DBus GLib now.
Is it possible with DBus GLib to delay response of Speak while processing other
methods?

[dmazzoni, 2011-10-19 05:45:08]

On Tue, Oct 18, 2011 at 10:40 PM, <hashimoto@chromium.org> wrote:

> One way to do it is to send a signal from the speech synthesis server once
>> the
>> speech is done. Chrome can monitor the signal and get notified. This
>> should be
>> reasonably easy, but not so clean.
>>
>
>  Another way, which is cleaner but tricker to implement, is to to change
>> Speak
>> method implementation in the server to defer returning the response until
>> the
>> speech is done. This can be tricky to implement. Most D-Bus methods
>> process
>> requests and return responses as soon as requests are delivered. Returning
>> responses at a later time would require asynchronous operation, which can
>> be
>> tricky to implement.
>>
> Speech Synthesizer is implemented with DBus GLib now.
> Is it possible with DBus GLib to delay response of Speak while processing
> other
> methods?
>

This seems more dangerous to me in the long run. I think that the first
solution - sending a signal when speech is done - is the right way. The
only case to worry about is if the server crashes or otherwise loses its
connection to Chrome.

- Dominic

[hashimoto, 2011-10-19 06:02:14]

New patch set, I will commit this.

http://codereview.chromium.org/8334018/diff/11002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tts_api_chromeos.cc (right):

http://codereview.chromium.org/8334018/diff/11002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tts_api_chromeos.cc:10: #include
"base/task.h"
On 2011/10/19 05:15:47, satorux1 wrote:
> I think base/task.h is unnecessary.
Removed.

It seems that this include and |method_factory_| are mistakenly revived while
merging.
Thank you for pointing it out.

[commit-bot: I haz the power, 2011-10-19 06:02:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/8334018/10025

[commit-bot: I haz the power, 2011-10-19 08:19:47]

Change committed as 106246