Improve settings override bubble to indicate policy installed extensions

chrome/browser/extensions/extension_service_test_with_install.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/extension_service_test_with_install.h"
 
 #include "base/files/file_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/extensions/crx_installer.h"
 #include "chrome/browser/extensions/extension_creator.h"
@@ skipping 108 matching lines @@
                            Extension::NO_FLAGS);
 }
 
 const Extension* ExtensionServiceTestWithInstall::PackAndInstallCRX(
     const base::FilePath& dir_path,
     InstallState install_state) {
   return PackAndInstallCRX(dir_path, base::FilePath(), install_state,
                            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
 // non-empty, expects that the existing extension's title was
 // |expected_old_name|.
 const Extension* ExtensionServiceTestWithInstall::InstallCRX(
     const base::FilePath& path,
     InstallState install_state,
     int creation_flags,
     const std::string& expected_old_name) {
@@ skipping 184 matching lines @@
     const std::string& id,
     bool use_helper,
     Extension::State expected_state) {
   // Verify that the extension is installed.
   base::FilePath extension_path = extensions_install_dir().AppendASCII(id);
   EXPECT_TRUE(base::PathExists(extension_path));
   size_t pref_key_count = GetPrefKeyCount();
   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(
         service(), id, extensions::UNINSTALL_REASON_FOR_TESTING));
   } else {
     EXPECT_TRUE(service()->UninstallExtension(
         id, extensions::UNINSTALL_REASON_FOR_TESTING,
         base::Bind(&base::DoNothing), nullptr));
   }
   --expected_extensions_count_;
 
   // 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();
   if (new_pref_key_count == pref_key_count) {
     ValidateIntegerPref(id, "state",
                         Extension::EXTERNAL_EXTENSION_UNINSTALLED);
   } else {
     EXPECT_EQ(new_pref_key_count, pref_key_count - 1);
   }
 
   // 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.
   EXPECT_FALSE(base::PathExists(extension_path));
 }
 
 void ExtensionServiceTestWithInstall::TerminateExtension(
     const std::string& id) {
   const Extension* extension = service()->GetInstalledExtension(id);
   if (!extension) {
@@ skipping 54 matching lines @@
   content::WindowedNotificationObserver observer(
       extensions::NOTIFICATION_CRX_INSTALLER_DONE,
       content::Source<extensions::CrxInstaller>(installer.get()));
 
   installer->InstallCrx(crx_path);
 
   observer.Wait();
 }
 
 }  // namespace extensions