Clean up ExtensionServiceTest.

chrome/browser/extensions/extension_service_unittest.cc

Index: chrome/browser/extensions/extension_service_unittest.cc
diff --git a/chrome/browser/extensions/extension_service_unittest.cc b/chrome/browser/extensions/extension_service_unittest.cc
index 17160bfe04d9c70f8eaa540beb83f109e5a05afc..16db31a60130eacc71daee4581e5ad51c3fc6709 100644
--- a/chrome/browser/extensions/extension_service_unittest.cc
+++ b/chrome/browser/extensions/extension_service_unittest.cc
@@ -8,7 +8,6 @@
 #include <set>
 #include <vector>
 
-#include "base/at_exit.h"
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/command_line.h"
@@ -380,7 +379,7 @@ class ExtensionTestingProfile : public TestingProfile {
 // Our message loop may be used in tests which require it to be an IO loop.
 ExtensionServiceTestBase::ExtensionServiceTestBase()
     : service_(NULL),
-      total_successes_(0),
+      expected_extensions_count_(0),
       loop_(MessageLoop::TYPE_IO),
       ui_thread_(BrowserThread::UI, &loop_),
       db_thread_(BrowserThread::DB, &loop_),
@@ -432,7 +431,7 @@ void ExtensionServiceTestBase::InitializeExtensionService(
   // will register one specifically.
   service_->ClearProvidersForTesting();
 
-  total_successes_ = 0;
+  expected_extensions_count_ = 0;
 }
 
 void ExtensionServiceTestBase::InitializeInstalledExtensionService(
@@ -554,19 +553,13 @@ class ExtensionServiceTest
   void TestExternalProvider(MockExtensionProvider* provider,
                             Extension::Location location);
 
-  void PackAndInstallCrx(const FilePath& dir_path,
-                         const FilePath& pem_path,
-                         bool should_succeed) {
-    FilePath crx_path;
-    ScopedTempDir temp_dir;
-    ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
-    crx_path = temp_dir_.path().AppendASCII("temp.crx");
-
+  void PackCrx(const FilePath& dir_path,

[Aaron Boodman, 2011/11/20 21:39:43]

As long as you're in here, can you fix the capitalization (CRX instead of Crx)

[Yoyo Zhou, 2011/11/21 23:14:52]

Why is CRX better? It looks like neither capitalization is a clear winner in the
codebase.

[Aaron Boodman, 2011/11/22 10:02:59]

I did a search a few weeks back of things like "Url" vs "URL" and "Id" vs "ID"
and found that Chromium prefers all caps, so I've been trying to fix things to
be more consistent as I see them.

[Yoyo Zhou, 2011/11/22 19:40:55]

Ok, I'll keep that in mind. Changed.

+               const FilePath& pem_path,
+               const FilePath& crx_path) {
     // Use the existing pem key, if provided.
     FilePath pem_output_path;
     if (pem_path.value().empty()) {
       pem_output_path = crx_path.DirName().AppendASCII("temp.pem");
-      ASSERT_TRUE(file_util::Delete(pem_output_path, false));

[Aaron Boodman, 2011/11/20 21:39:43]

Why remove this?

[Yoyo Zhou, 2011/11/21 23:14:52]

If you look carefully at the code above,

crx_path = temp_dir_... (*not* temp_dir...)

The same tempdir was getting reused, so I assume this was necessary before.
However, if we use a new tempdir properly, it's probably fair to assume a file
path inside it will not exist.

     } else {
       ASSERT_TRUE(file_util::PathExists(pem_path));
     }
@@ -580,12 +573,23 @@ class ExtensionServiceTest
                              pem_output_path));
 
     ASSERT_TRUE(file_util::PathExists(crx_path));
-    InstallCrx(crx_path, should_succeed);
   }
 
-  void PackAndInstallCrx(const FilePath& dir_path,
-                         bool should_succeed) {
-    PackAndInstallCrx(dir_path, FilePath(), should_succeed);
+  const Extension* PackAndInstallCrx(const FilePath& dir_path,
+                                     const FilePath& pem_path,
+                                     bool should_succeed) {
+    FilePath crx_path;
+    ScopedTempDir temp_dir;
+    EXPECT_TRUE(temp_dir.CreateUniqueTempDir());
+    crx_path = temp_dir.path().AppendASCII("temp.crx");
+
+    PackCrx(dir_path, pem_path, crx_path);
+    return InstallCrx(crx_path, should_succeed);
+  }
+
+  const Extension* PackAndInstallCrx(const FilePath& dir_path,
+                                     bool should_succeed) {
+    return PackAndInstallCrx(dir_path, FilePath(), should_succeed);
   }
 
   // Create a CrxInstaller and start installation. To allow the install
@@ -605,16 +609,22 @@ class ExtensionServiceTest
     installer->InstallCrx(crx_path);
   }
 
-  void InstallCrx(const FilePath& path,
-                        bool should_succeed) {
+  const Extension* InstallCrx(const FilePath& path,
+                              bool should_succeed) {
     StartCrxInstall(path);
-    WaitForCrxInstall(path, should_succeed);
+    return WaitForCrxInstall(path, should_succeed);
   }
 
-  void InstallCrxWithLocation(const FilePath& crx_path,
-                              Extension::Location install_location,
-                              bool should_succeed) {
-    ASSERT_TRUE(file_util::PathExists(crx_path))
+  const Extension* InstallCrxFromWebStore(const FilePath& path,
+                                          bool should_succeed) {
+    StartCrxInstall(path, true);
+    return WaitForCrxInstall(path, should_succeed);
+  }
+
+  const Extension* InstallCrxWithLocation(const FilePath& crx_path,
+                                          Extension::Location install_location,
+                                          bool should_succeed) {
+    EXPECT_TRUE(file_util::PathExists(crx_path))
         << "Path does not exist: "<< crx_path.value().c_str();
     // no client (silent install)
     scoped_refptr<CrxInstaller> installer(CrxInstaller::Create(service_, NULL));
@@ -622,24 +632,27 @@ class ExtensionServiceTest
     installer->set_install_source(install_location);
     installer->InstallCrx(crx_path);
 
-    WaitForCrxInstall(crx_path, should_succeed);
+    return WaitForCrxInstall(crx_path, should_succeed);
   }
 
   // Wait for a CrxInstaller to finish. Used by InstallCrx.
-  void WaitForCrxInstall(const FilePath& path,
-                         bool should_succeed) {
+  // Returns the ID of the extension if the install succeeded, NULL otherwise.

[Aaron Boodman, 2011/11/20 21:39:43]

Comment is incorrect.

[Yoyo Zhou, 2011/11/21 23:14:52]

Oops, fixed.

+  const Extension* WaitForCrxInstall(const FilePath& path,
+                                     bool should_succeed) {
     loop_.RunAllPending();
     std::vector<std::string> errors = GetErrors();
+    const Extension* extension = NULL;
     if (should_succeed) {
-      ++total_successes_;
+      ++expected_extensions_count_;
 
       EXPECT_TRUE(installed_) << path.value();
 
-      ASSERT_EQ(1u, loaded_.size()) << path.value();
+      EXPECT_EQ(1u, loaded_.size()) << path.value();
       EXPECT_EQ(0u, errors.size()) << path.value();
-      EXPECT_EQ(total_successes_, service_->extensions()->size()) <<
+      EXPECT_EQ(expected_extensions_count_, service_->extensions()->size()) <<
           path.value();
-      EXPECT_TRUE(service_->GetExtensionById(loaded_[0]->id(), false)) <<
+      extension = loaded_[0];
+      EXPECT_TRUE(service_->GetExtensionById(extension->id(), false)) <<
           path.value();
       for (std::vector<std::string>::iterator err = errors.begin();
         err != errors.end(); ++err) {
@@ -654,6 +667,7 @@ class ExtensionServiceTest
     installed_ = NULL;
     loaded_.clear();
     ExtensionErrorReporter::GetInstance()->ClearErrors();
+    return extension;
   }
 
   enum UpdateState {
@@ -746,7 +760,7 @@ class ExtensionServiceTest
     } else {
       EXPECT_TRUE(service_->UninstallExtension(id, false, NULL));
     }
-    total_successes_ = 0;
+    expected_extensions_count_ = 0;

[Aaron Boodman, 2011/11/20 21:39:43]

Seems like maybe this isn't needed anymore? It's only tested by
WaitForCrxInstall() which doesn't seem to be called for remainder of test.

[Yoyo Zhou, 2011/11/21 23:14:52]

Well, if we ever have a test that uninstalls an extension, and then installs one
afterward using the helper methods, it would depend on this count. But we don't
actually have any such tests.

However, this just seems wrong. It should be --, not = 0.

[Aaron Boodman, 2011/11/22 10:02:59]

Fair enough.

 
     // We should get an unload notification.
     EXPECT_FALSE(unloaded_id_.empty());
@@ -984,7 +998,6 @@ void PackExtensionTestClient::OnPackFailure(const std::string& error_message) {
 
 // Test loading good extensions from the profile directory.
 TEST_F(ExtensionServiceTest, LoadAllExtensionsFromDirectorySuccess) {
-  base::ShadowingAtExitManager at_exit_manager;
   PluginService::GetInstance()->Init();
 
   // Initialize the test dir with a good Preferences/extensions.
@@ -997,6 +1010,7 @@ TEST_F(ExtensionServiceTest, LoadAllExtensionsFromDirectorySuccess) {
   InitializeInstalledExtensionService(pref_path, source_install_dir);
 
   service_->Init();
+  loop_.RunAllPending();

[Aaron Boodman, 2011/11/20 21:39:43]

Why is this important?

[Yoyo Zhou, 2011/11/21 23:14:52]

Not sure, it seems like the other tests that initialize ExtensionService like
this do it. Maybe they don't need it either since loading installed extensions
happens synchronously.

If I remove this from the rest of the tests, only CleanupOnStartup fails. So
I've removed it from the rest of the tests and annotated that one.

[Aaron Boodman, 2011/11/22 10:02:59]

In the old days ExtensionService startup was asynchronous, so this makes sense.
Cool. It makes sense that CleanupOnStartup needs this.

 
   uint32 expected_num_extensions = 3u;
   ASSERT_EQ(expected_num_extensions, loaded_.size());
@@ -1124,7 +1138,6 @@ TEST_F(ExtensionServiceTest, LoadAllExtensionsFromDirectoryFail) {
 // Test that partially deleted extensions are cleaned up during startup
 // Test loading bad extensions from the profile directory.
 TEST_F(ExtensionServiceTest, CleanupOnStartup) {
-  base::ShadowingAtExitManager at_exit_manager;
   PluginService::GetInstance()->Init();
 
   FilePath source_install_dir = data_dir_
@@ -1432,12 +1445,11 @@ TEST_F(ExtensionServiceTest, GrantedPermissions) {
       prefs->GetGrantedPermissions(permissions_crx));
   EXPECT_FALSE(known_perms.get());
 
-  PackAndInstallCrx(path, pem_path, true);
+  const Extension* extension = PackAndInstallCrx(path, pem_path, true);
 
   EXPECT_EQ(0u, GetErrors().size());
   ASSERT_EQ(1u, service_->extensions()->size());
-  std::string extension_id = service_->extensions()->at(0)->id();
-  EXPECT_EQ(permissions_crx, extension_id);
+  EXPECT_EQ(permissions_crx, extension->id());
 
   // Verify that the valid API permissions have been recognized.
   expected_api_perms.insert(ExtensionAPIPermission::kTab);
@@ -1447,7 +1459,7 @@ TEST_F(ExtensionServiceTest, GrantedPermissions) {
   AddPattern(&expected_host_perms, "http://*.google.com.hk/*");
   AddPattern(&expected_host_perms, "http://www.example.com/*");
 
-  known_perms = prefs->GetGrantedPermissions(extension_id);
+  known_perms = prefs->GetGrantedPermissions(extension->id());
   EXPECT_TRUE(known_perms.get());
   EXPECT_FALSE(known_perms->IsEmpty());
   EXPECT_EQ(expected_api_perms, known_perms->apis());
@@ -1460,7 +1472,6 @@ TEST_F(ExtensionServiceTest, GrantedPermissions) {
 // an extension contains an NPAPI plugin. Don't run this test on Chrome OS
 // since they don't support plugins.
 TEST_F(ExtensionServiceTest, GrantedFullAccessPermissions) {
-  base::ShadowingAtExitManager at_exit_manager;
   PluginService::GetInstance()->Init();
 
   InitializeEmptyExtensionService();
@@ -1472,15 +1483,13 @@ TEST_F(ExtensionServiceTest, GrantedFullAccessPermissions) {
       .AppendASCII("2");
 
   ASSERT_TRUE(file_util::PathExists(path));
-  PackAndInstallCrx(path, true);
+  const Extension* extension = PackAndInstallCrx(path, true);
   EXPECT_EQ(0u, GetErrors().size());
   EXPECT_EQ(1u, service_->extensions()->size());
-  const Extension* extension = service_->extensions()->at(0);
-  std::string extension_id = extension->id();
   ExtensionPrefs* prefs = service_->extension_prefs();
 
   scoped_refptr<ExtensionPermissionSet> permissions(
-      prefs->GetGrantedPermissions(extension_id));
+      prefs->GetGrantedPermissions(extension->id()));
   EXPECT_FALSE(permissions->IsEmpty());
   EXPECT_TRUE(permissions->HasEffectiveFullAccess());
   EXPECT_FALSE(permissions->apis().empty());
@@ -1503,11 +1512,10 @@ TEST_F(ExtensionServiceTest, GrantedAPIAndHostPermissions) {
 
   ASSERT_TRUE(file_util::PathExists(path));
 
-  PackAndInstallCrx(path, true);
+  const Extension* extension = PackAndInstallCrx(path, true);
 
   EXPECT_EQ(0u, GetErrors().size());
   EXPECT_EQ(1u, service_->extensions()->size());
-  const Extension* extension = service_->extensions()->at(0);
   std::string extension_id = extension->id();
 
   ExtensionPrefs* prefs = service_->extension_prefs();
@@ -1531,7 +1539,7 @@ TEST_F(ExtensionServiceTest, GrantedAPIAndHostPermissions) {
   service_->ReloadExtensions();
 
   EXPECT_EQ(1u, service_->disabled_extensions()->size());
-  extension = service_->disabled_extensions()->at(0);
+  extension = *service_->disabled_extensions()->begin();
 
   ASSERT_TRUE(prefs->IsExtensionDisabled(extension_id));
   ASSERT_FALSE(service_->IsExtensionEnabled(extension_id));
@@ -1574,7 +1582,7 @@ TEST_F(ExtensionServiceTest, GrantedAPIAndHostPermissions) {
   service_->ReloadExtensions();
 
   EXPECT_EQ(1u, service_->disabled_extensions()->size());
-  extension = service_->disabled_extensions()->at(0);
+  extension = *service_->disabled_extensions()->begin();
 
   ASSERT_TRUE(prefs->IsExtensionDisabled(extension_id));
   ASSERT_FALSE(service_->IsExtensionEnabled(extension_id));
@@ -1784,8 +1792,9 @@ TEST_F(ExtensionServiceTest, LoadLocalizedTheme) {
   EXPECT_EQ(0u, GetErrors().size());
   ASSERT_EQ(1u, loaded_.size());
   EXPECT_EQ(1u, service_->extensions()->size());
-  EXPECT_EQ("name", service_->extensions()->at(0)->name());
-  EXPECT_EQ("description", service_->extensions()->at(0)->description());
+  const Extension* theme = *service_->extensions()->begin();
+  EXPECT_EQ("name", theme->name());
+  EXPECT_EQ("description", theme->description());
 }
 
 TEST_F(ExtensionServiceTest, InstallLocalizedTheme) {
@@ -1793,25 +1802,24 @@ TEST_F(ExtensionServiceTest, InstallLocalizedTheme) {
   FilePath theme_path = data_dir_
       .AppendASCII("theme_i18n");
 
-  PackAndInstallCrx(theme_path, true);
+  const Extension* theme = PackAndInstallCrx(theme_path, true);
 
   EXPECT_EQ(0u, GetErrors().size());
   EXPECT_EQ(1u, service_->extensions()->size());
-  EXPECT_EQ("name", service_->extensions()->at(0)->name());
-  EXPECT_EQ("description", service_->extensions()->at(0)->description());
+  EXPECT_EQ("name", theme->name());
+  EXPECT_EQ("description", theme->description());
 }
 
 TEST_F(ExtensionServiceTest, InstallApps) {
   InitializeEmptyExtensionService();
 
   // An empty app.
-  PackAndInstallCrx(data_dir_.AppendASCII("app1"), true);
+  const Extension* app = PackAndInstallCrx(data_dir_.AppendASCII("app1"), true);
   int pref_count = 0;
   ValidatePrefKeyCount(++pref_count);
   ASSERT_EQ(1u, service_->extensions()->size());
-  std::string id = service_->extensions()->at(0)->id();
-  ValidateIntegerPref(id, "state", Extension::ENABLED);
-  ValidateIntegerPref(id, "location", Extension::INTERNAL);
+  ValidateIntegerPref(app->id(), "state", Extension::ENABLED);
+  ValidateIntegerPref(app->id(), "location", Extension::INTERNAL);
 
   // Another app with non-overlapping extent. Should succeed.
   PackAndInstallCrx(data_dir_.AppendASCII("app2"), true);
@@ -1825,13 +1833,14 @@ TEST_F(ExtensionServiceTest, InstallApps) {
 // Tests that file access is OFF by default.
 TEST_F(ExtensionServiceTest, DefaultFileAccess) {
   InitializeEmptyExtensionService();
-  PackAndInstallCrx(data_dir_.AppendASCII("permissions").AppendASCII("files"),
-                    true);
-
+  const Extension* extension =
+      PackAndInstallCrx(data_dir_
+                        .AppendASCII("permissions")
+                        .AppendASCII("files"),
+                        true);
   EXPECT_EQ(0u, GetErrors().size());
   EXPECT_EQ(1u, service_->extensions()->size());
-  std::string id = service_->extensions()->at(0)->id();
-  EXPECT_FALSE(service_->extension_prefs()->AllowFileAccess(id));
+  EXPECT_FALSE(service_->extension_prefs()->AllowFileAccess(extension->id()));
 }
 
 TEST_F(ExtensionServiceTest, UpdateApps) {
@@ -1839,21 +1848,21 @@ TEST_F(ExtensionServiceTest, UpdateApps) {
   FilePath extensions_path = data_dir_.AppendASCII("app_update");
 
   // First install v1 of a hosted app.
-  InstallCrx(extensions_path.AppendASCII("v1.crx"), true);
+  const Extension* extension =
+      InstallCrx(extensions_path.AppendASCII("v1.crx"), true);
   ASSERT_EQ(1u, service_->extensions()->size());
-  std::string id = service_->extensions()->at(0)->id();
-  ASSERT_EQ(std::string("1"),
-            service_->extensions()->at(0)->version()->GetString());
+  std::string id = extension->id();
+  ASSERT_EQ(std::string("1"), extension->version()->GetString());
 
   // Now try updating to v2.
   UpdateExtension(id,
                   extensions_path.AppendASCII("v2.crx"),
                   ENABLED);
   ASSERT_EQ(std::string("2"),
-            service_->extensions()->at(0)->version()->GetString());
+            service_->GetExtensionById(id, false)->version()->GetString());
 }
 
-TEST_F(ExtensionServiceTest, InstallAppsWithUnlimtedStorage) {
+TEST_F(ExtensionServiceTest, InstallAppsWithUnlimitedStorage) {
   InitializeEmptyExtensionService();
   InitializeRequestContext();
   EXPECT_TRUE(service_->extensions()->empty());
@@ -1861,10 +1870,10 @@ TEST_F(ExtensionServiceTest, InstallAppsWithUnlimtedStorage) {
   int pref_count = 0;
 
   // Install app1 with unlimited storage.
-  PackAndInstallCrx(data_dir_.AppendASCII("app1"), true);
+  const Extension* extension =
+      PackAndInstallCrx(data_dir_.AppendASCII("app1"), true);
   ValidatePrefKeyCount(++pref_count);
   ASSERT_EQ(1u, service_->extensions()->size());
-  const Extension* extension = service_->extensions()->at(0);
   const std::string id1 = extension->id();
   EXPECT_TRUE(extension->HasAPIPermission(
       ExtensionAPIPermission::kUnlimitedStorage));
@@ -1875,10 +1884,9 @@ TEST_F(ExtensionServiceTest, InstallAppsWithUnlimtedStorage) {
       IsStorageUnlimited(origin1));
 
   // Install app2 from the same origin with unlimited storage.
-  PackAndInstallCrx(data_dir_.AppendASCII("app2"), true);
+  extension = PackAndInstallCrx(data_dir_.AppendASCII("app2"), true);
   ValidatePrefKeyCount(++pref_count);
   ASSERT_EQ(2u, service_->extensions()->size());
-  extension = service_->extensions()->at(1);
   const std::string id2 = extension->id();
   EXPECT_TRUE(extension->HasAPIPermission(
       ExtensionAPIPermission::kUnlimitedStorage));
@@ -1912,10 +1920,10 @@ TEST_F(ExtensionServiceTest, InstallAppsAndCheckStorageProtection) {
 
   int pref_count = 0;
 
-  PackAndInstallCrx(data_dir_.AppendASCII("app1"), true);
+  const Extension* extension =
+      PackAndInstallCrx(data_dir_.AppendASCII("app1"), true);
   ValidatePrefKeyCount(++pref_count);
   ASSERT_EQ(1u, service_->extensions()->size());
-  const Extension* extension = service_->extensions()->at(0);
   EXPECT_TRUE(extension->is_app());
   const std::string id1 = extension->id();
   const GURL origin1(extension->GetFullLaunchURL().GetOrigin());
@@ -1923,10 +1931,9 @@ TEST_F(ExtensionServiceTest, InstallAppsAndCheckStorageProtection) {
       IsStorageProtected(origin1));
 
   // App 4 has a different origin (maps.google.com).
-  PackAndInstallCrx(data_dir_.AppendASCII("app4"), true);
+  extension = PackAndInstallCrx(data_dir_.AppendASCII("app4"), true);
   ValidatePrefKeyCount(++pref_count);
   ASSERT_EQ(2u, service_->extensions()->size());
-  extension = service_->extensions()->at(1);
   const std::string id2 = extension->id();
   const GURL origin2(extension->GetFullLaunchURL().GetOrigin());
   ASSERT_NE(origin1, origin2);
@@ -1951,27 +1958,16 @@ TEST_F(ExtensionServiceTest, Reinstall) {
 
   // A simple extension that should install without error.
   FilePath path = data_dir_.AppendASCII("good.crx");
-  StartCrxInstall(path);
-  loop_.RunAllPending();
+  InstallCrx(path, true);
 
-  ASSERT_TRUE(installed_);
-  ASSERT_EQ(1u, loaded_.size());
-  ASSERT_EQ(0u, GetErrors().size());
   ValidatePrefKeyCount(1);
   ValidateIntegerPref(good_crx, "state", Extension::ENABLED);
   ValidateIntegerPref(good_crx, "location", Extension::INTERNAL);
 
-  installed_ = NULL;
-  loaded_.clear();
-  ExtensionErrorReporter::GetInstance()->ClearErrors();
-
   // Reinstall the same version, it should overwrite the previous one.
-  StartCrxInstall(path);
-  loop_.RunAllPending();
+  expected_extensions_count_--;
+  InstallCrx(path, true);
 
-  ASSERT_TRUE(installed_);
-  ASSERT_EQ(1u, loaded_.size());
-  ASSERT_EQ(0u, GetErrors().size());
   ValidatePrefKeyCount(1);
   ValidateIntegerPref(good_crx, "state", Extension::ENABLED);
   ValidateIntegerPref(good_crx, "location", Extension::INTERNAL);
@@ -1984,35 +1980,23 @@ TEST_F(ExtensionServiceTest, FromWebStore) {
 
   // A simple extension that should install without error.
   FilePath path = data_dir_.AppendASCII("good.crx");
-  StartCrxInstall(path, false);  // Not from web store.
-  loop_.RunAllPending();
+  const Extension* extension = InstallCrx(path, true);  // Not from web store.
+  std::string id = extension->id();
 
-  ASSERT_TRUE(installed_);
-  ASSERT_EQ(1u, loaded_.size());
-  ASSERT_EQ(0u, GetErrors().size());
   ValidatePrefKeyCount(1);
   ValidateBooleanPref(good_crx, "from_webstore", false);
-
-  const Extension* extension = service_->extensions()->at(0);
   ASSERT_FALSE(extension->from_webstore());
 
-  installed_ = NULL;
-  loaded_.clear();
-  ExtensionErrorReporter::GetInstance()->ClearErrors();
-
   // Test install from web store.
-  StartCrxInstall(path, true);  // From web store.
-  loop_.RunAllPending();
+  expected_extensions_count_--;
+  InstallCrxFromWebStore(path, true);  // From web store.
 
-  ASSERT_TRUE(installed_);
-  ASSERT_EQ(1u, loaded_.size());
-  ASSERT_EQ(0u, GetErrors().size());
   ValidatePrefKeyCount(1);
   ValidateBooleanPref(good_crx, "from_webstore", true);
 
   // Reload so extension gets reinitialized with new value.
   service_->ReloadExtensions();
-  extension = service_->extensions()->at(0);
+  extension = service_->GetExtensionById(id, false);
   ASSERT_TRUE(extension->from_webstore());
 
   // Upgrade to version 2.0
@@ -2027,22 +2011,19 @@ TEST_F(ExtensionServiceTest, UpgradeSignedGood) {
   InitializeEmptyExtensionService();
 
   FilePath path = data_dir_.AppendASCII("good.crx");
-  StartCrxInstall(path);
-  loop_.RunAllPending();
+  const Extension* extension = InstallCrx(path, true);
+  std::string id = extension->id();
 
-  ASSERT_TRUE(installed_);
-  ASSERT_EQ(1u, loaded_.size());
-  ASSERT_EQ("1.0.0.0", loaded_[0]->version()->GetString());
+  ASSERT_EQ("1.0.0.0", extension->version()->GetString());
   ASSERT_EQ(0u, GetErrors().size());
 
-  // Upgrade to version 2.0
+  // Upgrade to version 1.0.0.1
   path = data_dir_.AppendASCII("good2.crx");
-  StartCrxInstall(path);
-  loop_.RunAllPending();
+  expected_extensions_count_--;

[Aaron Boodman, 2011/11/20 21:39:43]

A little weird to directly modify the count in all these update cases. What if
instead, InstallCrx took an enum describing expected behavior (e.g.,
SUCCEED_NEW, SUCCEED_UPDATE, FAIL).

[Yoyo Zhou, 2011/11/21 23:14:52]

Yeah, I was looking at doing a similar thing to extension_browsertest and it
seems to be better that way.

Unfortunately the ideal names seem as though they will conflict with the
existing UpdateState.

+  InstallCrx(path, true);
+  extension = service_->GetExtensionById(id, false);
 
-  ASSERT_TRUE(installed_);
-  ASSERT_EQ(1u, loaded_.size());
-  ASSERT_EQ("1.0.0.1", loaded_[0]->version()->GetString());
+  ASSERT_EQ("1.0.0.1", extension->version()->GetString());
   ASSERT_EQ(0u, GetErrors().size());
 }
 
@@ -2051,23 +2032,12 @@ TEST_F(ExtensionServiceTest, UpgradeSignedBad) {
   InitializeEmptyExtensionService();
 
   FilePath path = data_dir_.AppendASCII("good.crx");
-  StartCrxInstall(path);
-  loop_.RunAllPending();
-
-  ASSERT_TRUE(installed_);
-  ASSERT_EQ(1u, loaded_.size());
-  ASSERT_EQ(0u, GetErrors().size());
-  installed_ = NULL;
+  InstallCrx(path, true);
 
   // Try upgrading with a bad signature. This should fail during the unpack,
   // because the key will not match the signature.
   path = data_dir_.AppendASCII("bad_signature.crx");
-  StartCrxInstall(path);
-  loop_.RunAllPending();
-
-  ASSERT_FALSE(installed_);
-  ASSERT_EQ(1u, loaded_.size());
-  ASSERT_EQ(1u, GetErrors().size());
+  InstallCrx(path, false);
 }
 
 // Test a normal update via the UpdateExtension API
@@ -2076,14 +2046,15 @@ TEST_F(ExtensionServiceTest, UpdateExtension) {
 
   FilePath path = data_dir_.AppendASCII("good.crx");
 
-  InstallCrx(path, true);
-  const Extension* good = service_->extensions()->at(0);
+  const Extension* good = InstallCrx(path, true);
   ASSERT_EQ("1.0.0.0", good->VersionString());
   ASSERT_EQ(good_crx, good->id());
 
   path = data_dir_.AppendASCII("good2.crx");
   UpdateExtension(good_crx, path, ENABLED);
-  ASSERT_EQ("1.0.0.1", loaded_[0]->version()->GetString());
+  ASSERT_EQ("1.0.0.1",
+            service_->GetExtensionById(good_crx, false)->
+            version()->GetString());
 }
 
 // Test updating a not-already-installed extension - this should fail
@@ -2105,15 +2076,16 @@ TEST_F(ExtensionServiceTest, UpdateWillNotDowngrade) {
 
   FilePath path = data_dir_.AppendASCII("good2.crx");
 
-  InstallCrx(path, true);
-  const Extension* good = service_->extensions()->at(0);
+  const Extension* good = InstallCrx(path, true);
   ASSERT_EQ("1.0.0.1", good->VersionString());
   ASSERT_EQ(good_crx, good->id());
 
   // Change path from good2.crx -> good.crx
   path = data_dir_.AppendASCII("good.crx");
   UpdateExtension(good_crx, path, FAILED);
-  ASSERT_EQ("1.0.0.1", service_->extensions()->at(0)->VersionString());
+  ASSERT_EQ("1.0.0.1",
+            service_->GetExtensionById(good_crx, false)->
+            version()->GetString());
 }
 
 // Make sure calling update with an identical version does nothing
@@ -2122,8 +2094,7 @@ TEST_F(ExtensionServiceTest, UpdateToSameVersionIsNoop) {
 
   FilePath path = data_dir_.AppendASCII("good.crx");
 
-  InstallCrx(path, true);
-  const Extension* good = service_->extensions()->at(0);
+  const Extension* good = InstallCrx(path, true);
   ASSERT_EQ(good_crx, good->id());
   UpdateExtension(good_crx, path, FAILED_SILENTLY);
 }
@@ -2134,8 +2105,7 @@ TEST_F(ExtensionServiceTest, UpdateExtensionPreservesState) {
 
   FilePath path = data_dir_.AppendASCII("good.crx");
 
-  InstallCrx(path, true);
-  const Extension* good = service_->extensions()->at(0);
+  const Extension* good = InstallCrx(path, true);
   ASSERT_EQ("1.0.0.0", good->VersionString());
   ASSERT_EQ(good_crx, good->id());
 
@@ -2147,7 +2117,7 @@ TEST_F(ExtensionServiceTest, UpdateExtensionPreservesState) {
   path = data_dir_.AppendASCII("good2.crx");
   UpdateExtension(good_crx, path, INSTALLED);
   ASSERT_EQ(1u, service_->disabled_extensions()->size());
-  const Extension* good2 = service_->disabled_extensions()->at(0);
+  const Extension* good2 = service_->GetExtensionById(good_crx, true);
   ASSERT_EQ("1.0.0.1", good2->version()->GetString());
   EXPECT_TRUE(service_->IsIncognitoEnabled(good2->id()));
 }
@@ -2158,8 +2128,7 @@ TEST_F(ExtensionServiceTest, UpdateExtensionPreservesLocation) {
 
   FilePath path = data_dir_.AppendASCII("good.crx");
 
-  InstallCrx(path, true);
-  const Extension* good = service_->extensions()->at(0);
+  const Extension* good = InstallCrx(path, true);
 
   ASSERT_EQ("1.0.0.0", good->VersionString());
   ASSERT_EQ(good_crx, good->id());
@@ -2169,7 +2138,7 @@ TEST_F(ExtensionServiceTest, UpdateExtensionPreservesLocation) {
 
   path = data_dir_.AppendASCII("good2.crx");
   UpdateExtension(good_crx, path, ENABLED);
-  const Extension* good2 = service_->extensions()->at(0);
+  const Extension* good2 = service_->GetExtensionById(good_crx, false);
   ASSERT_EQ("1.0.0.1", good2->version()->GetString());
   EXPECT_EQ(good2->location(), Extension::EXTERNAL_PREF);
 }
@@ -2428,9 +2397,8 @@ TEST_F(ExtensionServiceTest, UpdatePendingExtensionAlreadyInstalled) {
   InitializeEmptyExtensionService();
 
   FilePath path = data_dir_.AppendASCII("good.crx");
-  InstallCrx(path, true);
+  const Extension* good = InstallCrx(path, true);
   ASSERT_EQ(1u, service_->extensions()->size());
-  const Extension* good = service_->extensions()->at(0);
 
   EXPECT_FALSE(good->is_theme());
 
@@ -2478,8 +2446,7 @@ TEST_F(ExtensionServiceTest, UnloadBlacklistedExtension) {
 
   FilePath path = data_dir_.AppendASCII("good.crx");
 
-  InstallCrx(path, true);
-  const Extension* good = service_->extensions()->at(0);
+  const Extension* good = InstallCrx(path, true);
   EXPECT_EQ(good_crx, good->id());
   UpdateExtension(good_crx, path, FAILED_SILENTLY);
 
@@ -2516,8 +2483,7 @@ TEST_F(ExtensionServiceTest, BlacklistedExtensionWillNotInstall) {
 
   // We can not install good_crx.
   FilePath path = data_dir_.AppendASCII("good.crx");
-  StartCrxInstall(path);
-  loop_.RunAllPending();
+  InstallCrx(path, false);
   EXPECT_EQ(0u, service_->extensions()->size());
   ValidateBooleanPref(good_crx, "blacklist", true);
 }
@@ -2554,8 +2520,7 @@ TEST_F(ExtensionServiceTest, WillNotLoadBlacklistedExtensionsFromDirectory) {
   }
   ASSERT_EQ(2u, loaded_.size());
 
-  EXPECT_NE(std::string(good1), loaded_[0]->id());
-  EXPECT_NE(std::string(good1), loaded_[1]->id());
+  EXPECT_FALSE(service_->GetExtensionById(good1, true));
 }
 
 // Will not install extension blacklisted by policy.
@@ -2572,8 +2537,7 @@ TEST_F(ExtensionServiceTest, BlacklistedByPolicyWillNotInstall) {
 
   // Blacklist prevents us from installing good_crx.
   FilePath path = data_dir_.AppendASCII("good.crx");
-  StartCrxInstall(path);
-  loop_.RunAllPending();
+  InstallCrx(path, false);
   EXPECT_EQ(0u, service_->extensions()->size());
 
   // Now whitelist this particular extension.
@@ -2584,10 +2548,8 @@ TEST_F(ExtensionServiceTest, BlacklistedByPolicyWillNotInstall) {
     whitelist->Append(Value::CreateStringValue(good_crx));
   }
 
-
   // Ensure we can now install good_crx.
-  StartCrxInstall(path);
-  loop_.RunAllPending();
+  InstallCrx(path, true);
   EXPECT_EQ(1u, service_->extensions()->size());
 }
 
@@ -2597,8 +2559,7 @@ TEST_F(ExtensionServiceTest, BlacklistedByPolicyRemovedIfRunning) {
 
   // Install good_crx.
   FilePath path = data_dir_.AppendASCII("good.crx");
-  StartCrxInstall(path);
-  loop_.RunAllPending();
+  InstallCrx(path, true);
   EXPECT_EQ(1u, service_->extensions()->size());
 
   { // Scope for pref update notification.
@@ -2643,12 +2604,12 @@ TEST_F(ExtensionServiceTest, ComponentExtensionWhitelisted) {
 
   // Extension should be installed despite blacklist.
   ASSERT_EQ(1u, service_->extensions()->size());
-  EXPECT_EQ(good0, service_->extensions()->at(0)->id());
+  EXPECT_TRUE(service_->GetExtensionById(good0, false));
 
   // Poke external providers and make sure the extension is still present.
   service_->CheckForExternalUpdates();
   ASSERT_EQ(1u, service_->extensions()->size());
-  EXPECT_EQ(good0, service_->extensions()->at(0)->id());
+  EXPECT_TRUE(service_->GetExtensionById(good0, false));
 
   // Extension should not be uninstalled on blacklist changes.
   {
@@ -2659,7 +2620,7 @@ TEST_F(ExtensionServiceTest, ComponentExtensionWhitelisted) {
   }
   loop_.RunAllPending();
   ASSERT_EQ(1u, service_->extensions()->size());
-  EXPECT_EQ(good0, service_->extensions()->at(0)->id());
+  EXPECT_TRUE(service_->GetExtensionById(good0, false));
 }
 
 // Tests that policy-installed extensions are not blacklisted by policy.
@@ -2689,7 +2650,7 @@ TEST_F(ExtensionServiceTest, PolicyInstalledExtensionsWhitelisted) {
 
   // Extension should be installed despite blacklist.
   ASSERT_EQ(1u, service_->extensions()->size());
-  EXPECT_EQ(good_crx, service_->extensions()->at(0)->id());
+  EXPECT_TRUE(service_->GetExtensionById(good_crx, false));
 
   // Blacklist update should not uninstall the extension.
   {
@@ -2700,7 +2661,7 @@ TEST_F(ExtensionServiceTest, PolicyInstalledExtensionsWhitelisted) {
   }
   loop_.RunAllPending();
   ASSERT_EQ(1u, service_->extensions()->size());
-  EXPECT_EQ(good_crx, service_->extensions()->at(0)->id());
+  EXPECT_TRUE(service_->GetExtensionById(good_crx, false));
 }
 
 // Tests disabling extensions
@@ -2709,16 +2670,16 @@ TEST_F(ExtensionServiceTest, DisableExtension) {
 
   InstallCrx(data_dir_.AppendASCII("good.crx"), true);
   EXPECT_FALSE(service_->extensions()->empty());
-  EXPECT_TRUE(service_->GetExtensionById(good_crx, true) != NULL);
-  EXPECT_TRUE(service_->GetExtensionById(good_crx, false) != NULL);
+  EXPECT_TRUE(service_->GetExtensionById(good_crx, true));
+  EXPECT_TRUE(service_->GetExtensionById(good_crx, false));
   EXPECT_TRUE(service_->disabled_extensions()->empty());
 
   // Disable it.
   service_->DisableExtension(good_crx);
 
   EXPECT_TRUE(service_->extensions()->empty());
-  EXPECT_TRUE(service_->GetExtensionById(good_crx, true) != NULL);
-  EXPECT_FALSE(service_->GetExtensionById(good_crx, false) != NULL);
+  EXPECT_TRUE(service_->GetExtensionById(good_crx, true));
+  EXPECT_FALSE(service_->GetExtensionById(good_crx, false));
   EXPECT_FALSE(service_->disabled_extensions()->empty());
 }
 
@@ -2733,7 +2694,7 @@ TEST_F(ExtensionServiceTest, DisableTerminatedExtension) {
   service_->DisableExtension(good_crx);
 
   EXPECT_FALSE(service_->GetTerminatedExtension(good_crx));
-  EXPECT_TRUE(service_->GetExtensionById(good_crx, true) != NULL);
+  EXPECT_TRUE(service_->GetExtensionById(good_crx, true));
   EXPECT_FALSE(service_->disabled_extensions()->empty());
 }
 
@@ -2741,7 +2702,6 @@ TEST_F(ExtensionServiceTest, DisableTerminatedExtension) {
 TEST_F(ExtensionServiceTest, DisableAllExtensions) {
   InitializeEmptyExtensionService();
 
-
   FilePath path = data_dir_.AppendASCII("good.crx");
   InstallCrx(path, true);
 
@@ -2772,7 +2732,7 @@ TEST_F(ExtensionServiceTest, DisableAllExtensions) {
   EXPECT_EQ(0u, service_->disabled_extensions()->size());
 }
 
-// Tests reloading extensions
+// Tests reloading extensions.
 TEST_F(ExtensionServiceTest, ReloadExtensions) {
   InitializeEmptyExtensionService();
 
@@ -2873,8 +2833,7 @@ TEST_F(ExtensionServiceTest, ClearExtensionData) {
   // Load a test extension.
   FilePath path = data_dir_;
   path = path.AppendASCII("good.crx");
-  InstallCrx(path, true);
-  const Extension* extension = service_->GetExtensionById(good_crx, false);
+  const Extension* extension = InstallCrx(path, true);
   ASSERT_TRUE(extension);
   GURL ext_url(extension->url());
   string16 origin_id =
@@ -2962,10 +2921,10 @@ TEST_F(ExtensionServiceTest, ClearAppData) {
   int pref_count = 0;
 
   // Install app1 with unlimited storage.
-  PackAndInstallCrx(data_dir_.AppendASCII("app1"), true);
+  const Extension* extension =
+      PackAndInstallCrx(data_dir_.AppendASCII("app1"), true);
   ValidatePrefKeyCount(++pref_count);
   ASSERT_EQ(1u, service_->extensions()->size());
-  const Extension* extension = service_->extensions()->at(0);
   const std::string id1 = extension->id();
   EXPECT_TRUE(extension->HasAPIPermission(
       ExtensionAPIPermission::kUnlimitedStorage));
@@ -2976,10 +2935,9 @@ TEST_F(ExtensionServiceTest, ClearAppData) {
       webkit_database::DatabaseUtil::GetOriginIdentifier(origin1);
 
   // Install app2 from the same origin with unlimited storage.
-  PackAndInstallCrx(data_dir_.AppendASCII("app2"), true);
+  extension = PackAndInstallCrx(data_dir_.AppendASCII("app2"), true);
   ValidatePrefKeyCount(++pref_count);
   ASSERT_EQ(2u, service_->extensions()->size());
-  extension = service_->extensions()->at(1);
   const std::string id2 = extension->id();
   EXPECT_TRUE(extension->HasAPIPermission(
       ExtensionAPIPermission::kUnlimitedStorage));
@@ -3124,7 +3082,6 @@ TEST_F(ExtensionServiceTest, LoadExtension) {
 TEST_F(ExtensionServiceTest, GenerateID) {
   InitializeEmptyExtensionService();
 
-
   FilePath no_id_ext = data_dir_.AppendASCII("no_id");
   extensions::UnpackedInstaller::Create(service_)->Load(no_id_ext);
   loop_.RunAllPending();
@@ -3766,11 +3723,11 @@ TEST_F(ExtensionServiceTest, ComponentExtensions) {
   ValidatePrefKeyCount(0);
 
   // Reload all extensions, and make sure it comes back.
-  std::string extension_id = service_->extensions()->at(0)->id();
+  std::string extension_id = (*service_->extensions()->begin())->id();
   loaded_.clear();
   service_->ReloadExtensions();
   ASSERT_EQ(1u, service_->extensions()->size());
-  EXPECT_EQ(extension_id, service_->extensions()->at(0)->id());
+  EXPECT_EQ(extension_id, (*service_->extensions()->begin())->id());
 }
 
 namespace {