Allow autoupdate to update disabled extensions.

chrome/browser/extensions/extension_management_browsertest.cc

Index: chrome/browser/extensions/extension_management_browsertest.cc
diff --git a/chrome/browser/extensions/extension_management_browsertest.cc b/chrome/browser/extensions/extension_management_browsertest.cc
index 6d532c1e437e7befe7d079d3ae2684e5a008cb5c..835f85499db6f0fe19f4d0101473606b8dadbc5d 100644
--- a/chrome/browser/extensions/extension_management_browsertest.cc
+++ b/chrome/browser/extensions/extension_management_browsertest.cc
@@ -364,6 +364,60 @@ IN_PROC_BROWSER_TEST_F(ExtensionManagementTest, AutoUpdate) {
   ASSERT_EQ("2.0", extension->VersionString());
 }
 
+// Tests extension autoupdate.
+IN_PROC_BROWSER_TEST_F(ExtensionManagementTest, AutoUpdateDisabledExtensions) {

[Matt Tytel, 2012/03/20 20:36:37]

There is some overlap with the above extension, but after trying some different
things it didn't help much and made the tests less readable.

+  NotificationListener notification_listener;
+  FilePath basedir = test_data_dir_.AppendASCII("autoupdate");
+  // Note: This interceptor gets requests on the IO thread.
+  scoped_refptr<AutoUpdateInterceptor> interceptor(new AutoUpdateInterceptor());
+  content::URLFetcher::SetEnableInterceptionForTests(true);
+
+  interceptor->SetResponseOnIOThread("http://localhost/autoupdate/manifest",
+                                     basedir.AppendASCII("manifest_v2.xml"));
+  interceptor->SetResponseOnIOThread("http://localhost/autoupdate/v2.crx",
+                                     basedir.AppendASCII("v2.crx"));
+
+  // Install version 1 of the extension.
+  ExtensionTestMessageListener listener1("v1 installed", false);
+  ExtensionService* service = browser()->profile()->GetExtensionService();
+  const size_t enabled_size_before = service->extensions()->size();
+  const size_t disabled_size_before = service->disabled_extensions()->size();
+  const Extension* extension =
+      InstallExtension(basedir.AppendASCII("v1.crx"), 1);
+  ASSERT_TRUE(extension);
+  listener1.WaitUntilSatisfied();
+  service->DisableExtension(extension->id());
+  ASSERT_EQ(disabled_size_before + 1, service->disabled_extensions()->size());
+  ASSERT_EQ(enabled_size_before, service->extensions()->size());

[asargent_no_longer_on_chrome, 2012/03/21 18:47:31]

shouldn't this be "enabled_size_before - 1", since you just called
DisableExtension to disable one extension?

[Matt Tytel, 2012/03/26 05:23:59]

I create it, then disable it. So the number of enabled extensions should stay
the same.

+  ASSERT_EQ("ogjcoiohnmldgjemafoockdghcjciccf", extension->id());
+  ASSERT_EQ("1.0", extension->VersionString());
+
+  // We don't want autoupdate blacklist checks.
+  service->updater()->set_blacklist_checks_enabled(false);
+
+  ExtensionTestMessageListener listener2("v2 installed", false);
+  // Run autoupdate and make sure version 2 of the extension was installed.
+  service->updater()->CheckNow();
+  ASSERT_TRUE(WaitForExtensionInstall());
+  ASSERT_EQ(disabled_size_before + 1, service->disabled_extensions()->size());
+  ASSERT_EQ(enabled_size_before, service->extensions()->size());

[asargent_no_longer_on_chrome, 2012/03/21 18:47:31]

same question here

+  extension = service->GetExtensionById(
+      "ogjcoiohnmldgjemafoockdghcjciccf", true);
+  ASSERT_TRUE(extension);
+  ASSERT_EQ("2.0", extension->VersionString());
+
+  // The extension should have not made the callback because it is disabled.
+  // When we enabled it, it should then make the callback.
+  ASSERT_FALSE(listener2.was_satisfied());

[asargent_no_longer_on_chrome, 2012/03/21 18:47:31]

If there were a bug where the extension did accidentally get re-enabled, this
check is probably a race condition because the message from the extension's
background page might or might not have been sent and processed during the spin
of the message loop when you block inside WaitForExtensionInstall() above. 

Instead, how about you just add a direct assertion that the extension is
disabled right where you assert it has the new version? E.g.

ASSERT_EQ(NULL, service->GetExtensionById(
    "ogjcoiohnmldgjemafoockdghcjciccf", false));

or maybe

ASSERT_TRUE(ContainsKey(service->disabled_extensions(),
                        extension));
just after line 406. 

[Matt Tytel, 2012/03/26 05:23:59]

Yea I was debating putting this line in for that reason, but it won't fail if it
works and it's a check that's not dependent on service->disabled_extensions().
I slightly prefer it there, but I'll remove it if you don't.

[asargent_no_longer_on_chrome, 2012/03/27 00:35:51]

Seems fine to leave it in since you added the check using GetExtensionById above
- I just didn't think it should be the only line of defense.

+  service->EnableExtension(extension->id());
+  listener2.WaitUntilSatisfied();
+  ASSERT_TRUE(notification_listener.started());
+  ASSERT_TRUE(notification_listener.finished());
+  ASSERT_TRUE(ContainsKey(notification_listener.updates(),
+                          "ogjcoiohnmldgjemafoockdghcjciccf"));
+  notification_listener.Reset();
+}
+
 IN_PROC_BROWSER_TEST_F(ExtensionManagementTest, ExternalUrlUpdate) {
   ExtensionService* service = browser()->profile()->GetExtensionService();
   const char* kExtensionId = "ogjcoiohnmldgjemafoockdghcjciccf";