Automatically trigger installation of high-quality speech synthesis extension.

Automatically trigger installation of high-quality speech synthesis extension.

When a certain threshold number of utterances are spoken in a given session
with the built-in speech synthesis, set a preference that enables
automatic download and (silent) installation of a component extension that
provides higher-quality speech synthesis.

This change also gets rid of the code that loaded the built-in speech
synthesis component extension on-demand and instead installs it when
loading all component extensions. It now uses an event page, so the
cost of loading it is minimal.

BUG=314718
R=asargent@chromium.org, dpolukhin@chromium.org, thestig@chromium.org

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

[dmazzoni, 2013-12-05 18:13:50]

@dpolukhin, could you do an initial review before I add more owners?

[Dmitry Polukhin, 2013-12-05 18:35:32]

In general things look good to me with few comments.

https://codereview.chromium.org/102263005/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/speech_synthesis_loader.cc (right):

https://codereview.chromium.org/102263005/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/speech_synthesis_loader.cc:30:
lang_to_extension_id_map_["en-US"] =
Nit, I suspect we will expand it in future so let's create const array of lang
to ID mapping. So move it from code to data.

https://codereview.chromium.org/102263005/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/speech_synthesis_loader.cc:44:
pref_service->GetList(prefs::kHighQualitySpeechSynthesisLanguages);
Why do we need more than one language here?

https://codereview.chromium.org/102263005/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/speech_synthesis_loader.h (right):

https://codereview.chromium.org/102263005/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/speech_synthesis_loader.h:45: Profile*
profile_;
Need to find out how to make it work with multiprofile. Do we want to enable
spoken feedback for all profile in session or keep it per profile?

[dmazzoni, 2013-12-05 23:19:47]

+kalman for extensions changes

https://codereview.chromium.org/102263005/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/speech_synthesis_loader.cc (right):

https://codereview.chromium.org/102263005/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/speech_synthesis_loader.cc:30:
lang_to_extension_id_map_["en-US"] =
On 2013/12/05 18:35:32, Dmitry Polukhin wrote:
> Nit, I suspect we will expand it in future so let's create const array of lang
> to ID mapping. So move it from code to data.

Done.

https://codereview.chromium.org/102263005/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/speech_synthesis_loader.cc:44:
pref_service->GetList(prefs::kHighQualitySpeechSynthesisLanguages);
On 2013/12/05 18:35:32, Dmitry Polukhin wrote:
> Why do we need more than one language here?

Suppose a French-Canadian user has their UI set to English, but they visit a lot
of French sites and read those using French TTS. I don't see any reason we
shouldn't upgrade both their English and French TTS voices.

https://codereview.chromium.org/102263005/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/speech_synthesis_loader.h (right):

https://codereview.chromium.org/102263005/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/speech_synthesis_loader.h:45: Profile*
profile_;
On 2013/12/05 18:35:32, Dmitry Polukhin wrote:
> Need to find out how to make it work with multiprofile. Do we want to enable
> spoken feedback for all profile in session or keep it per profile?

Currently TTS is all done per-profile - even on desktop Chrome, if you install a
new TTS voice from the web store in one profile, another profile opened in a
different window won't use it. I think it's unlikely this will affect very many
users negatively in practice, but if we change it, we should fix it throughout
the TTS system.

My understanding is that sometime later we'll have the ability for the same
extension installed in another profile to share the same storage - is that still
the plan? If so, I think that provides a bigger benefit.

[Dmitry Polukhin, 2013-12-05 23:29:19]

lgtm

https://codereview.chromium.org/102263005/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/speech_synthesis_loader.cc (right):

https://codereview.chromium.org/102263005/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/speech_synthesis_loader.cc:44:
pref_service->GetList(prefs::kHighQualitySpeechSynthesisLanguages);
On 2013/12/05 23:19:47, Dominic Mazzoni wrote:
> On 2013/12/05 18:35:32, Dmitry Polukhin wrote:
> > Why do we need more than one language here?
> 
> Suppose a French-Canadian user has their UI set to English, but they visit a
lot
> of French sites and read those using French TTS. I don't see any reason we
> shouldn't upgrade both their English and French TTS voices.

Sounds reasonable, I didn't think about this uses case. I thought about UI
language only. Thanks for the explanation!

https://codereview.chromium.org/102263005/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/speech_synthesis_loader.h (right):

https://codereview.chromium.org/102263005/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/speech_synthesis_loader.h:45: Profile*
profile_;
On 2013/12/05 23:19:47, Dominic Mazzoni wrote:
> On 2013/12/05 18:35:32, Dmitry Polukhin wrote:
> > Need to find out how to make it work with multiprofile. Do we want to enable
> > spoken feedback for all profile in session or keep it per profile?
> 
> Currently TTS is all done per-profile - even on desktop Chrome, if you install
a
> new TTS voice from the web store in one profile, another profile opened in a
> different window won't use it. I think it's unlikely this will affect very
many
> users negatively in practice, but if we change it, we should fix it throughout
> the TTS system.
> 
> My understanding is that sometime later we'll have the ability for the same
> extension installed in another profile to share the same storage - is that
still
> the plan? If so, I think that provides a bigger benefit.

On Chrome OS accessibility UI is available from system tray. So it is kind of
system wide... Anyway it not something that needs to be fixed in this CL. I just
wondering how it should work.

https://codereview.chromium.org/102263005/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/speech_synthesis_loader.cc (right):

https://codereview.chromium.org/102263005/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/speech_synthesis_loader.cc:27: { "en-US",
extension_misc::kHighQuality_en_US_ExtensionId }
Optional, it looks like this ID used only here, does it still matter to define
in another file and put into header?

[dmazzoni, 2013-12-05 23:39:04]

https://codereview.chromium.org/102263005/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/speech_synthesis_loader.h (right):

https://codereview.chromium.org/102263005/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/speech_synthesis_loader.h:45: Profile*
profile_;
On 2013/12/05 23:29:19, Dmitry Polukhin wrote:
> On 2013/12/05 23:19:47, Dominic Mazzoni wrote:
> > On 2013/12/05 18:35:32, Dmitry Polukhin wrote:
> > > Need to find out how to make it work with multiprofile. Do we want to
enable
> > > spoken feedback for all profile in session or keep it per profile?
> > 
> > Currently TTS is all done per-profile - even on desktop Chrome, if you
install
> a
> > new TTS voice from the web store in one profile, another profile opened in a
> > different window won't use it. I think it's unlikely this will affect very
> many
> > users negatively in practice, but if we change it, we should fix it
throughout
> > the TTS system.
> > 
> > My understanding is that sometime later we'll have the ability for the same
> > extension installed in another profile to share the same storage - is that
> still
> > the plan? If so, I think that provides a bigger benefit.
> 
> On Chrome OS accessibility UI is available from system tray. So it is kind of
> system wide... Anyway it not something that needs to be fixed in this CL. I
just
> wondering how it should work.

Yes, but speech synthesis is a service available to more than just accessibility
- there are other talking apps that use the same (public) extension API, and in
M33 we now have a public speech synthesis API for the web, so we expect more
talking web sites.

This feature will apply to them too, i.e. to anyone who uses speech - that's why
I set the threshold reasonably high at 100 utterances in a single session.

[not at google - send to devlin, 2013-12-09 21:08:09]

https://codereview.chromium.org/102263005/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/external_provider_impl.cc (right):

https://codereview.chromium.org/102263005/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/external_provider_impl.cc:463: if
(!is_chromeos_demo_session) {
Could this go in ExternalComponentLoader? It seems to be what you're going for
here.

I mean this stuff all seems rather arbitrary (e.g. why chromeos_demo_session for
one) but that's some kind of precedent.

[dmazzoni, 2013-12-10 07:55:52]

https://codereview.chromium.org/102263005/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/external_provider_impl.cc (right):

https://codereview.chromium.org/102263005/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/external_provider_impl.cc:463: if
(!is_chromeos_demo_session) {
On 2013/12/09 21:08:09, kalman wrote:
> Could this go in ExternalComponentLoader? It seems to be what you're going for
> here.

SpeechSynthesisLoader is Chrome OS only. Merging it into ExternalComponentLoader
would just add a bunch of ifdefs.

> I mean this stuff all seems rather arbitrary (e.g. why chromeos_demo_session
for
> one) but that's some kind of precedent.

I was just following the precent. I think it's unlikely this code would be
triggered in a demo session so I could just remove that check. Up to you.

[not at google - send to devlin, 2013-12-10 20:37:47]

+ antony here, hopefully he has more context than me. Why did you choose the
external component for one?

Perhaps come around the corner and chat to us when you're back.

https://codereview.chromium.org/102263005/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/external_provider_impl.cc (right):

https://codereview.chromium.org/102263005/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/external_provider_impl.cc:463: if
(!is_chromeos_demo_session) {
On 2013/12/10 07:55:53, Dominic Mazzoni wrote:
> On 2013/12/09 21:08:09, kalman wrote:
> > Could this go in ExternalComponentLoader? It seems to be what you're going
for
> > here.
> 
> SpeechSynthesisLoader is Chrome OS only. Merging it into
ExternalComponentLoader
> would just add a bunch of ifdefs.

So it's more than just #if defined(OS_CHROMEOS)? If so... ok.

> 
> > I mean this stuff all seems rather arbitrary (e.g. why chromeos_demo_session
> for
> > one) but that's some kind of precedent.
> 
> I was just following the precent. I think it's unlikely this code would be
> triggered in a demo session so I could just remove that check. Up to you.

I don't have any informed opinion i.e. I don't even really know what a "chromeos
demo session" is.

[asargent_no_longer_on_chrome, 2013-12-12 00:23:46]

I still need to talk with saroop@ about the concerns we had with other external
component extensions. If he ends up being ok with it, we may want to move your
code into 

chrome/browser/extensions/external_component_loader.cc

instead of

chrome/browser/extensions/external_provider_impl.cc

Because that's where the other EXTERNAL_COMPONENT extensions get loaded.

[dmazzoni, 2013-12-12 22:53:23]

On 2013/12/12 00:23:46, Antony Sargent wrote:
> I still need to talk with saroop@ about the concerns we had with other
external
> component extensions.

Any update?

> If he ends up being ok with it, we may want to move your
> code into 
> 
> chrome/browser/extensions/external_component_loader.cc

Done - but I don't really like all of the ifdefs. What would you think about a
chromeos_external_component_loader that inherits from it?

Or, if it doesn't bother you, this should be fine. We can refactor later if we
end up adding many more extensions.

[asargent_no_longer_on_chrome, 2013-12-13 00:47:00]

This version LGTM, since it seems to map a little more closely to how things are
currently organized (in general it seems like we have these external providers
organized by their source). 

Re: the ifdefs, instead of subclassing, would composition work better? I.e. just
adding a separate class that we may have an instance of owned by
ExternalComponentLoader?

[dmazzoni, 2013-12-13 00:48:26]

Thanks!

I'll look into pulling a bit of this into a helper class, that might clean it up
a bit.

[dmazzoni, 2013-12-13 18:12:15]

+thestig for chrome/ OWNERS review of two files

[Lei Zhang, 2013-12-14 19:32:31]

On 2013/12/13 18:12:15, Dominic Mazzoni wrote:
> +thestig for chrome/ OWNERS review of two files

Patch set 10 doesn't build with clang and it's not obvious which two files we
are talking about.

[dmazzoni, 2013-12-14 21:59:10]

On 2013/12/14 19:32:31, Lei Zhang wrote:
> On 2013/12/13 18:12:15, Dominic Mazzoni wrote:
> > +thestig for chrome/ OWNERS review of two files
> 
> Patch set 10 doesn't build with clang and it's not obvious which two files we
> are talking about.

Sorry! Wanted your OWNERS review for:

chrome/browser/prefs/browser_prefs.cc
chrome/browser/resources/chromeos/speech_synthesis/manifest.json

I'll fix the clang error shortly.

[dmazzoni, 2013-12-16 20:42:38]

PTAL? Clang error fixed in previous patch set, latest patch set should pass all
tests.

[Lei Zhang, 2013-12-16 21:20:54]

On 2013/12/14 21:59:10, Dominic Mazzoni wrote:
> Sorry! Wanted your OWNERS review for:
> 
> chrome/browser/prefs/browser_prefs.cc
> chrome/browser/resources/chromeos/speech_synthesis/manifest.json

lgtm

[dmazzoni, 2013-12-17 00:21:30]

Committed patchset #12 manually as r241081 (presubmit successful).