Uses a system-wide notion of isSpeaking in the Mac extension TTS api.

Uses a system-wide notion of isSpeaking in the Mac extension TTS api.


BUG=none
TEST=manual.


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

[dmazzoni, 2012-04-09 23:30:32]

Have you tested this, does it work? I'm not sure the javascript isSpeaking call
directly calls the native isSpeaking method, you might need to change that
logic.

Basic idea sounds good, though. Please add a sentence to the docs, too, though -
so users know this will return if any known voice is speaking.

[David Tseng, 2012-04-10 00:23:00]

On 4/9/12, dmazzoni@chromium.org <dmazzoni@chromium.org> wrote:
> Have you tested this, does it work? I'm not sure the javascript isSpeaking
> call
> directly calls the native isSpeaking method, you might need to change that
> logic.

Actually, you're right; the platform impl interface
(TtsExtensionPlatformImpl) appears to be incorrectly specified.
Noteably, IsSpeaing is missing (the inherited platform specific
classes declare a virtual IsSpeaking method with no IsSpeaking base to
override). Probably something coverity would catch.

New patch up with changes to address this. Let me know if you agree
with the new behavior cross platform for is speaking within the
controller before I update docs.

[David Tseng, 2012-04-11 20:26:44]

PTAL; want to see if this is the general direction we want to go for
all platforms. isSpeaking seems currently independent of the native
API's statement of isSpeaking. This would change with the proposed
patch.

On 4/9/12, David Tseng <dtseng@chromium.org> wrote:
> On 4/9/12, dmazzoni@chromium.org <dmazzoni@chromium.org> wrote:
>> Have you tested this, does it work? I'm not sure the javascript
>> isSpeaking
>> call
>> directly calls the native isSpeaking method, you might need to change
>> that
>> logic.
>
> Actually, you're right; the platform impl interface
> (TtsExtensionPlatformImpl) appears to be incorrectly specified.
> Noteably, IsSpeaing is missing (the inherited platform specific
> classes declare a virtual IsSpeaking method with no IsSpeaking base to
> override). Probably something coverity would catch.
>
> New patch up with changes to address this. Let me know if you agree
> with the new behavior cross platform for is speaking within the
> controller before I update docs.
>

[dmazzoni, 2012-04-11 20:44:18]

Yes, I think this is a positive change. It doesn't take away any functionality
(users can still keep track of their use of the TTS API on their own) but it
exposes more information about native speech.

https://chromiumcodereview.appspot.com/9967021/diff/4001/chrome/browser/speec...
File chrome/browser/speech/extension_api/tts_extension_api_controller.cc
(right):

https://chromiumcodereview.appspot.com/9967021/diff/4001/chrome/browser/speec...
chrome/browser/speech/extension_api/tts_extension_api_controller.cc:287: return
current_utterance_ != NULL && GetPlatformImpl()->IsSpeaking();
Shouldn't this be || instead of &&

[David Tseng, 2012-04-11 22:20:47]

http://codereview.chromium.org/9967021/diff/4001/chrome/browser/speech/extens...
File chrome/browser/speech/extension_api/tts_extension_api_controller.cc
(right):

http://codereview.chromium.org/9967021/diff/4001/chrome/browser/speech/extens...
chrome/browser/speech/extension_api/tts_extension_api_controller.cc:287: return
current_utterance_ != NULL && GetPlatformImpl()->IsSpeaking();
On 2012/04/11 20:44:18, Dominic Mazzoni wrote:

You're right; the current && logic adds nothing. I don't think we expect
utterance to be non-null when isSpeaking returns true elsewhere?

[dmazzoni, 2012-04-12 06:56:07]

Looks good pending docs change

[David Tseng, 2012-04-13 16:55:03]

Which docs am I looking for (sites, in the Chrome repo, etc)?

Thanks,
David

On 4/11/12, dmazzoni@chromium.org <dmazzoni@chromium.org> wrote:
> Looks good pending docs change
>
> http://codereview.chromium.org/9967021/
>

[dmazzoni, 2012-04-13 17:03:28]

The files you should consider editing are:
chrome/common/extensions/docs/static/tts.html
chrome/common/extensions/api/tts.json

After editing one or both of these files, you need to build DumpRenderTree
and then run a script to rebuild the extension docs - instructions are here:
chrome/common/extensions/docs/README.txt

Then include all of the changed files in your changelist, although every
once in a while a completely unrelated file seems to get modified, you can
exclude that.

- Dominic

On Fri, Apr 13, 2012 at 9:54 AM, David Tseng <dtseng@chromium.org> wrote:

> Which docs am I looking for (sites, in the Chrome repo, etc)?
>
> Thanks,
> David
>
> On 4/11/12, dmazzoni@chromium.org <dmazzoni@chromium.org> wrote:
> > Looks good pending docs change
> >
> > http://codereview.chromium.org/9967021/
> >
>

[David Tseng, 2012-04-23 19:43:01]

PTAL; updated description of isSpeaking.

On 4/13/12, Dominic Mazzoni <dmazzoni@chromium.org> wrote:
> The files you should consider editing are:
> chrome/common/extensions/docs/static/tts.html
> chrome/common/extensions/api/tts.json
>
> After editing one or both of these files, you need to build DumpRenderTree
> and then run a script to rebuild the extension docs - instructions are here:
> chrome/common/extensions/docs/README.txt
>
> Then include all of the changed files in your changelist, although every
> once in a while a completely unrelated file seems to get modified, you can
> exclude that.
>
> - Dominic
>
> On Fri, Apr 13, 2012 at 9:54 AM, David Tseng <dtseng@chromium.org> wrote:
>
>> Which docs am I looking for (sites, in the Chrome repo, etc)?
>>
>> Thanks,
>> David
>>
>> On 4/11/12, dmazzoni@chromium.org <dmazzoni@chromium.org> wrote:
>> > Looks good pending docs change
>> >
>> > http://codereview.chromium.org/9967021/
>> >
>>
>

[dmazzoni, 2012-04-23 23:50:46]

lgtm

https://chromiumcodereview.appspot.com/9967021/diff/11001/chrome/common/exten...
File chrome/common/extensions/api/tts.json (right):

https://chromiumcodereview.appspot.com/9967021/diff/11001/chrome/common/exten...
chrome/common/extensions/api/tts.json:186: "description": "Reports if the engine
is currently speaking. Note: reports if any engines system wide is speaking when
using the native Mac OS X engine.",
how about this:
Note: On Mac OS X, this will return true if the system speech engine is
speaking, even if it wasn't initiated by Chrome.

[commit-bot: I haz the power, 2012-04-24 22:13:38]

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

[commit-bot: I haz the power, 2012-04-24 22:13:43]

Presubmit check for 9967021-16001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/common/extensions/api

Presubmit checks took 1.1s to calculate.

[David Tseng, 2012-04-24 22:14:31]

On 4/23/12, dmazzoni@chromium.org <dmazzoni@chromium.org> wrote:
> lgtm
>
>
>
>
>
https://chromiumcodereview.appspot.com/9967021/diff/11001/chrome/common/exten...
> File chrome/common/extensions/api/tts.json (right):
>
>
https://chromiumcodereview.appspot.com/9967021/diff/11001/chrome/common/exten...
> chrome/common/extensions/api/tts.json:186: "description": "Reports if
> the engine is currently speaking. Note: reports if any engines system
> wide is speaking when using the native Mac OS X engine.",
> how about this:
> Note: On Mac OS X, this will return true if the system speech engine is
> speaking, even if it wasn't initiated by Chrome.
>
Done.

> https://chromiumcodereview.appspot.com/9967021/
>

[David Tseng, 2012-04-24 22:17:26]

+ aa for chrome/common/extensions/api OWNERS approval.

On 4/24/12, David Tseng <dtseng@chromium.org> wrote:
> On 4/23/12, dmazzoni@chromium.org <dmazzoni@chromium.org> wrote:
>> lgtm
>>
>>
>>
>>
>>
https://chromiumcodereview.appspot.com/9967021/diff/11001/chrome/common/exten...
>> File chrome/common/extensions/api/tts.json (right):
>>
>>
https://chromiumcodereview.appspot.com/9967021/diff/11001/chrome/common/exten...
>> chrome/common/extensions/api/tts.json:186: "description": "Reports if
>> the engine is currently speaking. Note: reports if any engines system
>> wide is speaking when using the native Mac OS X engine.",
>> how about this:
>> Note: On Mac OS X, this will return true if the system speech engine is
>> speaking, even if it wasn't initiated by Chrome.
>>
> Done.
>
>> https://chromiumcodereview.appspot.com/9967021/
>>
>

[David Tseng, 2012-04-26 19:08:52]

+ kathyw for chrome/common/extensions OWNERS approval.

[kathyw, 2012-04-26 22:21:34]

http://codereview.chromium.org/9967021/diff/34001/chrome/common/extensions/ap...
File chrome/common/extensions/api/tts.json (right):

http://codereview.chromium.org/9967021/diff/34001/chrome/common/extensions/ap...
chrome/common/extensions/api/tts.json:186: "description": "Reports if the engine
is currently speaking. Note: On Mac OS X, this will return true if the system
speech engine is speaking, even if it wasn't initiated by Chrome.",
"Reports if" is ambiguous. How about "Checks whether...". ("Reports if" could be
construed as "is called iff".)

Also, I believe that "this will return true" isn't strictly correct. (there's no
return value)

How about:

Checks whether the engine is currently speaking. On Mac OS X, the result is true
whenever the system speech engine is speaking, even if the speech wasn't
initiated by Chrome.

[David Tseng, 2012-04-26 23:20:42]

http://codereview.chromium.org/9967021/diff/34001/chrome/common/extensions/ap...
File chrome/common/extensions/api/tts.json (right):

http://codereview.chromium.org/9967021/diff/34001/chrome/common/extensions/ap...
chrome/common/extensions/api/tts.json:186: "description": "Reports if the engine
is currently speaking. Note: On Mac OS X, this will return true if the system
speech engine is speaking, even if it wasn't initiated by Chrome.",
On 2012/04/26 22:21:34, kathyw wrote:

Done.

[kathyw, 2012-04-27 00:13:37]

lgtm

[commit-bot: I haz the power, 2012-04-27 00:28:47]

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

[commit-bot: I haz the power, 2012-04-27 01:57:51]

Change committed as 134209