Improve settings override bubble to indicate policy installed extensions

chrome/browser/extensions/extension_service_test_with_install.cc

Index: chrome/browser/extensions/extension_service_test_with_install.cc
diff --git a/chrome/browser/extensions/extension_service_test_with_install.cc b/chrome/browser/extensions/extension_service_test_with_install.cc
index 9ff4cf07e22e1857a8d6baa20e5c2f03e42d08cc..aea8e970e224fb7c3073b410bc8b2d106bedcfcd 100644
--- a/chrome/browser/extensions/extension_service_test_with_install.cc
+++ b/chrome/browser/extensions/extension_service_test_with_install.cc
@@ -126,6 +126,29 @@ const Extension* ExtensionServiceTestWithInstall::PackAndInstallCRX(
                            Extension::NO_FLAGS);
 }
 
+const Extension* ExtensionServiceTestWithInstall::PackAndInstallCRXWithLocation(
+    const base::FilePath& dir_path,
+    Manifest::Location install_location,
+    InstallState install_state) {
+  base::FilePath crx_path;

[Devlin, 2016/09/26 20:13:09]

With functions like these, it's best to house the implementation in a single
place and just fill in the missing pieces, rather than copying the code.  As an
example, PackAndInstallCRX on line 122 just calls PackAndInstallCRX(), adding in
reasonable defaults for file path and flags.  So here, I think we should add a
new version of PackAndInstallCRX that includes a location, and have others call
into that.

[catmullings, 2016/10/06 18:24:19]

Wrt "add a new version of PackAndInstallCRX that includes a location", do you
mean create something like the PackAndInstallCRX() at line 100, except swap out
line 111 with lines 140 - 149?

[Devlin, 2016/10/10 15:27:28]

Not quite.  With these types of functions, where the majority of the
implementation is the same and it's just one line that might be different, it's
best to have only one version actually do the implementation and have the rest
call into that version.  The version with the implementation takes all possible
arguments, and the other versions call into that version, providing defaults for
any missing arguments.  You can see we already do this for most
"PackAndInstallCRX()" function versions here - e.g. the ones on line 114 and 122
both just call into PackAndInstallCRX providing flag and/or pem path values.

So for this, instead of writing a new function with the full implementation, we
should add a location argument to the "main" PackAndInstallCRX() function, and
have others provide the default install location.  Does that make sense?

[catmullings, 2016/10/12 18:58:58]

Done.

+  base::ScopedTempDir temp_dir;
+  EXPECT_TRUE(temp_dir.CreateUniqueTempDir());
+  crx_path = temp_dir.GetPath().AppendASCII("temp.crx");
+
+  PackCRX(dir_path, base::FilePath(), crx_path);
+
+  scoped_refptr<CrxInstaller> installer(CrxInstaller::CreateSilent(service()));
+  installer->set_install_source(install_location);
+
+  content::WindowedNotificationObserver observer(
+      extensions::NOTIFICATION_CRX_INSTALLER_DONE,
+      content::NotificationService::AllSources());
+  installer->InstallCrx(crx_path);
+  observer.Wait();
+
+  return VerifyCrxInstall(crx_path, install_state);
+}
+
 // Attempts to install an extension. Use INSTALL_FAILED if the installation
 // is expected to fail.
 // If |install_state| is INSTALL_UPDATED, and |expected_old_name| is
@@ -330,6 +353,9 @@ void ExtensionServiceTestWithInstall::UninstallExtension(
   EXPECT_GT(pref_key_count, 0u);
   ValidateIntegerPref(id, "state", expected_state);
 
+  // We make a copy of the extension's id since the extension can be deleted
+  // once it's uninstalled.
+  std::string extension_id = id;
   // Uninstall it.
   if (use_helper) {
     EXPECT_TRUE(ExtensionService::UninstallExtensionHelper(
@@ -343,7 +369,7 @@ void ExtensionServiceTestWithInstall::UninstallExtension(
 
   // We should get an unload notification.
   EXPECT_FALSE(unloaded_id_.empty());
-  EXPECT_EQ(id, unloaded_id_);
+  EXPECT_EQ(extension_id, unloaded_id_);
 
   // Verify uninstalled state.
   size_t new_pref_key_count = GetPrefKeyCount();
@@ -355,7 +381,7 @@ void ExtensionServiceTestWithInstall::UninstallExtension(
   }
 
   // The extension should not be in the service anymore.
-  EXPECT_FALSE(service()->GetInstalledExtension(id));
+  EXPECT_FALSE(service()->GetInstalledExtension(extension_id));
   base::RunLoop().RunUntilIdle();
 
   // The directory should be gone.