Add an installType property to the management API

chrome/browser/extensions/api/management/management_apitest.cc

Index: chrome/browser/extensions/api/management/management_apitest.cc
diff --git a/chrome/browser/extensions/extension_management_apitest.cc b/chrome/browser/extensions/api/management/management_apitest.cc
similarity index 88%
rename from chrome/browser/extensions/extension_management_apitest.cc
rename to chrome/browser/extensions/api/management/management_apitest.cc
index 81a4347d44379b2ca251529bbd89476cb2850949..941787dcbb3eadcea97d3602b79a68e6997df672 100644
--- a/chrome/browser/extensions/extension_management_apitest.cc
+++ b/chrome/browser/extensions/api/management/management_apitest.cc
@@ -16,9 +16,11 @@
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_switches.h"
+#include "chrome/common/extensions/manifest.h"
 #include "content/public/test/test_utils.h"
 
 using extensions::Extension;
+using extensions::Manifest;
 
 namespace {
 
@@ -44,7 +46,7 @@ class ExtensionManagementApiTest : public ExtensionApiTest {
     command_line->AppendSwitch(switches::kEnablePanels);
   }
 
-  virtual void InstallExtensions() {
+  virtual void LoadExtensions() {
     FilePath basedir = test_data_dir_.AppendASCII("management");
 
     // Load 4 enabled items.
@@ -74,29 +76,45 @@ class ExtensionManagementApiTest : public ExtensionApiTest {
   }
 
  protected:
-  void InstallNamedExtension(FilePath basedir, std::string name) {
-    const Extension* extension = LoadExtension(basedir.AppendASCII(name));
-    ASSERT_TRUE(extension);
+  Extension* InstallNamedExtension(const FilePath& path,
+                                   const std::string& name) {
+    const Extension* extension = LoadExtension(path.AppendASCII(name));
+    //ASSERT_TRUE(extension);

[asargent_no_longer_on_chrome, 2012/07/31 19:55:53]

nit: you should leave this check in, but you can change it to:

if (!extension) {
  ADD_FAILURE("extension is null");
  return NULL;
}

[mitchellwrosen, 2012/07/31 21:48:48]

Sorry, forgot to un-comment this line. I was getting a weird compiler error that
I meant to investigate: void value ought to be ignored.

[asargent_no_longer_on_chrome, 2012/07/31 22:33:00]

yeah, ASSERT_TRUE(expr) compiles to something like:

{ if (expr) { ADD_FAILURE(); return; } }

so it's a common problem that when you are using it in a helper function which
returns void, but change to start returning something else, that you get the
compiler warning you about trying to return without a value of the appropriate
type. 
 

     extension_ids_[name] = extension->id();
+    return const_cast<Extension*>(extension);
   }
 
-  // Maps installed extension names to their IDs..
+  // Maps installed extension names to their IDs.
   std::map<std::string, std::string> extension_ids_;
 };
 
 IN_PROC_BROWSER_TEST_F(ExtensionManagementApiTest, Basics) {
-  InstallExtensions();
+  LoadExtensions();
+
+  FilePath basedir = test_data_dir_.AppendASCII("management");
+  Extension* extension = InstallNamedExtension(basedir, "internal_extension");
+  Manifest* manifest = const_cast<Manifest*>(extension->manifest());
+  manifest->location_ = Extension::INTERNAL;

[asargent_no_longer_on_chrome, 2012/07/31 19:55:53]

Instead of removing the const and relying on friending to be able to modify this
private data member, can you instead just do this?

const base::DictionaryValue* source = extension->manifest()->value();
scoped_ptr<Manifest> manifest(
    new Manifest(Extension::INTERNAL, source->DeepCopy());

...

manifest.reset(
  new Manifest(Extension::EXTERNAL_PREF, source->DeepCopy());


etc.

[mitchellwrosen, 2012/07/31 21:48:48]

Sure, but I'm not seeing any way to modify the extension's manifest with this
scoped ptr. Could you elaborate?

[asargent_no_longer_on_chrome, 2012/07/31 22:33:00]

Sorry, I didn't think this all the way through. I guess you need a couple
different extensions installed with various locations before we jump over to
finish the test by running the js test code in the extension's renderer process.


I think the right way to do this is to add the ability to specify an
Extension::Location to ExtensionBrowserTest::LoadExtensionWithOptions, and then
have a few different test extensions that you load with the various location
values we want to test.

+
+  extension = InstallNamedExtension(basedir, "external_extension");
+  manifest = const_cast<Manifest*>(extension->manifest());
+  manifest->location_ = Extension::EXTERNAL_PREF;
+
+  extension = InstallNamedExtension(basedir, "admin_extension");
+  manifest = const_cast<Manifest*>(extension->manifest());
+  manifest->location_ = Extension::EXTERNAL_POLICY_DOWNLOAD;
+
   ASSERT_TRUE(RunExtensionSubtest("management/test", "basics.html"));
 }
 
 IN_PROC_BROWSER_TEST_F(ExtensionManagementApiTest, Uninstall) {
-  InstallExtensions();
+  LoadExtensions();
   ASSERT_TRUE(RunExtensionSubtest("management/test", "uninstall.html"));
 }
 
 // Tests actions on extensions when no management policy is in place.
 IN_PROC_BROWSER_TEST_F(ExtensionManagementApiTest, ManagementPolicyAllowed) {
-  InstallExtensions();
+  LoadExtensions();
   ExtensionService* service = browser()->profile()->GetExtensionService();
   EXPECT_TRUE(service->GetExtensionById(extension_ids_["enabled_extension"],
                                         false));
@@ -114,7 +132,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionManagementApiTest, ManagementPolicyAllowed) {
 
 // Tests actions on extensions when management policy prohibits those actions.
 IN_PROC_BROWSER_TEST_F(ExtensionManagementApiTest, ManagementPolicyProhibited) {
-  InstallExtensions();
+  LoadExtensions();
   ExtensionService* service = browser()->profile()->GetExtensionService();
   EXPECT_TRUE(service->GetExtensionById(extension_ids_["enabled_extension"],
                                         false));