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

chrome/browser/search/hotword_service.cc

Index: chrome/browser/search/hotword_service.cc
diff --git a/chrome/browser/search/hotword_service.cc b/chrome/browser/search/hotword_service.cc
index 7e67c647ad8d040c6e2421367063ef9a757136e7..0800f2642e81bbd518a9d3d031ec8a23fad19663 100644
--- a/chrome/browser/search/hotword_service.cc
+++ b/chrome/browser/search/hotword_service.cc
@@ -12,6 +12,9 @@
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/pending_extension_manager.h"
+#include "chrome/browser/extensions/updater/extension_updater.h"
+#include "chrome/browser/extensions/webstore_startup_installer.h"
 #include "chrome/browser/plugins/plugin_prefs.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search/hotword_service_factory.h"
@@ -24,6 +27,7 @@
 #include "content/public/common/webplugininfo.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/common/extension.h"
+#include "extensions/common/one_shot_event.h"
 #include "grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 
@@ -145,6 +149,18 @@ ExtensionService* GetExtensionService(Profile* profile) {
   return NULL;
 }
 
+// static
+std::string GetCurrentLocale(Profile* profile) {
+  std::string locale =
+#if defined(OS_CHROMEOS)
+      // On ChromeOS locale is per-profile.
+      profile->GetPrefs()->GetString(prefs::kApplicationLocale);
+#else
+      g_browser_process->GetApplicationLocale();
+#endif
+  return locale;
+}
+
 }  // namespace
 
 namespace hotword_internal {
@@ -157,14 +173,8 @@ const char kHotwordUnusablePrefName[] = "hotword.search_enabled";
 
 // static
 bool HotwordService::DoesHotwordSupportLanguage(Profile* profile) {
-  std::string locale =
-#if defined(OS_CHROMEOS)
-      // On ChromeOS locale is per-profile.
-      profile->GetPrefs()->GetString(prefs::kApplicationLocale);
-#else
-      g_browser_process->GetApplicationLocale();
-#endif
-  std::string normalized_locale = l10n_util::NormalizeLocale(locale);
+  std::string normalized_locale =
+      l10n_util::NormalizeLocale(GetCurrentLocale(profile));
   StringToLowerASCII(&normalized_locale);
 
   for (size_t i = 0; i < arraysize(kSupportedLocales); i++) {
@@ -176,8 +186,12 @@ bool HotwordService::DoesHotwordSupportLanguage(Profile* profile) {
 
 HotwordService::HotwordService(Profile* profile)
     : profile_(profile),
+      extension_registry_observer_(this),
       client_(NULL),
-      error_message_(0) {
+      error_message_(0),
+      uninstall_pending_(false),
+      weak_factory_(this) {
+  extension_registry_observer_.Add(extensions::ExtensionRegistry::Get(profile));
   // This will be called during profile initialization which is a good time
   // to check the user's hotword state.
   HotwordEnabled enabled_state = UNSET;
@@ -203,12 +217,15 @@ HotwordService::HotwordService(Profile* profile)
                  base::Unretained(this)));
 
   registrar_.Add(this,
-                 chrome::NOTIFICATION_EXTENSION_INSTALLED_DEPRECATED,
-                 content::Source<Profile>(profile_));
-  registrar_.Add(this,
                  chrome::NOTIFICATION_BROWSER_WINDOW_READY,
                  content::NotificationService::AllSources());
 
+  extensions::ExtensionSystem::Get(profile_)->ready().Post(
+      FROM_HERE,
+      base::Bind(base::IgnoreResult(
+          &HotwordService::MaybeUninstallHotwordExtension),
+                 weak_factory_.GetWeakPtr()));
+
   // Clear the old user pref because it became unusable.
   // TODO(rlp): Remove this code per crbug.com/358789.
   if (profile_->GetPrefs()->HasPrefPath(
@@ -223,25 +240,7 @@ HotwordService::~HotwordService() {
 void HotwordService::Observe(int type,
                              const content::NotificationSource& source,
                              const content::NotificationDetails& details) {
-  if (type == chrome::NOTIFICATION_EXTENSION_INSTALLED_DEPRECATED) {
-    const extensions::Extension* extension =
-        content::Details<const extensions::InstalledExtensionInfo>(details)
-              ->extension;
-    // Disabling the extension automatically on install should only occur
-    // if the user is in the field trial for auto-install which is gated
-    // by the IsHotwordAllowed check.
-    if (IsHotwordAllowed() &&
-        extension->id() == extension_misc::kHotwordExtensionId &&
-        !profile_->GetPrefs()->GetBoolean(prefs::kHotwordSearchEnabled)) {
-      DisableHotwordExtension(GetExtensionService(profile_));
-      // Once the extension is disabled, it will not be enabled until the
-      // user opts in at which point the pref registrar will take over
-      // enabling and disabling.
-      registrar_.Remove(this,
-                        chrome::NOTIFICATION_EXTENSION_INSTALLED_DEPRECATED,
-                        content::Source<Profile>(profile_));
-    }
-  } else if (type == chrome::NOTIFICATION_BROWSER_WINDOW_READY) {
+  if (type == chrome::NOTIFICATION_BROWSER_WINDOW_READY) {
     // The microphone monitor must be initialized as the page is loading
     // so that the state of the microphone is available when the page
     // loads. The Ok Google Hotword setting will display an error if there
@@ -257,6 +256,120 @@ void HotwordService::Observe(int type,
   }
 }
 
+void HotwordService::OnExtensionUninstalled(
+    content::BrowserContext* browser_context,
+    const extensions::Extension* extension) {
+  CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  if (extension->id() != extension_misc::kHotwordExtensionId ||
+      profile_ != static_cast<Profile*>(browser_context) ||

[Yoyo Zhou, 2014/06/18 00:33:21]

Profile::FromBrowserContext

[rpetterson, 2014/06/18 02:08:44]

Done.

+      !GetExtensionService(profile_))
+    return;
+
+  // Verify that the extension was uninstalled because of a locale mismatch.
+  // If that's the case, reinstall and reset the saved locale.
+  if (!ShouldUninstallHotwordExtension())

[Yoyo Zhou, 2014/06/18 00:33:21]

Does uninstall_pending_ serve as well here?

[rpetterson, 2014/06/18 02:08:44]

You mean just replace the check ShouldUninstallHotwordExtension() with
uninstall_pending_? Yes, that should work since uninstall_pending_ should be set
if we've decided an uninstall is needed.

I've updated it.

+    return;
+
+  InstallHotwordExtensionFromWebstore();
+  SetPreviousLocalePref();
+}
+
+void HotwordService::InstallHotwordExtensionFromWebstore() {
+  extensions::WebstoreStandaloneInstaller::Callback callback =
+      base::Bind(&HotwordService::OnHotwordExtensionInstalled,
+                 base::Unretained(this));
+  installer_ = new extensions::WebstoreStartupInstaller(

[Yoyo Zhou, 2014/06/18 00:33:21]

I'm not confident that this is the right choice of installer (there are too many
to choose from =( ), but if it installs silently without prompting the user,
then I think that's what you need.

[rpetterson, 2014/06/18 02:08:44]

This is the same one used by ChromeOS to install the hotword extension for the
app_list which is why I used it since it's already been in production in a
similar context. It does silently install without prompting the user from what I
can tell.

+      extension_misc::kHotwordExtensionId,
+      profile_,
+      false,
+      callback);
+  installer_->BeginInstall();
+}
+
+void HotwordService::OnExtensionInstalled(
+    content::BrowserContext* browser_context,
+    const extensions::Extension* extension) {
+
+  if (extension->id() != extension_misc::kHotwordExtensionId ||
+      profile_ != static_cast<Profile*>(browser_context))
+    return;
+
+  // If the previous locale pref has never been set, set it now since
+  // the extension has been installed.
+  if (!profile_->GetPrefs()->HasPrefPath(prefs::kHotwordPreviousLanguage))
+    SetPreviousLocalePref();
+
+  // If MaybeUninstallHotwordExtension already triggered an uninstall, we
+  // don't want to loop and trigger another uninstall-install cycle.
+  // However, if we arrived her via an uninstall-triggered-install (and in
+  // that case |uninstall_pending_| will be true) then we know install
+  // has completed and we can reset |uninstall_pending_|.
+  if (!uninstall_pending_)

[Yoyo Zhou, 2014/06/18 00:33:21]

What do you think about calling this reinstall_pending_ (that would make more
sense in the context here), and MaybeReinstallHotwordExtension?

It also should be possible to put most of this reinstalling logic into a
separate helper class if HotwordService is getting unwieldy. Up to you.

[rpetterson, 2014/06/18 02:08:44]

sgtm. done.
I'd rather do that as a separate CL for now. I've added a tracking bug for it:
crbug.com/385969

+    MaybeUninstallHotwordExtension();
+  else
+    uninstall_pending_ = false;
+
+  // Now that the extension is installed, if the user has not selected
+  // the preference on, make sure it is turned off.
+  //
+  // Disabling the extension automatically on install should only occur
+  // if the user is in the field trial for auto-install which is gated
+  // by the IsHotwordAllowed check. The check for IsHotwordAllowed() here
+  // can be removed once it's known that few people have manually
+  // installed extension.
+  if (IsHotwordAllowed() &&

[Yoyo Zhou, 2014/06/18 00:33:21]

This check/disable looks repeated from the constructor.

[rpetterson, 2014/06/18 02:08:44]

Yes, but we have to repeat it because the HotwordService constructor can finish
before the external component loader finishes installing the extensions. In that
case, we catch it here and disable it. 

+      !profile_->GetPrefs()->GetBoolean(prefs::kHotwordSearchEnabled)) {
+    DisableHotwordExtension(GetExtensionService(profile_));
+  }
+}
+
+bool HotwordService::MaybeUninstallHotwordExtension() {
+  CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  ExtensionService* extension_service = GetExtensionService(profile_);
+  if (!extension_service)
+    return false;
+
+  const extensions::Extension* extension = extension_service->GetExtensionById(
+      extension_misc::kHotwordExtensionId, true);
+  if (!extension)
+    return false;
+
+  // If the extension is currently pending, return and we'll check again
+  // after the install is finished.
+  extensions::PendingExtensionManager* pending_manager =
+      extension_service->pending_extension_manager();
+  if (pending_manager->IsIdPending(extension->id()))
+    return false;
+
+  // If there is already a pending request from HotwordService, don't try
+  // to uninstall either.
+  if (uninstall_pending_)
+    return false;
+
+  // Check if the current locale matches the previous. If they don't match,
+  // uninstall the extension.
+  if (!ShouldUninstallHotwordExtension())
+    return false;
+
+  uninstall_pending_ = true;
+  return UninstallHotwordExtension(extension_service);
+}
+
+bool HotwordService::UninstallHotwordExtension(
+    ExtensionService* extension_service) {
+  base::string16 error;
+  if (!extension_service->UninstallExtension(
+          extension_misc::kHotwordExtensionId, true, &error)) {
+    LOG(WARNING) << "Cannot uninstall extension with id "
+                 << extension_misc::kHotwordExtensionId
+                 << ": " << error;
+    return false;
+  }
+  return true;
+}
+
 bool HotwordService::IsServiceAvailable() {
   error_message_ = 0;
 
@@ -371,3 +484,29 @@ void HotwordService::StopHotwordSession(HotwordClient* client) {
     event_service->OnHotwordSessionStopped();
 #endif
 }
+
+void HotwordService::SetPreviousLocalePref() {
+  profile_->GetPrefs()->SetString(prefs::kHotwordPreviousLanguage,
+                                  GetCurrentLocale(profile_));
+}
+
+bool HotwordService::ShouldUninstallHotwordExtension() {
+  std::string previous_locale =
+      profile_->GetPrefs()->GetString(prefs::kHotwordPreviousLanguage);
+  std::string locale = GetCurrentLocale(profile_);
+
+  // If it's a new locale, then the old extension should be uninstalled.
+  // If there is no previous locale pref, then this is the first install
+  // so no need to uninstall first.
+  return locale != previous_locale &&
+      profile_->GetPrefs()->HasPrefPath(prefs::kHotwordPreviousLanguage) &&

[Yoyo Zhou, 2014/06/18 00:33:21]

nit: seems like the no-previous-locale check should come at the beginning of
this function (though I think it works either way)

[rpetterson, 2014/06/18 02:08:44]

Done.

+      HotwordService::DoesHotwordSupportLanguage(profile_);
+}
+
+void HotwordService::OnHotwordExtensionInstalled(bool success,

[Yoyo Zhou, 2014/06/18 00:33:21]

This callback doesn't really do anything. Is it needed?

[rpetterson, 2014/06/18 02:08:44]

I agree. Removed.

+                                                 const std::string& error) {
+  if (!success)
+    LOG(WARNING) << "Cannot install extension with id "
+                 << extension_misc::kHotwordExtensionId
+                 << ": " << error;
+}