[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..bbe0e55fb3fe5cb9aba290721f0f216a9b7ed8a0 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

[Jered, 2014/06/18 15:04:19]

delete this comment

[rpetterson, 2014/06/18 20:48:33]

Done.

+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),
+      reinstall_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::MaybeReinstallHotwordExtension),
+                 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,117 @@ 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_ != Profile::FromBrowserContext(browser_context) ||
+      !GetExtensionService(profile_))
+    return;
+
+  // If the extension wasn't uninstalled due to language change, don't try to
+  // reinstall it.
+  if (!reinstall_pending_)
+    return;
+
+  InstallHotwordExtensionFromWebstore();
+  SetPreviousLocalePref();
+}
+
+void HotwordService::InstallHotwordExtensionFromWebstore() {
+  installer_ = new extensions::WebstoreStartupInstaller(
+      extension_misc::kHotwordExtensionId,
+      profile_,
+      false,
+      extensions::WebstoreStandaloneInstaller::Callback());
+  installer_->BeginInstall();
+}
+
+void HotwordService::OnExtensionInstalled(
+    content::BrowserContext* browser_context,
+    const extensions::Extension* extension) {
+
+  if (extension->id() != extension_misc::kHotwordExtensionId ||
+      profile_ != Profile::FromBrowserContext(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 MaybeReinstallHotwordExtension 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

[Jered, 2014/06/18 15:04:19]

her -> here

[rpetterson, 2014/06/18 20:48:33]

Done.

+  // that case |reinstall_pending_| will be true) then we know install
+  // has completed and we can reset |reinstall_pending_|.
+  if (!reinstall_pending_)
+    MaybeReinstallHotwordExtension();

[Jered, 2014/06/18 15:04:19]

Huh? Why would we ever want to reinstall the hotword extension just after it got
installed?

[Yoyo Zhou, 2014/06/18 19:10:31]

I believe this is for the scenario where the locale changes and the extension
updates at the same time. We hit the IsIdPending at startup, so we wait for the
update to finish. When it has done so, we get here.

[rpetterson, 2014/06/18 20:48:33]

That's correct. Typically, no, we would not. If we've just installed it, chances
are good that we installed it in the correct language. However, this callback is
also triggered when the extension has been installed from an update. So here's a
hypothetical situation:

- Select new language
- Restart Chrome to let new language take effect
- Upon Restart, external component loader decides it's time to update the
extensions
- Hotword has an update so we download the new one
- Meanwhile, the extension system says it's ready so we do a check
- Our correct language check tells us that the hotword extension is pending
(because it's downloading) so we'll wait until the OnExtensionInstalled trigger
- OnExtensionInstalled triggers and we realize that the updated extension is the
wrong language
- Proceed to uninstall and reinstall in the correct language

Would it be simpler to do this check before the external component loader does
it's updates so that we don't download the extension twice sometimes? Yes.
However, that requires more complicated hooks into the extension system.
Eventually the extension folks plan to build in forced updates which will serve
our purpose as well, but until then this is less invasive. We're covering edge
cases with this check that should rarely happen so extra downloads in general
shouldn't be an issue.

[Jered, 2014/06/18 21:06:04]

Ok, thanks for explaining.

+  else
+    reinstall_pending_ = false;

[Jered, 2014/06/18 15:04:19]

Just unconditionally set this to false.

[Yoyo Zhou, 2014/06/18 19:10:31]

I don't think that works with the previous.

[rpetterson, 2014/06/18 20:48:33]

I thought about this, but it needs to be in an else. Otherwise it might be set
to false before the OnExtensionUninstalled triggers which puts it to false
inside that call and that would be incorrect (I've seen this in manual testing).
Only after we know the reinstall has completed should it be set back to false.

I'm also not sure it's "better" since without the else we go through all the
ShouldReinstallHotwordExtension checks after the extension is reinstalled rather
than just checking the one boolean.

[Jered, 2014/06/18 21:06:04]

sgtm.

+
+  // 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() &&
+      !profile_->GetPrefs()->GetBoolean(prefs::kHotwordSearchEnabled)) {
+    DisableHotwordExtension(GetExtensionService(profile_));
+  }
+}
+
+bool HotwordService::MaybeReinstallHotwordExtension() {
+  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 (reinstall_pending_)
+    return false;
+
+  // Check if the current locale matches the previous. If they don't match,
+  // uninstall the extension.
+  if (!ShouldReinstallHotwordExtension())
+    return false;
+
+  reinstall_pending_ = true;

[Jered, 2014/06/18 15:04:19]

Should this be set to the return value of UninstallHotwordExtension, and then be
returned?

[Yoyo Zhou, 2014/06/18 19:10:31]

That seems right.

[rpetterson, 2014/06/18 20:48:33]

Sorta. I still need to set reinstall_pending_ to true before the call to
UninstallHotwordExtension. Otherwise it's still false when we get into
OnExtensionUninstalled and we never reinstall since it might be the result of a
different kind of uninstall. But I can set the ultimate value of
reinstall_pending_ to be the return value of UninstallHotwordExtension and
return that.

+  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 +481,23 @@ void HotwordService::StopHotwordSession(HotwordClient* client) {
     event_service->OnHotwordSessionStopped();
 #endif
 }
+
+void HotwordService::SetPreviousLocalePref() {

[Jered, 2014/06/18 15:04:19]

nit: The pref is called previous language, so should this be previous language
rather than previous locale?

[rpetterson, 2014/06/18 20:48:33]

Done.

+  profile_->GetPrefs()->SetString(prefs::kHotwordPreviousLanguage,
+                                  GetCurrentLocale(profile_));
+}
+
+bool HotwordService::ShouldReinstallHotwordExtension() {
+  // If there is no previous locale pref, then this is the first install
+  // so no need to uninstall first.
+  if (!profile_->GetPrefs()->HasPrefPath(prefs::kHotwordPreviousLanguage))
+    return false;
+
+  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.
+  return locale != previous_locale &&
+      HotwordService::DoesHotwordSupportLanguage(profile_);
+}