Implement web speech synthesis.

Implement web speech synthesis.

Refactors TtsController a bit so that it can be used
both by Tts extensions and by web speech. Implements
almost all of the features of the web speech API.

Requires corresponding WebKit patch:
https://bugs.webkit.org/show_bug.cgi?id=111695

BUG=171887
NOTRY=true

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

[dmazzoni, 2013-03-07 08:58:54]

This is ready for an initial look.

I mostly used speech recognition as a guide, but made a few
different design decisions. Please feel free to question
anything - I tried to get all of the pieces in place, but
I'm sure some of it could be more clear.

(Feedback welcome on the WebKit side, too!)

[tommi (sloooow) - chröme, 2013-03-07 13:04:46]

Looks pretty great overall.  I don't know anything about the webkit side or
actually how this really works! :) but tried to give helpful comments.

https://codereview.chromium.org/12589005/diff/1/chrome/browser/chrome_content...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/12589005/diff/1/chrome/browser/chrome_content...
chrome/browser/chrome_content_browser_client.cc:1681: return new
TtsRequestHandler();
it would be great to start using scoped_ptr's support for passing ownership in
these methods.  Also, I would personally prefer to use the "Create" prefix
instead of "Get".  Can we change the method like so?

scoped_ptr<content::TtsDispatcherHostDelegate>
ChromeContentBrowserClient::CreateTtsDispatcherHostDelegate()

Imho GetSpeechRecognitionManagerDelegate should be renamed as well, so please
add a todo for me to fix it.  GetNetLog() and CreateAccessTokenStore() below are
appropriately named at least as far as Get vs Create goes.

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/extensi...
File chrome/browser/speech/extension_api/tts_engine_extension_api.cc (right):

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/extensi...
chrome/browser/speech/extension_api/tts_engine_extension_api.cc:67:
out_voices->push_back(VoiceData());
nit: if you can assume that |out_voices| is always empty when the function is
called (and maybe it might be worth DCHECK()ing that), then you can call
out_voices->resize(tts_voices->size()) prior to the loop and then at(i) for both
tts_voices and out_voices and remove push_back().

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/extensi...
chrome/browser/speech/extension_api/tts_engine_extension_api.cc:75: for
(std::set<std::string>::const_iterator iter =
std::copy now that both variables are stl?

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/extensi...
File chrome/browser/speech/extension_api/tts_extension_api.cc (right):

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/extensi...
chrome/browser/speech/extension_api/tts_extension_api.cc:28: std::string
TtsEventTypeToString(TtsEventType event_type) {
nit: is there a reason for returning a string object and not just a static
string?

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/extensi...
chrome/browser/speech/extension_api/tts_extension_api.cc:103: delete this;
thinking out loud here - is there some way for us to guarantee that the caller
immediately nulls the dangling pointer to the deleted object?  This feels like a
scenario where a weak ptr might be the safe way to go but that might require
significant changes for all I know.

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/extensi...
chrome/browser/speech/extension_api/tts_extension_api.cc:267: for (size_t i = 0;
i < voices.size(); i++) {
++i

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/extensi...
chrome/browser/speech/extension_api/tts_extension_api.cc:271: if
(!voice.lang.empty())
I'm guessing that if all of these are empty, then that'd be an error, so maybe
you'd want to dcheck that:
DCHECK(!voice.lang.empty() || !voice.gender.empty() ||
       !voice.extension_id.empty());

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/extensi...
chrome/browser/speech/extension_api/tts_extension_api.cc:279: for (size_t j = 0;
j < voice.events.size(); j++)
++j

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_con...
File chrome/browser/speech/tts_controller.cc (right):

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_con...
chrome/browser/speech/tts_controller.cc:89: event_delegate_->OnTtsEvent(this,
event_type, char_index, error_message);
if finished_ is true, then should we set the event_delegate_ to NULL here?
(since we must assume it has been deleted)

Actually, I see that |finished_ = true| can be set via Finish() below, so it's
theoratically possible that finished_ is true while IsFinalTtsEventType()
returns false.  That would mean that the delegate would not self delete.  If we
hit that case, is that a programmer error or something we should expect?

Would this DCHECK make sense?

DCHECK_EQ(finished_, IsFinalTtsEventType(event_type));

If not, then I think we need to keep a local flag here as well for the
IsFinalTtsEventType() result and do something like:

if (event_delegate_) {
  event_delegate_->OnTtsEvent(this, event_type, char_index, error_message);
  if (is_final_tts_event_type)
    event_delegate_ = NULL;  // The delegate has been deleted.
}

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_con...
File chrome/browser/speech/tts_controller.h (right):

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_con...
chrome/browser/speech/tts_controller.h:37: 
nit: one empty line is fine as a separator (here and below)

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_con...
chrome/browser/speech/tts_controller.h:203: UtteranceEventDelegate*
event_delegate_;
document ownership?

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_mes...
File chrome/browser/speech/tts_message_filter.cc (right):

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_mes...
chrome/browser/speech/tts_message_filter.cc:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
I think that a decision was made to not change the copyright year.

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_mes...
chrome/browser/speech/tts_message_filter.cc:9: #include
"chrome/common/spellcheck_messages.h" /////////////
remove ///////?

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_mes...
chrome/browser/speech/tts_message_filter.cc:136: Profile* profile =
Profile::FromBrowserContext(host->GetBrowserContext());
I seem to recall FromBrowserContext return NULL in some cases.  I did a quick
check and do see some places that check for NULL.  Maybe that doesn't apply
here, but you might want to double check.

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_req...
File chrome/browser/speech/tts_request_handler.cc (right):

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_req...
chrome/browser/speech/tts_request_handler.cc:82: Profile* profile =
Profile::FromBrowserContext(context);
check FromBrowserContext (here and elsewhere)

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_req...
chrome/browser/speech/tts_request_handler.cc:88: for (size_t i = 0; i <
voices.size(); i++) {
++i

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_req...
chrome/browser/speech/tts_request_handler.cc:89:
out_voices.push_back(content::TtsVoice());
do out_voices.reset(voices.size()) outside the loop instead?

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_req...
chrome/browser/speech/tts_request_handler.cc:126: // Not supported by
TtsController yet.
NOTIMPLEMENTED() or TODO?

https://codereview.chromium.org/12589005/diff/1/content/browser/speech/tts_di...
File content/browser/speech/tts_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/12589005/diff/1/content/browser/speech/tts_di...
content/browser/speech/tts_dispatcher_host_impl.cc:45: BrowserThread::UI,
FROM_HERE,
out of curiosity, why do we do this on the ui thread?

https://codereview.chromium.org/12589005/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12589005/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:6429: //#if
defined(ENABLE_SPEECH_SYNTHESIS)
uncomment or remove?

https://codereview.chromium.org/12589005/diff/1/content/renderer/tts_dispatch...
File content/renderer/tts_dispatcher.cc (right):

https://codereview.chromium.org/12589005/diff/1/content/renderer/tts_dispatch...
content/renderer/tts_dispatcher.cc:88: for (size_t i = 0; i < voices.size();
i++) {
++i

https://codereview.chromium.org/12589005/diff/1/content/renderer/tts_dispatch...
File content/renderer/tts_dispatcher.h (right):

https://codereview.chromium.org/12589005/diff/1/content/renderer/tts_dispatch...
content/renderer/tts_dispatcher.h:55: WebKit::WebSpeechSynthesisClient*
synthesis_client_;
document ownership/scope?

[dmazzoni, 2013-03-07 17:48:13]

On Thu, Mar 7, 2013 at 5:04 AM, <tommi@chromium.org> wrote:

> Looks pretty great overall.  I don't know anything about the webkit side or
> actually how this really works! :) but tried to give helpful comments.
>

Thanks for the review! I'm relying on others who understand TtsController
and the TTS implementation, and the WebKit side, but I'm hoping that if you
treat those two as black boxes, you can be comfortable with all of the
details of this middle IPC / dispatcher bit. Otherwise please recommend
anyone else I should have as a reviewer.

thinking out loud here - is there some way for us to guarantee that the
> caller immediately nulls the dangling pointer to the deleted object?
> This feels like a scenario where a weak ptr might be the safe way to go
> but that might require significant changes for all I know.
>

The protocol used throughout TTS is that once an utterance has a "final"
event fired on it (as determined by IsFinalTtsEventType), that utterance is
no longer valid and should be deleted automatically. It's not clear if we
can enforce that in code, I focused more on having a simple and consistent
rule.

I guess the Utterance could own the Delegate, but if so I think it might
need a different name. Any thoughts?

Actually, I see that |finished_ = true| can be set via Finish() below,
> so it's theoratically possible that finished_ is true while
> IsFinalTtsEventType() returns false.  That would mean that the delegate
> would not self delete.  If we hit that case, is that a programmer error
> or something we should expect?
>

An utterance shouldn't fire any more events if finished is true, no matter
how it got there. But yeah, let's have the utterance own the delegate so
there's no way to get it wrong - the only question is how to make that
ownership more clear.

chrome/browser/speech/tts_**message_filter.cc:1: // Copyright (c) 2013 The
> Chromium Authors. All rights reserved.
> I think that a decision was made to not change the copyright year.
>

I'm confused - I thought we're supposed to add a valid copyright with the
current year for new files.

https://codereview.chromium.**org/12589005/diff/1/content/**
>
browser/speech/tts_dispatcher_**host_impl.cc#newcode45<https://codereview.chromium.org/12589005/diff/1/content/browser/speech/tts_dispatcher_host_impl.cc#newcode45>
> content/browser/speech/tts_**dispatcher_host_impl.cc:45:
> BrowserThread::UI, FROM_HERE,
> out of curiosity, why do we do this on the ui thread?
>

Mapping a BrowserContext to a Profile must be done on the UI thread.
TtsController also currently operates on the UI thread, but I actually may
consider changing that later to reduce jank.

https://codereview.chromium.**org/12589005/diff/1/content/**
>
renderer/render_view_impl.cc#**newcode6429<https://codereview.chromium.org/12589005/diff/1/content/renderer/render_view_impl.cc#newcode6429>
> content/renderer/render_view_**impl.cc:6429: //#if
> defined(ENABLE_SPEECH_**SYNTHESIS)
> uncomment or remove?
>

Do you think this needs a feature flag? Or a runtime flag? Or neither,
because we can just toggle the feature in WebKit and this code is safe
either way?

[tommi (sloooow) - chröme, 2013-03-07 19:46:30]

Regarding the copyright year, I think that either rietveld or git cl upload
is confusing both of us!  In the diff view, it doesn't show up as a new
file, but now that I look at the file list, I see that it's indeed a new
file.  In any case, just ignore my comment, 2013 is correct.

Regarding the ENABLE_SPEECH_SYNTHESIS flag, I wasn't sure if you meant to
check the code in with the #ifdef commented out, uncomment it or if you
intended to remove it.  Toggling the feature in webkit sounds good to me.


On Thu, Mar 7, 2013 at 6:46 PM, Dominic Mazzoni <dmazzoni@chromium.org>wrote:

> On Thu, Mar 7, 2013 at 5:04 AM, <tommi@chromium.org> wrote:
>
>> Looks pretty great overall.  I don't know anything about the webkit side
>> or
>> actually how this really works! :) but tried to give helpful comments.
>>
>
> Thanks for the review! I'm relying on others who understand TtsController
> and the TTS implementation, and the WebKit side, but I'm hoping that if you
> treat those two as black boxes, you can be comfortable with all of the
> details of this middle IPC / dispatcher bit. Otherwise please recommend
> anyone else I should have as a reviewer.
>
> thinking out loud here - is there some way for us to guarantee that the
>> caller immediately nulls the dangling pointer to the deleted object?
>> This feels like a scenario where a weak ptr might be the safe way to go
>> but that might require significant changes for all I know.
>>
>
> The protocol used throughout TTS is that once an utterance has a "final"
> event fired on it (as determined by IsFinalTtsEventType), that utterance is
> no longer valid and should be deleted automatically. It's not clear if we
> can enforce that in code, I focused more on having a simple and consistent
> rule.
>
> I guess the Utterance could own the Delegate, but if so I think it might
> need a different name. Any thoughts?
>
> Actually, I see that |finished_ = true| can be set via Finish() below,
>> so it's theoratically possible that finished_ is true while
>> IsFinalTtsEventType() returns false.  That would mean that the delegate
>> would not self delete.  If we hit that case, is that a programmer error
>> or something we should expect?
>>
>
> An utterance shouldn't fire any more events if finished is true, no matter
> how it got there. But yeah, let's have the utterance own the delegate so
> there's no way to get it wrong - the only question is how to make that
> ownership more clear.
>
> chrome/browser/speech/tts_**message_filter.cc:1: // Copyright (c) 2013 The
>> Chromium Authors. All rights reserved.
>> I think that a decision was made to not change the copyright year.
>>
>
> I'm confused - I thought we're supposed to add a valid copyright with the
> current year for new files.
>
> https://codereview.chromium.**org/12589005/diff/1/content/**
>>
browser/speech/tts_dispatcher_**host_impl.cc#newcode45<https://codereview.chromium.org/12589005/diff/1/content/browser/speech/tts_dispatcher_host_impl.cc#newcode45>
>> content/browser/speech/tts_**dispatcher_host_impl.cc:45:
>> BrowserThread::UI, FROM_HERE,
>> out of curiosity, why do we do this on the ui thread?
>>
>
> Mapping a BrowserContext to a Profile must be done on the UI thread.
> TtsController also currently operates on the UI thread, but I actually may
> consider changing that later to reduce jank.
>
> https://codereview.chromium.**org/12589005/diff/1/content/**
>>
renderer/render_view_impl.cc#**newcode6429<https://codereview.chromium.org/12589005/diff/1/content/renderer/render_view_impl.cc#newcode6429>
>> content/renderer/render_view_**impl.cc:6429: //#if
>> defined(ENABLE_SPEECH_**SYNTHESIS)
>> uncomment or remove?
>>
>
> Do you think this needs a feature flag? Or a runtime flag? Or neither,
> because we can just toggle the feature in WebKit and this code is safe
> either way?
>
>

[hans, 2013-03-09 14:19:52]

I looked at the WebKit patch too, but I see that Adam had some comments about
the architecture there so I'm holding off reviewing it until that stuff is
figured out.

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/extensi...
File chrome/browser/speech/extension_api/tts_engine_extension_api.cc (right):

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/extensi...
chrome/browser/speech/extension_api/tts_engine_extension_api.cc:68: VoiceData*
result_voice = &out_voices->back();
i would have made result_voice a reference instead, but I don't think it matters
much

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/extensi...
File chrome/browser/speech/extension_api/tts_extension_api.cc (right):

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/extensi...
chrome/browser/speech/extension_api/tts_extension_api.cc:82: DictionaryValue*
details = new DictionaryValue();
maybe make this a scoped_ptr?

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/extensi...
chrome/browser/speech/extension_api/tts_extension_api.cc:269: DictionaryValue*
result_voice = new DictionaryValue();
scoped_ptr?

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/extensi...
chrome/browser/speech/extension_api/tts_extension_api.cc:278: ListValue*
event_types = new ListValue();
scoped_ptr?

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/extensi...
chrome/browser/speech/extension_api/tts_extension_api.cc:281:
result_voice->Set(constants::kEventTypesKey, event_types);
naive question because I don't have the whole picture: a list of strings seems
like a pretty inefficient way of representing a set of event types. Is the
different types of events fixed, and if so could the set of event types be
represented as a bitmask?

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_con...
File chrome/browser/speech/tts_controller.cc (right):

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_con...
chrome/browser/speech/tts_controller.cc:216: if (platform_impl &&
platform_impl->PlatformImplAvailable()) {
maybe return early instead when this false?

oh, never mind - i now see the GetExtensionVoices call below.

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_con...
chrome/browser/speech/tts_controller.cc:218: VoiceData* voice =
&out_voices->back();
i would have gone for a reference here, but no big deal

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_mes...
File chrome/browser/speech/tts_message_filter.cc (right):

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_mes...
chrome/browser/speech/tts_message_filter.cc:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
On 2013/03/07 13:04:46, tommi wrote:
> I think that a decision was made to not change the copyright year.

something is weird here.. it seems this is a new file, but it was similar enough
to spellcheck_message_filter.cc that the tools are showing the diffs against
that. If it's a new file, I think using 2013 is the right thing.

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_mes...
chrome/browser/speech/tts_message_filter.cc:26: if (message.type() ==
SpellCheckHostMsg_RequestDictionary::ID ||
what's up with all the SpellCheck stuff here? am I missing something?

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_mes...
chrome/browser/speech/tts_message_filter.cc:39:
IPC_MESSAGE_HANDLER(SpellCheckHostMsg_RequestDictionary,
again with the spellcheck.. i'm not sure i understand this file at all

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_req...
File chrome/browser/speech/tts_request_handler.cc (right):

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_req...
chrome/browser/speech/tts_request_handler.cc:106: Utterance* utterance = new
Utterance(profile);
scoped_ptr? Then it would be more clear that we're handing off the ownership
with SpeakOrEnqueue(utterance.Pass()) below.

https://codereview.chromium.org/12589005/diff/1/content/browser/speech/tts_di...
File content/browser/speech/tts_dispatcher_host_impl.h (right):

https://codereview.chromium.org/12589005/diff/1/content/browser/speech/tts_di...
content/browser/speech/tts_dispatcher_host_impl.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
2013 since it's a new file?

[dmazzoni, 2013-03-13 19:27:45]

Hi! Just an FYI, I just refactored the WebKit portion and I'm going to wait
until that's in a little better shape before I address the rest of your
feedback.

I'll let you know when it's ready.

[dmazzoni, 2013-03-19 17:30:22]

Ready for another look!

I just got an r+ on the WebKit side so this should be
ready shortly. I'll probably try to commit this right
after we branch Chrome 27 so we can test it in dev for
a while and hopefully land in Chrome 28.

https://codereview.chromium.org/12589005/diff/1/chrome/browser/chrome_content...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/12589005/diff/1/chrome/browser/chrome_content...
chrome/browser/chrome_content_browser_client.cc:1681: return new
TtsRequestHandler();
On 2013/03/07 13:04:46, tommi wrote:
> it would be great to start using scoped_ptr's support for passing ownership in
> these methods.  Also, I would personally prefer to use the "Create" prefix
> instead of "Get".  Can we change the method like so?
> 
> scoped_ptr<content::TtsDispatcherHostDelegate>
> ChromeContentBrowserClient::CreateTtsDispatcherHostDelegate()
> 
> Imho GetSpeechRecognitionManagerDelegate should be renamed as well, so please
> add a todo for me to fix it.  GetNetLog() and CreateAccessTokenStore() below
are
> appropriately named at least as far as Get vs Create goes.

Cool - done, and added the TODO for you.

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/extensi...
File chrome/browser/speech/extension_api/tts_engine_extension_api.cc (right):

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/extensi...
chrome/browser/speech/extension_api/tts_engine_extension_api.cc:67:
out_voices->push_back(VoiceData());
On 2013/03/07 13:04:46, tommi wrote:
> nit: if you can assume that |out_voices| is always empty when the function is
> called (and maybe it might be worth DCHECK()ing that), then you can call
> out_voices->resize(tts_voices->size()) prior to the loop and then at(i) for
both
> tts_voices and out_voices and remove push_back().

Actually we can't assume that - there are also platform voices, we might be
appending onto the end.

Since this vector is so tiny, ok to leave it like this? I do wish there was a
one-liner to add to the end of the vector and get a pointer or reference...

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/extensi...
chrome/browser/speech/extension_api/tts_engine_extension_api.cc:68: VoiceData*
result_voice = &out_voices->back();
On 2013/03/09 14:19:52, hans wrote:
> i would have made result_voice a reference instead, but I don't think it
matters
> much

Done.

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/extensi...
chrome/browser/speech/extension_api/tts_engine_extension_api.cc:75: for
(std::set<std::string>::const_iterator iter =
On 2013/03/07 13:04:46, tommi wrote:
> std::copy now that both variables are stl?

I tried this and it crashes - any idea what I'm doing wrong?

std::copy(voice.event_types.begin(),
          voice.event_types.end(),
          result_voice.events.begin());

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/extensi...
File chrome/browser/speech/extension_api/tts_extension_api.cc (right):

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/extensi...
chrome/browser/speech/extension_api/tts_extension_api.cc:28: std::string
TtsEventTypeToString(TtsEventType event_type) {
On 2013/03/07 13:04:46, tommi wrote:
> nit: is there a reason for returning a string object and not just a static
> string?

Changed to const char *, that way no string copying is needed.

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/extensi...
chrome/browser/speech/extension_api/tts_extension_api.cc:82: DictionaryValue*
details = new DictionaryValue();
On 2013/03/09 14:19:52, hans wrote:
> maybe make this a scoped_ptr?

Done.

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/extensi...
chrome/browser/speech/extension_api/tts_extension_api.cc:103: delete this;
On 2013/03/07 13:04:46, tommi wrote:
> thinking out loud here - is there some way for us to guarantee that the caller
> immediately nulls the dangling pointer to the deleted object?  This feels like
a
> scenario where a weak ptr might be the safe way to go but that might require
> significant changes for all I know.

Since TTS spans processes, we use a simple convention to manage memory: every
utterance must get a "final" callback (the current list is one of "end",
"interrupted", "cancelled", or "error"), and after that it must be deleted; no
more callbacks are allowed to be sent on that utterance.

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/extensi...
chrome/browser/speech/extension_api/tts_extension_api.cc:267: for (size_t i = 0;
i < voices.size(); i++) {
On 2013/03/07 13:04:46, tommi wrote:
> ++i

Done throughout

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/extensi...
chrome/browser/speech/extension_api/tts_extension_api.cc:269: DictionaryValue*
result_voice = new DictionaryValue();
On 2013/03/09 14:19:52, hans wrote:
> scoped_ptr?

Same - this gets owned by the ListValue.

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/extensi...
chrome/browser/speech/extension_api/tts_extension_api.cc:271: if
(!voice.lang.empty())
On 2013/03/07 13:04:46, tommi wrote:
> I'm guessing that if all of these are empty, then that'd be an error, so maybe
> you'd want to dcheck that:
> DCHECK(!voice.lang.empty() || !voice.gender.empty() ||
>        !voice.extension_id.empty());

Well, it still has a name...

Currently support for platform-native voices is a little weak, we don't expose
the lang or gender for those. I'd like to add the DCHECK when that support is
better.

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/extensi...
chrome/browser/speech/extension_api/tts_extension_api.cc:281:
result_voice->Set(constants::kEventTypesKey, event_types);
On 2013/03/09 14:19:52, hans wrote:
> naive question because I don't have the whole picture: a list of strings seems
> like a pretty inefficient way of representing a set of event types. Is the
> different types of events fixed, and if so could the set of event types be
> represented as a bitmask?

It's done this way because it's a JavaScript API. Currently the list of
supported events is only needed for the extension API - if the web API ends up
having a similar API then it'd probably make sense to switch to a bitmap and
translate to a list of strings downstream.

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_con...
File chrome/browser/speech/tts_controller.cc (right):

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_con...
chrome/browser/speech/tts_controller.cc:89: event_delegate_->OnTtsEvent(this,
event_type, char_index, error_message);
On 2013/03/07 13:04:46, tommi wrote:
> if finished_ is true, then should we set the event_delegate_ to NULL here?
> (since we must assume it has been deleted)

Good idea, done.

> Actually, I see that |finished_ = true| can be set via Finish() below, so it's
> theoratically possible that finished_ is true while IsFinalTtsEventType()
> returns false.  That would mean that the delegate would not self delete.  If
we
> hit that case, is that a programmer error or something we should expect?

You're right, but that should only happen when TtsController is being destroyed
- and since it's a singleton, that should only happen at program shutdown.

In fact, that call to Finish is only there to prevent other checks from firing.

> Would this DCHECK make sense?
> 
> DCHECK_EQ(finished_, IsFinalTtsEventType(event_type));

...only if we could bypass it when TtsController is being destroyed, because
then it's not safe to send callback messages.

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_con...
chrome/browser/speech/tts_controller.cc:218: VoiceData* voice =
&out_voices->back();
On 2013/03/09 14:19:52, hans wrote:
> i would have gone for a reference here, but no big deal

Done.

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_con...
File chrome/browser/speech/tts_controller.h (right):

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_con...
chrome/browser/speech/tts_controller.h:37: 
On 2013/03/07 13:04:46, tommi wrote:
> nit: one empty line is fine as a separator (here and below)

Done.

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_con...
chrome/browser/speech/tts_controller.h:203: UtteranceEventDelegate*
event_delegate_;
On 2013/03/07 13:04:46, tommi wrote:
> document ownership?

Done.

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_mes...
File chrome/browser/speech/tts_message_filter.cc (right):

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_mes...
chrome/browser/speech/tts_message_filter.cc:26: if (message.type() ==
SpellCheckHostMsg_RequestDictionary::ID ||
On 2013/03/09 14:19:52, hans wrote:
> what's up with all the SpellCheck stuff here? am I missing something?

Sorry, my mistake - I switched implementation strategies halfway through and
never meant to include this file in the upload. It doesn't compile and isn't
needed. :)

Sorry!

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_req...
File chrome/browser/speech/tts_request_handler.cc (right):

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_req...
chrome/browser/speech/tts_request_handler.cc:82: Profile* profile =
Profile::FromBrowserContext(context);
On 2013/03/07 13:04:46, tommi wrote:
> check FromBrowserContext (here and elsewhere)

Done.

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_req...
chrome/browser/speech/tts_request_handler.cc:88: for (size_t i = 0; i <
voices.size(); i++) {
On 2013/03/07 13:04:46, tommi wrote:
> ++i

Done.

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_req...
chrome/browser/speech/tts_request_handler.cc:89:
out_voices.push_back(content::TtsVoice());
On 2013/03/07 13:04:46, tommi wrote:
> do out_voices.reset(voices.size()) outside the loop instead?

Agreed here, no reason not to.

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_req...
chrome/browser/speech/tts_request_handler.cc:106: Utterance* utterance = new
Utterance(profile);
On 2013/03/09 14:19:52, hans wrote:
> scoped_ptr? Then it would be more clear that we're handing off the ownership
> with SpeakOrEnqueue(utterance.Pass()) below.

Done.

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/tts_req...
chrome/browser/speech/tts_request_handler.cc:126: // Not supported by
TtsController yet.
On 2013/03/07 13:04:46, tommi wrote:
> NOTIMPLEMENTED() or TODO?

Done.

https://codereview.chromium.org/12589005/diff/1/content/browser/speech/tts_di...
File content/browser/speech/tts_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/12589005/diff/1/content/browser/speech/tts_di...
content/browser/speech/tts_dispatcher_host_impl.cc:45: BrowserThread::UI,
FROM_HERE,
On 2013/03/07 13:04:46, tommi wrote:
> out of curiosity, why do we do this on the ui thread?

Currently TtsController is implemented on the UI thread, that's where all
extension messages get sent.

Perhaps it should start its own thread? If you think that'd
be a good idea I can add a bug.

https://codereview.chromium.org/12589005/diff/1/content/browser/speech/tts_di...
File content/browser/speech/tts_dispatcher_host_impl.h (right):

https://codereview.chromium.org/12589005/diff/1/content/browser/speech/tts_di...
content/browser/speech/tts_dispatcher_host_impl.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2013/03/09 14:19:52, hans wrote:
> 2013 since it's a new file?

Done.

https://codereview.chromium.org/12589005/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12589005/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:6429: //#if
defined(ENABLE_SPEECH_SYNTHESIS)
On 2013/03/07 13:04:46, tommi wrote:
> uncomment or remove?

Done.

https://codereview.chromium.org/12589005/diff/1/content/renderer/tts_dispatch...
File content/renderer/tts_dispatcher.cc (right):

https://codereview.chromium.org/12589005/diff/1/content/renderer/tts_dispatch...
content/renderer/tts_dispatcher.cc:88: for (size_t i = 0; i < voices.size();
i++) {
On 2013/03/07 13:04:46, tommi wrote:
> ++i

Done.

https://codereview.chromium.org/12589005/diff/1/content/renderer/tts_dispatch...
File content/renderer/tts_dispatcher.h (right):

https://codereview.chromium.org/12589005/diff/1/content/renderer/tts_dispatch...
content/renderer/tts_dispatcher.h:55: WebKit::WebSpeechSynthesisClient*
synthesis_client_;
On 2013/03/07 13:04:46, tommi wrote:
> document ownership/scope?

Done.

[tommi (sloooow) - chröme, 2013-03-21 14:34:02]

lgtm

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/extensi...
File chrome/browser/speech/extension_api/tts_engine_extension_api.cc (right):

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/extensi...
chrome/browser/speech/extension_api/tts_engine_extension_api.cc:67:
out_voices->push_back(VoiceData());
On 2013/03/19 17:30:22, Dominic Mazzoni wrote:
> On 2013/03/07 13:04:46, tommi wrote:
> > nit: if you can assume that |out_voices| is always empty when the function
is
> > called (and maybe it might be worth DCHECK()ing that), then you can call
> > out_voices->resize(tts_voices->size()) prior to the loop and then at(i) for
> both
> > tts_voices and out_voices and remove push_back().
> 
> Actually we can't assume that - there are also platform voices, we might be
> appending onto the end.
> 
> Since this vector is so tiny, ok to leave it like this? I do wish there was a
> one-liner to add to the end of the vector and get a pointer or reference...

OK as is

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/extensi...
chrome/browser/speech/extension_api/tts_engine_extension_api.cc:75: for
(std::set<std::string>::const_iterator iter =
On 2013/03/19 17:30:22, Dominic Mazzoni wrote:
> On 2013/03/07 13:04:46, tommi wrote:
> > std::copy now that both variables are stl?
> 
> I tried this and it crashes - any idea what I'm doing wrong?
> 
> std::copy(voice.event_types.begin(),
>           voice.event_types.end(),
>           result_voice.events.begin());

hmm... maybe you need to do something like:

result_voice->events.reserve(voice.event_types.size() +
                             result_voice->events.size());
std::copy(voice.event_types.begin(), voice.event_types.end(),
          std::back_inserter(result_voice->events.begin()));

Anyway, it's a total nit and getting into stl trivia territory so if it gives
you problems, just leave as is.

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/extensi...
File chrome/browser/speech/extension_api/tts_extension_api.cc (right):

https://codereview.chromium.org/12589005/diff/1/chrome/browser/speech/extensi...
chrome/browser/speech/extension_api/tts_extension_api.cc:271: if
(!voice.lang.empty())
On 2013/03/19 17:30:22, Dominic Mazzoni wrote:
> On 2013/03/07 13:04:46, tommi wrote:
> > I'm guessing that if all of these are empty, then that'd be an error, so
maybe
> > you'd want to dcheck that:
> > DCHECK(!voice.lang.empty() || !voice.gender.empty() ||
> >        !voice.extension_id.empty());
> 
> Well, it still has a name...
> 
> Currently support for platform-native voices is a little weak, we don't expose
> the lang or gender for those. I'd like to add the DCHECK when that support is
> better.

OK

https://codereview.chromium.org/12589005/diff/1/content/browser/speech/tts_di...
File content/browser/speech/tts_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/12589005/diff/1/content/browser/speech/tts_di...
content/browser/speech/tts_dispatcher_host_impl.cc:45: BrowserThread::UI,
FROM_HERE,
On 2013/03/19 17:30:22, Dominic Mazzoni wrote:
> On 2013/03/07 13:04:46, tommi wrote:
> > out of curiosity, why do we do this on the ui thread?
> 
> Currently TtsController is implemented on the UI thread, that's where all
> extension messages get sent.
> 
> Perhaps it should start its own thread? If you think that'd
> be a good idea I can add a bug.

As is I can't tell if it's a good idea or not :)
Let's file a bug if it turns out to be a problem.

https://codereview.chromium.org/12589005/diff/19001/chrome/browser/speech/ext...
File chrome/browser/speech/extension_api/tts_extension_api.cc (right):

https://codereview.chromium.org/12589005/diff/19001/chrome/browser/speech/ext...
chrome/browser/speech/extension_api/tts_extension_api.cc:23: const char
kOnEvent[] = "tts.onEvent";
nit: no indent

[hans, 2013-03-21 14:34:59]

lgtm too

[dmazzoni, 2013-03-29 15:47:06]

tsepez: could you please look at the ipcs?
brettw: could you do an owners review of content/?

WebKit half has landed, getting close!

https://codereview.chromium.org/12589005/diff/19001/chrome/browser/speech/ext...
File chrome/browser/speech/extension_api/tts_extension_api.cc (right):

https://codereview.chromium.org/12589005/diff/19001/chrome/browser/speech/ext...
chrome/browser/speech/extension_api/tts_extension_api.cc:23: const char
kOnEvent[] = "tts.onEvent";
On 2013/03/21 14:34:02, tommi wrote:
> nit: no indent

Done.

[Tom Sepez, 2013-03-29 16:51:16]

lgtm

[jam, 2013-03-29 17:30:11]

It looks like there's a delegate interface from WebKit, WebSpeechSynthesizer,
which is implemented in content which then delegates everything to its embedder.

Why not implement WebSpeechSynthesizer directly in chrome, so that we don't need
to have two sets of embedder interfaces related to this feature? i.e. see
WebSpellCheckClient, WebAutofillClient etc.

[dmazzoni, 2013-03-29 17:44:56]

On 2013/03/29 17:30:11, jam wrote:
> It looks like there's a delegate interface from WebKit, WebSpeechSynthesizer,
> which is implemented in content which then delegates everything to its
embedder.
> 
> Why not implement WebSpeechSynthesizer directly in chrome, so that we don't
need
> to have two sets of embedder interfaces related to this feature? i.e. see
> WebSpellCheckClient, WebAutofillClient etc.

Because speech is part of the web platform now, not just a Chrome feature.
Another browser built on Content should be able to implement speech, and in fact
we should refactor it so that basic speech works in Content Shell, minus
Chrome-specific features (like pluggable voices).

[jam, 2013-03-29 18:15:21]

On 2013/03/29 17:44:56, Dominic Mazzoni wrote:
> On 2013/03/29 17:30:11, jam wrote:
> > It looks like there's a delegate interface from WebKit,
WebSpeechSynthesizer,
> > which is implemented in content which then delegates everything to its
> embedder.
> > 
> > Why not implement WebSpeechSynthesizer directly in chrome, so that we don't
> need
> > to have two sets of embedder interfaces related to this feature? i.e. see
> > WebSpellCheckClient, WebAutofillClient etc.
> 
> Because speech is part of the web platform now, not just a Chrome feature.
> Another browser built on Content should be able to implement speech, and in
fact
> we should refactor it so that basic speech works in Content Shell, minus
> Chrome-specific features (like pluggable voices).

Agreed. However, the code as it stands, is just doing an extra layer of
plumbing.

So that's why I'd prefer if the implementation is in chrome, there's no plumbing
in content. If the implementation moves to content, then content can implement
the interfaces from WebKit.

[dmazzoni, 2013-03-29 18:26:17]

On 2013/03/29 18:15:21, jam wrote:
> Agreed. However, the code as it stands, is just doing an extra layer of
> plumbing.

What if I promise to start moving TTS into content as my very next change? I'd
prefer to just refactor TTS sooner, rather than move this plumbing to chrome
only to move it back to content later.

[jam, 2013-03-29 19:44:31]

On 2013/03/29 18:26:17, Dominic Mazzoni wrote:
> On 2013/03/29 18:15:21, jam wrote:
> > Agreed. However, the code as it stands, is just doing an extra layer of
> > plumbing.
> 
> What if I promise to start moving TTS into content as my very next change? I'd
> prefer to just refactor TTS sooner, rather than move this plumbing to chrome
> only to move it back to content later.

If TTS is is content though, wouldn't the content api changes look much
different than this?

[dmazzoni, 2013-03-29 20:45:34]

On 2013/03/29 19:44:31, jam wrote:
> On 2013/03/29 18:26:17, Dominic Mazzoni wrote:
> > On 2013/03/29 18:15:21, jam wrote:
> > > Agreed. However, the code as it stands, is just doing an extra layer of
> > > plumbing.
> > 
> > What if I promise to start moving TTS into content as my very next change?
I'd
> > prefer to just refactor TTS sooner, rather than move this plumbing to chrome
> > only to move it back to content later.
> 
> If TTS is is content though, wouldn't the content api changes look much
> different than this?

Fair enough. Maybe it'd be easier to start refactoring TTS first...let me fiddle
with it and get back to you.

[dmazzoni, 2013-04-29 22:58:04]

Sorry I neglected this for so long. Ready for another look,
it's now all in chrome and not content.

The only other change is adding a runtime flag.

[jam, 2013-04-30 17:11:19]

content lgtm

https://codereview.chromium.org/12589005/diff/45001/chrome/common/all_messages.h
File chrome/common/all_messages.h (right):

https://codereview.chromium.org/12589005/diff/45001/chrome/common/all_message...
chrome/common/all_messages.h:26: #include "chrome/common/tts_messages.h"
don't add it here, instead put this in chrome/common/common_message_generator.h

https://codereview.chromium.org/12589005/diff/45001/content/renderer/renderer...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/12589005/diff/45001/content/renderer/renderer...
content/renderer/renderer_webkitplatformsupport_impl.cc:865: return NULL;
nit:

return GetContentClient()->renderer()->OverrideSpeechSynthesizer(client);

[dmazzoni, 2013-05-02 22:13:25]

https://codereview.chromium.org/12589005/diff/45001/chrome/common/all_messages.h
File chrome/common/all_messages.h (right):

https://codereview.chromium.org/12589005/diff/45001/chrome/common/all_message...
chrome/common/all_messages.h:26: #include "chrome/common/tts_messages.h"
On 2013/04/30 17:11:20, jam wrote:
> don't add it here, instead put this in
chrome/common/common_message_generator.h

Done.

https://codereview.chromium.org/12589005/diff/45001/content/renderer/renderer...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/12589005/diff/45001/content/renderer/renderer...
content/renderer/renderer_webkitplatformsupport_impl.cc:865: return NULL;
On 2013/04/30 17:11:20, jam wrote:
> nit:
> 
> return GetContentClient()->renderer()->OverrideSpeechSynthesizer(client);

Done.

[commit-bot: I haz the power, 2013-05-02 22:15:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/12589005/65001

[commit-bot: I haz the power, 2013-05-03 04:00:46]

Retried try job too often on linux_chromeos for step(s) ash_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-05-03 04:39:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/12589005/65001

[commit-bot: I haz the power, 2013-05-03 04:39:35]

Change committed as 198060