Store TTS engine extension voices in extension prefs.

A TTS engine extension can advertise the set of voices it provides
either using the extension manifest, or by calling
chrome.ttsEngine.updateVoices to provide a dynamic list.

Previously we cached the set of dynamic voices provided by updateVoices
in a map in TtsEngineExtensionObserver. That works fine if a TTS extension
has a chance to run.

However, on profile load, installed extensions don't typically run. That means
that when a web page queries the set of installed voices, it gets the
list of voices from the manifest, not the dynamic list of voices. This is
only fixed once the extension spins up, typically when speech starts.

Instead, when an extension calls chrome.ttsEngine.updateVoices, cache
the voices in the extension preferences, so that when a profile loads it
always gets the most up-to-date list of voices.

Partially covered by the existing TtsApiTest.UpdateVoicesApi test,
and adds a new 2-part test to cover the caching in particular.

(This supercedes: http://crrev.com/c/1247023)

Bug: 889591
Change-Id: I79d8e576114951869af66e682920672924f247a7
Reviewed-on: https://chromium-review.googlesource.com/c/1263035
Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Katie Dektar <katie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601848}

chrome/browser/speech/extension_api/tts_engine_extension_api.cc

@@ -50,6 +50,10 @@ const char kOnResume[] = "ttsEngine.onResume";
 
 namespace {
 
+// An extension preference to keep track of the TTS voices that a
+// TTS engine extension makes available.
+const char kPrefTtsVoices[] = "tts_voices";
+
 void WarnIfMissingPauseOrResumeListener(Profile* profile,
                                         EventRouter* event_router,
                                         std::string extension_id) {
@@ -68,14 +72,87 @@ void WarnIfMissingPauseOrResumeListener(Profile* profile,
       constants::kErrorMissingPauseOrResume);
 }
 
-const std::vector<extensions::TtsVoice>* GetVoicesInternal(
+std::unique_ptr<std::vector<extensions::TtsVoice>>
+ValidateAndConvertToTtsVoiceVector(const extensions::Extension* extension,
+                                   const base::ListValue& voices_data,
+                                   bool return_after_first_error,
+                                   const char** error) {
+  auto tts_voices = std::make_unique<std::vector<extensions::TtsVoice>>();
+  for (size_t i = 0; i < voices_data.GetSize(); i++) {
+    extensions::TtsVoice voice;
+    const base::DictionaryValue* voice_data = nullptr;
+    voices_data.GetDictionary(i, &voice_data);
+
+    // Note partial validation of these attributes occurs based on tts engine's
+    // json schema (e.g. for data type matching). The missing checks follow
+    // similar checks in manifest parsing.
+    if (voice_data->HasKey(constants::kVoiceNameKey))
+      voice_data->GetString(constants::kVoiceNameKey, &voice.voice_name);
+    if (voice_data->HasKey(constants::kLangKey)) {
+      voice_data->GetString(constants::kLangKey, &voice.lang);
+      if (!l10n_util::IsValidLocaleSyntax(voice.lang)) {
+        *error = constants::kErrorInvalidLang;
+        if (return_after_first_error) {
+          tts_voices->clear();
+          return tts_voices;
+        }
+        continue;
+      }
+    }
+    if (voice_data->HasKey(constants::kRemoteKey))
+      voice_data->GetBoolean(constants::kRemoteKey, &voice.remote);
+    if (voice_data->HasKey(constants::kExtensionIdKey)) {
+      // Allow this for clients who might have used |chrome.tts.getVoices| to
+      // update existing voices. However, trying to update the voice of another
+      // extension should trigger an error.
+      std::string extension_id;
+      voice_data->GetString(constants::kExtensionIdKey, &extension_id);
+      if (extension->id() != extension_id) {
+        *error = constants::kErrorExtensionIdMismatch;
+        if (return_after_first_error) {
+          tts_voices->clear();
+          return tts_voices;
+        }
+        continue;
+      }
+    }
+    const base::ListValue* event_types = nullptr;
+    if (voice_data->HasKey(constants::kEventTypesKey))
+      voice_data->GetList(constants::kEventTypesKey, &event_types);
+
+    if (event_types) {
+      for (size_t j = 0; j < event_types->GetSize(); j++) {
+        std::string event_type;
+        event_types->GetString(j, &event_type);
+        voice.event_types.insert(event_type);
+      }
+    }
+
+    tts_voices->push_back(voice);
+  }
+  return tts_voices;
+}
+
+// Get the voices for an extension, checking the preferences first
+// (in case the extension has ever called UpdateVoices in the past),
+// and the manifest second.
+std::unique_ptr<std::vector<extensions::TtsVoice>> GetVoicesInternal(
     content::BrowserContext* context,
     const extensions::Extension* extension) {
-  Profile* profile = Profile::FromBrowserContext(context);
-  const std::vector<extensions::TtsVoice>* voices =
-      TtsEngineExtensionObserver::GetInstance(profile)->GetRuntimeVoices(
-          extension->id());
-  return voices ? voices : extensions::TtsVoices::GetTtsVoices(extension);
+  // First try to get the saved set of voices from extension prefs.
+  auto* extension_prefs = extensions::ExtensionPrefs::Get(context);
+  const base::ListValue* voices_data = nullptr;
+  if (extension_prefs->ReadPrefAsList(extension->id(), kPrefTtsVoices,
+                                      &voices_data)) {
+    const char* error = nullptr;
+    return ValidateAndConvertToTtsVoiceVector(
+        extension, *voices_data,
+        /* return_after_first_error = */ false, &error);
+  }
+
+  // Fall back on the extension manifest.
+  auto* manifest_voices = extensions::TtsVoices::GetTtsVoices(extension);
+  return std::make_unique<std::vector<extensions::TtsVoice>>(*manifest_voices);
 }
 
 }  // namespace
@@ -106,8 +183,7 @@ void TtsExtensionEngine::GetVoices(content::BrowserContext* browser_context,
       continue;
     }
 
-    const std::vector<extensions::TtsVoice>* tts_voices =
-        GetVoicesInternal(profile, extension);
+    auto tts_voices = GetVoicesInternal(profile, extension);
     if (!tts_voices)
       continue;
 
@@ -266,60 +342,24 @@ ExtensionFunction::ResponseAction
 ExtensionTtsEngineUpdateVoicesFunction::Run() {
   base::ListValue* voices_data = nullptr;
   EXTENSION_FUNCTION_VALIDATE(args_->GetList(0, &voices_data));
-  auto tts_voices = std::make_unique<extensions::TtsVoices>();
-  const char* error = nullptr;
-  for (size_t i = 0; i < voices_data->GetSize(); i++) {
-    extensions::TtsVoice voice;
-    base::DictionaryValue* voice_data = nullptr;
-    voices_data->GetDictionary(i, &voice_data);
-
-    // Note partial validation of these attributes occurs based on tts engine's
-    // json schema (e.g. for data type matching). The missing checks follow
-    // similar checks in manifest parsing.
-    if (voice_data->HasKey(constants::kVoiceNameKey))
-      voice_data->GetString(constants::kVoiceNameKey, &voice.voice_name);
-    if (voice_data->HasKey(constants::kLangKey)) {
-      voice_data->GetString(constants::kLangKey, &voice.lang);
-      if (!l10n_util::IsValidLocaleSyntax(voice.lang)) {
-        error = constants::kErrorInvalidLang;
-        continue;
-      }
-    }
-    if (voice_data->HasKey(constants::kRemoteKey))
-      voice_data->GetBoolean(constants::kRemoteKey, &voice.remote);
-    if (voice_data->HasKey(constants::kExtensionIdKey)) {
-      // Allow this for clients who might have used |chrome.tts.getVoices| to
-      // update existing voices. However, trying to update the voice of another
-      // extension should trigger an error.
-      std::string extension_id;
-      voice_data->GetString(constants::kExtensionIdKey, &extension_id);
-      if (extension()->id() != extension_id) {
-        error = constants::kErrorExtensionIdMismatch;
-        continue;
-      }
-    }
-    base::ListValue* event_types = nullptr;
-    if (voice_data->HasKey(constants::kEventTypesKey))
-      voice_data->GetList(constants::kEventTypesKey, &event_types);
-
-    if (event_types) {
-      for (size_t j = 0; j < event_types->GetSize(); j++) {
-        std::string event_type;
-        event_types->GetString(j, &event_type);
-        voice.event_types.insert(event_type);
-      }
-    }
-
-    tts_voices->voices.push_back(voice);
-  }
-
-  Profile* profile = Profile::FromBrowserContext(browser_context());
-  TtsEngineExtensionObserver::GetInstance(profile)->SetRuntimeVoices(
-      std::move(tts_voices), extension()->id());
 
+  // Validate the voices and return an error if there's a problem.
+  const char* error = nullptr;
+  auto tts_voices = ValidateAndConvertToTtsVoiceVector(
+      extension(), *voices_data,
+      /* return_after_first_error = */ true, &error);
   if (error)
     return RespondNow(Error(error));
 
+  // Save these voices to the extension's prefs if they validated.
+  auto* extension_prefs = extensions::ExtensionPrefs::Get(browser_context());
+  extension_prefs->UpdateExtensionPref(
+      extension()->id(), kPrefTtsVoices,
+      std::make_unique<base::Value>(voices_data->Clone()));
+
+  // Notify that voices have changed.
+  TtsController::GetInstance()->VoicesChanged();
+
   return RespondNow(NoArguments());
 }
 
@@ -344,8 +384,7 @@ ExtensionTtsEngineSendTtsEventFunction::Run() {
   // Make sure the extension has included this event type in its manifest.
   bool event_type_allowed = false;
   Profile* profile = Profile::FromBrowserContext(browser_context());
-  const std::vector<extensions::TtsVoice>* tts_voices =
-      GetVoicesInternal(profile, extension());
+  auto tts_voices = GetVoicesInternal(profile, extension());
   if (!tts_voices)
     return RespondNow(Error(constants::kErrorUndeclaredEventType));
 

chrome/browser/speech/extension_api/tts_engine_extension_observer.cc

@@ -93,22 +93,6 @@ const std::set<std::string> TtsEngineExtensionObserver::GetTtsExtensions() {
   return engine_extension_ids_;
 }
 
-const std::vector<extensions::TtsVoice>*
-TtsEngineExtensionObserver::GetRuntimeVoices(const std::string extension_id) {
-  auto it = extension_id_to_runtime_voices_.find(extension_id);
-  if (it == extension_id_to_runtime_voices_.end())
-    return nullptr;
-
-  return &it->second->voices;
-}
-
-void TtsEngineExtensionObserver::SetRuntimeVoices(
-    std::unique_ptr<extensions::TtsVoices> tts_voices,
-    const std::string extension_id) {
-  extension_id_to_runtime_voices_[extension_id] = std::move(tts_voices);
-  TtsController::GetInstance()->VoicesChanged();
-}
-
 void TtsEngineExtensionObserver::Shutdown() {
   extensions::EventRouter::Get(profile_)->UnregisterObserver(this);
 }
@@ -143,7 +127,6 @@ void TtsEngineExtensionObserver::OnExtensionUnloaded(
     extensions::UnloadedExtensionReason reason) {
   size_t erase_count = 0;
   erase_count += engine_extension_ids_.erase(extension->id());
-  erase_count += extension_id_to_runtime_voices_.erase(extension->id());
   if (erase_count > 0)
     TtsController::GetInstance()->VoicesChanged();
 }

chrome/browser/speech/extension_api/tts_engine_extension_observer.h

@@ -14,11 +14,6 @@
 
 class Profile;
 
-namespace extensions {
-struct TtsVoice;
-struct TtsVoices;
-};  // namespace extensions
-
 // Profile-keyed class that observes the extension registry to determine load of
 // extension-based tts engines.
 class TtsEngineExtensionObserver
@@ -35,15 +30,6 @@ class TtsEngineExtensionObserver
   // Gets the currently loaded TTS extension ids.
   const std::set<std::string> GetTtsExtensions();
 
-  // Gets voices for |extension_id| updated through TtsEngine.updateVoices.
-  const std::vector<extensions::TtsVoice>* GetRuntimeVoices(
-      const std::string extension_id);
-
-  // Called to update the voices list for the given extension. This overrides
-  // voices declared in the extension manifest.
-  void SetRuntimeVoices(std::unique_ptr<extensions::TtsVoices> tts_voices,
-                        const std::string extension_id);
-
   // Implementation of KeyedService.
   void Shutdown() override;
 
@@ -69,9 +55,6 @@ class TtsEngineExtensionObserver
 
   std::set<std::string> engine_extension_ids_;
 
-  std::map<std::string, std::unique_ptr<extensions::TtsVoices>>
-      extension_id_to_runtime_voices_;
-
   friend class TtsEngineExtensionObserverFactory;
 
   DISALLOW_COPY_AND_ASSIGN(TtsEngineExtensionObserver);

chrome/browser/speech/extension_api/tts_extension_apitest.cc

@@ -14,12 +14,14 @@
 #include "chrome/browser/extensions/component_loader.h"
 #include "chrome/browser/extensions/extension_apitest.h"
 #include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/speech/extension_api/tts_engine_extension_api.h"
 #include "chrome/browser/speech/extension_api/tts_extension_api.h"
 #include "chrome/browser/speech/tts_controller.h"
 #include "chrome/browser/speech/tts_platform.h"
 #include "chrome/common/chrome_switches.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/test/test_utils.h"
+#include "extensions/browser/event_router.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/notification_types.h"
 #include "net/base/network_change_notifier.h"
@@ -178,6 +180,32 @@ class FakeNetworkOnlineStateForTest : public net::NetworkChangeNotifier {
   DISALLOW_COPY_AND_ASSIGN(FakeNetworkOnlineStateForTest);
 };
 
+class EventRouterAddListenerWaiter : public EventRouter::Observer {
+ public:
+  EventRouterAddListenerWaiter(Profile* profile, const std::string& event_name)
+      : event_router_(EventRouter::EventRouter::Get(profile)) {
+    DCHECK(profile);
+    event_router_->RegisterObserver(this, event_name);
+  }
+
+  ~EventRouterAddListenerWaiter() override {
+    event_router_->UnregisterObserver(this);
+  }
+
+  void Wait() { loop_runner_.Run(); }
+
+  // EventRouter::Observer overrides.
+  void OnListenerAdded(const EventListenerInfo& details) override {
+    loop_runner_.Quit();
+  }
+
+ private:
+  EventRouter* const event_router_;
+  base::RunLoop loop_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(EventRouterAddListenerWaiter);
+};
+
 class TtsApiTest : public ExtensionApiTest {
  public:
   void SetUpInProcessBrowserTestFixture() override {
@@ -198,6 +226,17 @@ class TtsApiTest : public ExtensionApiTest {
   }
 
  protected:
+  bool HasVoiceWithName(const std::string& name) {
+    std::vector<VoiceData> voices;
+    TtsController::GetInstance()->GetVoices(profile(), &voices);
+    for (auto& voice : voices) {
+      if (voice.name == name)
+        return true;
+    }
+
+    return false;
+  }
+
   StrictMock<MockTtsPlatformImpl> mock_platform_impl_;
 };
 
@@ -471,4 +510,26 @@ IN_PROC_BROWSER_TEST_F(TtsApiTest, UpdateVoicesApi) {
   ASSERT_TRUE(RunExtensionTest("tts_engine/update_voices_api")) << message_;
 }
 
+IN_PROC_BROWSER_TEST_F(TtsApiTest, PRE_VoicesAreCached) {
+  EXPECT_FALSE(HasVoiceWithName("Dynamic Voice 1"));
+  EXPECT_FALSE(HasVoiceWithName("Dynamic Voice 2"));
+  ASSERT_TRUE(RunExtensionTest("tts_engine/call_update_voices")) << message_;
+  EXPECT_TRUE(HasVoiceWithName("Dynamic Voice 1"));
+  EXPECT_TRUE(HasVoiceWithName("Dynamic Voice 2"));
+}
+
+IN_PROC_BROWSER_TEST_F(TtsApiTest, VoicesAreCached) {
+  // Make sure the dynamically loaded voices are available even though
+  // the extension didn't "run". Note that the voices might not be available
+  // immediately when the test runs, but the test should pass shortly after
+  // the extension's event listeners are registered.
+  while (!HasVoiceWithName("Dynamic Voice 1") ||
+         !HasVoiceWithName("Dynamic Voice 2")) {
+    // Wait for the extension's event listener to be registered before
+    // checking what voices are registered.
+    EventRouterAddListenerWaiter waiter(profile(), tts_engine_events::kOnStop);
+    waiter.Wait();
+  }
+}
+
 }  // namespace extensions

chrome/test/data/extensions/api_test/tts_engine/call_update_voices/manifest.json

+{
+  "name": "chrome.ttsEngine",
+  "version": "0.1",
+  "manifest_version": 2,
+  "description": "browser test for chrome.ttsEngine.updateVoices API",
+  "background": {
+    "scripts": ["test.js"],
+    "persistent": false
+  },
+  "tts_engine": {
+    "voices": []
+  },
+  "permissions": ["tts", "ttsEngine"]
+}

chrome/test/data/extensions/api_test/tts_engine/call_update_voices/test.js

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+function setup() {
+  var speakListener = function(utterance, options, sendTtsEvent) {};
+  var stopListener = function() {};
+  chrome.ttsEngine.onSpeak.addListener(speakListener);
+  chrome.ttsEngine.onStop.addListener(stopListener);
+}
+
+chrome.test.runTests([
+  function testSetUpDynamicVoices() {
+    var testVoiceData = [
+      {
+        eventTypes: ['start'],
+        lang: 'zh-TW',
+        remote: false,
+        voiceName: 'Dynamic Voice 1'
+      },
+      {
+        eventTypes: ['end', 'interrupted', 'cancelled'],
+        lang: 'en-GB',
+        remote: false,
+        voiceName: 'Dynamic Voice 2'
+      }
+    ];
+    setup();
+    chrome.ttsEngine.updateVoices(testVoiceData);
+    chrome.test.succeed();
+  }
+]);

chrome/test/data/extensions/api_test/tts_engine/update_voices_api/test.js

@@ -54,6 +54,8 @@ chrome.test.runTests([
   },
   function testExtensionIdMismatch() {
     setup();
+    chrome.ttsEngine.updateVoices([]);
+
     chrome.ttsEngine.updateVoices([{
       eventTypes: [ 'end', 'interrupted', 'cancelled', 'error'],
       extensionId: 'interloper',