[Hotword] Uninstall and reinstall the extension upon language change.

There are two places we check to see if we need uninstall-install. The first is on startup when the extension system is ready (in response to the ExtensionSystem::ready() OneShot timer). If the extension is currently pending, then we'll check again when we get the notification that the extension has installed when OnExtensionInstalled is called.

The function MaybeUninstallHotwordExtension does a series of checks to determine if we should uninstall the extension based on pending installs and previous vs. current locale. If an uninstall is necessary, it kicks it off. 

Upon Uninstall, OnExtensionUninstalled is triggered. When OnExtensionUninstalled is triggered for the hotword extension, it will reinstall it and update the previous locale (language). If the user goes in to try to uninstall the hotword extension manually, this change will also force it to be reinstalled.

This will trigger a second call to OnExtensionInstalled. To prevent a loop, we use HotwordService::uninstall_pending_ to prevent another uninstall attempt.

This CL also removes the dependency on the deprecated NOTIFICATION_EXTENSION_INSTALLED as a side effect.

Several unittests were added to the unittest suite to test various parts of the uninstall-install pipeline. In order to have access to the extension system during tests, the HotwordServiceTest now extends ExtensionServiceTestBase.

BUG=345806

Review URL: https://codereview.chromium.org/330193005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@278644 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/search/hotword_service.h

@@ -6,15 +6,24 @@
 #define CHROME_BROWSER_SEARCH_HOTWORD_SERVICE_H_
 
 #include "base/basictypes.h"
+#include "base/memory/weak_ptr.h"
 #include "base/prefs/pref_change_registrar.h"
+#include "base/scoped_observer.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/browser/extension_registry_observer.h"
 
 class ExtensionService;
 class HotwordClient;
 class Profile;
 
+namespace extensions {
+class Extension;
+class WebstoreStandaloneInstaller;
+}  // namespace extensions
+
 namespace hotword_internal {
 // Constants for the hotword field trial.
 extern const char kHotwordFieldTrialName[];
@@ -24,6 +33,7 @@ extern const char kHotwordFieldTrialDisabledGroupName[];
 // Provides an interface for the Hotword component that does voice triggered
 // search.
 class HotwordService : public content::NotificationObserver,
+                       public extensions::ExtensionRegistryObserver,
                        public KeyedService {
  public:
   // Returns true if the hotword supports the current system language.
@@ -37,6 +47,14 @@ class HotwordService : public content::NotificationObserver,
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE;
 
+  // Overridden from ExtensionRegisterObserver:
+  virtual void OnExtensionInstalled(
+      content::BrowserContext* browser_context,
+      const extensions::Extension* extension) OVERRIDE;
+  virtual void OnExtensionUninstalled(
+      content::BrowserContext* browser_context,
+      const extensions::Extension* extension) OVERRIDE;
+
   // Checks for whether all the necessary files have downloaded to allow for
   // using the extension.
   virtual bool IsServiceAvailable();
@@ -62,6 +80,23 @@ class HotwordService : public content::NotificationObserver,
   void StopHotwordSession(HotwordClient* client);
   HotwordClient* client() { return client_; }
 
+  // Checks if the current version of the hotword extension should be
+  // uninstalled in order to update to a different language version.
+  // Returns true if the extension was uninstalled.
+  bool MaybeReinstallHotwordExtension();
+
+  // Checks based on locale if the current version should be uninstalled so that
+  // a version with a different language can be installed.
+  bool ShouldReinstallHotwordExtension();
+
+  // Helper functions pulled out for testing purposes.
+  // UninstallHotwordExtension returns true if the extension was uninstalled.
+  virtual bool UninstallHotwordExtension(ExtensionService* extension_service);
+  virtual void InstallHotwordExtensionFromWebstore();
+
+  // Sets the pref value of the previous language.
+  void SetPreviousLanguagePref();
+
   // Returns the current error message id. A value of 0 indicates
   // no error.
   int error_message() { return error_message_; }
@@ -73,8 +108,18 @@ class HotwordService : public content::NotificationObserver,
 
   content::NotificationRegistrar registrar_;
 
+  // For observing the ExtensionRegistry.
+  ScopedObserver<extensions::ExtensionRegistry,
+                 extensions::ExtensionRegistryObserver>
+      extension_registry_observer_;
+
+  scoped_refptr<extensions::WebstoreStandaloneInstaller> installer_;
+
   HotwordClient* client_;
   int error_message_;
+  bool reinstall_pending_;
+
+  base::WeakPtrFactory<HotwordService> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(HotwordService);
 };

chrome/browser/search/hotword_service_factory.cc

@@ -103,6 +103,9 @@ void HotwordServiceFactory::RegisterProfilePrefs(
   prefs->RegisterBooleanPref(prefs::kHotwordAudioLoggingEnabled,
                              true,
                              user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
+  prefs->RegisterStringPref(prefs::kHotwordPreviousLanguage,
+                            std::string(),
+                            user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
 }
 
 KeyedService* HotwordServiceFactory::BuildServiceInstanceFor(

chrome/browser/search/hotword_service_unittest.cc

@@ -2,17 +2,77 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/memory/scoped_ptr.h"
 #include "base/metrics/field_trial.h"
 #include "base/prefs/pref_service.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/extensions/extension_service_test_base.h"
+#include "chrome/browser/extensions/test_extension_service.h"
 #include "chrome/browser/search/hotword_service.h"
 #include "chrome/browser/search/hotword_service_factory.h"
+#include "chrome/common/extensions/extension_constants.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/testing_profile.h"
 #include "content/public/test/test_browser_thread_bundle.h"
+#include "extensions/browser/extension_system.h"
+#include "extensions/common/extension.h"
+#include "extensions/common/extension_builder.h"
+#include "extensions/common/manifest.h"
+#include "extensions/common/one_shot_event.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-class HotwordServiceTest : public testing::Test {
+namespace {
+
+class MockHotwordService : public HotwordService {
+ public:
+  explicit MockHotwordService(Profile* profile)
+      : HotwordService(profile),
+        uninstall_count_(0) {
+  }
+
+  virtual bool UninstallHotwordExtension(
+      ExtensionService* extension_service) OVERRIDE {
+    uninstall_count_++;
+    return HotwordService::UninstallHotwordExtension(extension_service);
+  }
+
+  virtual void InstallHotwordExtensionFromWebstore() OVERRIDE{
+    scoped_ptr<base::DictionaryValue> manifest =
+        extensions::DictionaryBuilder()
+        .Set("name", "Hotword Test Extension")
+        .Set("version", "1.0")
+        .Set("manifest_version", 2)
+        .Build();
+    scoped_refptr<extensions::Extension> extension =
+        extensions::ExtensionBuilder().SetManifest(manifest.Pass())
+        .AddFlags(extensions::Extension::FROM_WEBSTORE
+                  | extensions::Extension::WAS_INSTALLED_BY_DEFAULT)
+        .SetID(extension_misc::kHotwordExtensionId)
+        .SetLocation(extensions::Manifest::EXTERNAL_COMPONENT)
+        .Build();
+    ASSERT_TRUE(extension.get());
+    service_->OnExtensionInstalled(extension, syncer::StringOrdinal());
+  }
+
+
+  int uninstall_count() { return uninstall_count_; }
+
+  void SetExtensionService(ExtensionService* service) { service_ = service; }
+
+  ExtensionService* extension_service() { return service_; }
+
+ private:
+  ExtensionService* service_;
+  int uninstall_count_;
+};
+
+KeyedService* BuildMockHotwordService(content::BrowserContext* context) {
+  return new MockHotwordService(static_cast<Profile*>(context));
+}
+
+}  // namespace
+
+class HotwordServiceTest : public extensions::ExtensionServiceTestBase {
  protected:
   HotwordServiceTest() : field_trial_list_(NULL) {}
   virtual ~HotwordServiceTest() {}
@@ -28,7 +88,6 @@ class HotwordServiceTest : public testing::Test {
 
  private:
   base::FieldTrialList field_trial_list_;
-  content::TestBrowserThreadBundle thread_bundle_;
 };
 
 TEST_F(HotwordServiceTest, IsHotwordAllowedBadFieldTrial) {
@@ -126,3 +185,129 @@ TEST_F(HotwordServiceTest, AudioLoggingPrefSetCorrectly) {
   // it should return false if the preference has never been set.
   EXPECT_FALSE(hotword_service->IsOptedIntoAudioLogging());
 }
+
+TEST_F(HotwordServiceTest, ShouldReinstallExtension) {
+  // Set the field trial to a valid one.
+  ASSERT_TRUE(base::FieldTrialList::CreateFieldTrial(
+      hotword_internal::kHotwordFieldTrialName, "Install"));
+
+  InitializeEmptyExtensionService();
+
+  HotwordServiceFactory* hotword_service_factory =
+      HotwordServiceFactory::GetInstance();
+
+  MockHotwordService* hotword_service = static_cast<MockHotwordService*>(
+      hotword_service_factory->SetTestingFactoryAndUse(
+          profile(), BuildMockHotwordService));
+  EXPECT_TRUE(hotword_service != NULL);
+
+  // If no locale has been set, no reason to uninstall.
+  EXPECT_FALSE(hotword_service->ShouldReinstallHotwordExtension());
+
+  SetApplicationLocale(profile(), "en");
+  hotword_service->SetPreviousLanguagePref();
+
+  // Now a locale is set, but it hasn't changed.
+  EXPECT_FALSE(hotword_service->ShouldReinstallHotwordExtension());
+
+  SetApplicationLocale(profile(), "fr_fr");
+
+  // Now it's a different locale so it should uninstall.
+  EXPECT_TRUE(hotword_service->ShouldReinstallHotwordExtension());
+}
+
+TEST_F(HotwordServiceTest, PreviousLanguageSetOnInstall) {
+  // Set the field trial to a valid one.
+  ASSERT_TRUE(base::FieldTrialList::CreateFieldTrial(
+      hotword_internal::kHotwordFieldTrialName, "Install"));
+
+  InitializeEmptyExtensionService();
+  service_->Init();
+
+  HotwordServiceFactory* hotword_service_factory =
+      HotwordServiceFactory::GetInstance();
+
+  MockHotwordService* hotword_service = static_cast<MockHotwordService*>(
+      hotword_service_factory->SetTestingFactoryAndUse(
+          profile(), BuildMockHotwordService));
+  EXPECT_TRUE(hotword_service != NULL);
+  hotword_service->SetExtensionService(service());
+
+  // If no locale has been set, no reason to uninstall.
+  EXPECT_FALSE(hotword_service->ShouldReinstallHotwordExtension());
+
+  SetApplicationLocale(profile(), "test_locale");
+
+  hotword_service->InstallHotwordExtensionFromWebstore();
+  base::MessageLoop::current()->RunUntilIdle();
+
+  EXPECT_EQ("test_locale",
+            profile()->GetPrefs()->GetString(prefs::kHotwordPreviousLanguage));
+}
+
+TEST_F(HotwordServiceTest, UninstallReinstallTriggeredCorrectly) {
+  // Set the field trial to a valid one.
+  ASSERT_TRUE(base::FieldTrialList::CreateFieldTrial(
+      hotword_internal::kHotwordFieldTrialName, "Install"));
+
+  InitializeEmptyExtensionService();
+  service_->Init();
+
+  HotwordServiceFactory* hotword_service_factory =
+      HotwordServiceFactory::GetInstance();
+
+  MockHotwordService* hotword_service = static_cast<MockHotwordService*>(
+      hotword_service_factory->SetTestingFactoryAndUse(
+          profile(), BuildMockHotwordService));
+  EXPECT_TRUE(hotword_service != NULL);
+  hotword_service->SetExtensionService(service());
+
+  // Initialize the locale to "en".
+  SetApplicationLocale(profile(), "en");
+
+  // The previous locale should not be set. No reason to uninstall.
+  EXPECT_FALSE(hotword_service->MaybeReinstallHotwordExtension());
+
+   // Do an initial installation.
+  hotword_service->InstallHotwordExtensionFromWebstore();
+  base::MessageLoop::current()->RunUntilIdle();
+  EXPECT_EQ("en",
+            profile()->GetPrefs()->GetString(prefs::kHotwordPreviousLanguage));
+
+  // Verify the extension is installed but disabled.
+  EXPECT_EQ(1U, registry()->disabled_extensions().size());
+  EXPECT_TRUE(registry()->disabled_extensions().Contains(
+      extension_misc::kHotwordExtensionId));
+
+   // The previous locale should be set but should match the current
+  // locale. No reason to uninstall.
+  EXPECT_FALSE(hotword_service->MaybeReinstallHotwordExtension());
+
+  // Switch the locale to a valid but different one.
+  SetApplicationLocale(profile(), "fr_fr");
+  EXPECT_TRUE(HotwordServiceFactory::IsHotwordAllowed(profile()));
+
+   // Different but valid locale so expect uninstall.
+  EXPECT_TRUE(hotword_service->MaybeReinstallHotwordExtension());
+  EXPECT_EQ(1, hotword_service->uninstall_count());
+  EXPECT_EQ("fr_fr",
+            profile()->GetPrefs()->GetString(prefs::kHotwordPreviousLanguage));
+
+  // Verify the extension is installed. It's still disabled.
+  EXPECT_TRUE(registry()->disabled_extensions().Contains(
+      extension_misc::kHotwordExtensionId));
+
+  // Switch the locale to an invalid one.
+  SetApplicationLocale(profile(), "invalid");
+  EXPECT_FALSE(HotwordServiceFactory::IsHotwordAllowed(profile()));
+  EXPECT_FALSE(hotword_service->MaybeReinstallHotwordExtension());
+  EXPECT_EQ("fr_fr",
+            profile()->GetPrefs()->GetString(prefs::kHotwordPreviousLanguage));
+
+  // If the locale is set back to the last valid one, then an uninstall-install
+  // shouldn't be needed.
+  SetApplicationLocale(profile(), "fr_fr");
+  EXPECT_TRUE(HotwordServiceFactory::IsHotwordAllowed(profile()));
+  EXPECT_FALSE(hotword_service->MaybeReinstallHotwordExtension());
+  EXPECT_EQ(1, hotword_service->uninstall_count());  // no change
+}

chrome/common/pref_names.cc

@@ -1866,6 +1866,11 @@ const char kHotwordSearchEnabled[] = "hotword.search_enabled_2";
 // seconds of audio data before is sent back to improve voice search.
 const char kHotwordAudioLoggingEnabled[] = "hotword.audio_logging_enabled";
 
+// A string holding the locale information under which Hotword was installed.
+// It is used for comparison since the hotword voice search trigger must be
+// reinstalled to handle a new language.
+const char kHotwordPreviousLanguage[] = "hotword.previous_language";
+
 #if defined(OS_ANDROID)
 // Boolean that controls the global enabled-state of protected media identifier.
 const char kProtectedMediaIdentifierEnabled[] =

chrome/common/pref_names.h

@@ -658,6 +658,7 @@ extern const char kVideoCaptureAllowedUrls[];
 
 extern const char kHotwordSearchEnabled[];
 extern const char kHotwordAudioLoggingEnabled[];
+extern const char kHotwordPreviousLanguage[];
 
 #if defined(OS_ANDROID)
 extern const char kProtectedMediaIdentifierEnabled[];