Add an installType property to the management API

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

 // Copyright (c) 2012 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 <map>
 
 #include "chrome/browser/extensions/extension_apitest.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/extensions/extension_test_message_listener.h"
 #include "chrome/browser/extensions/test_management_policy.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/browser_finder.h"
 #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 {
 
 // Find a browser other than |browser|.
 Browser* FindOtherBrowser(Browser* browser) {
   Browser* found = NULL;
   for (BrowserList::const_iterator it = BrowserList::begin();
        it != BrowserList::end(); ++it) {
     if (*it == browser)
       continue;
     found = *it;
   }
 
   return found;
 }
 
 }  // namespace
 
 class ExtensionManagementApiTest : public ExtensionApiTest {
  public:
   virtual void SetUpCommandLine(CommandLine* command_line) {
     ExtensionApiTest::SetUpCommandLine(command_line);
     command_line->AppendSwitch(switches::kEnablePanels);
   }
 
-  virtual void InstallExtensions() {
+  virtual void LoadExtensions() {
     FilePath basedir = test_data_dir_.AppendASCII("management");
 
     // Load 4 enabled items.
     InstallNamedExtension(basedir, "enabled_extension");
     InstallNamedExtension(basedir, "enabled_app");
     InstallNamedExtension(basedir, "description");
     InstallNamedExtension(basedir, "permissions");
 
     // Load 2 disabled items.
     InstallNamedExtension(basedir, "disabled_extension");
     DisableExtension(extension_ids_["disabled_extension"]);
     InstallNamedExtension(basedir, "disabled_app");
     DisableExtension(extension_ids_["disabled_app"]);
   }
 
   // Load an app, and wait for a message from app "management/launch_on_install"
   // indicating that the new app has been launched.
   void LoadAndWaitForLaunch(const std::string& app_path,
                             std::string* out_app_id) {
     ExtensionTestMessageListener launched_app("launched app", false);
     ASSERT_TRUE(LoadExtension(test_data_dir_.AppendASCII(app_path)));
 
     if (out_app_id)
       *out_app_id = last_loaded_extension_id_;
 
     ASSERT_TRUE(launched_app.WaitUntilSatisfied());
   }
 
  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));
 
   // Ensure that all actions are allowed.
   extensions::ExtensionSystem::Get(
       browser()->profile())->management_policy()->UnregisterAllProviders();
 
   ASSERT_TRUE(RunExtensionSubtest("management/management_policy",
                                   "allowed.html"));
   // The last thing the test does is uninstall the "enabled_extension".
   EXPECT_FALSE(service->GetExtensionById(extension_ids_["enabled_extension"],
                                          true));
 }
 
 // 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));
 
   // Prohibit status changes.
   extensions::ManagementPolicy* policy = extensions::ExtensionSystem::Get(
       browser()->profile())->management_policy();
   policy->UnregisterAllProviders();
   extensions::TestManagementPolicyProvider provider(
     extensions::TestManagementPolicyProvider::PROHIBIT_MODIFY_STATUS);
@@ skipping 104 matching lines @@
   ASSERT_EQ(2, browser()->tab_count());
 #else
   // Find the app's browser.  Opening in a new window will create
   // a new browser.
   ASSERT_EQ(2u, browser::GetBrowserCount(browser()->profile()));
   Browser* app_browser = FindOtherBrowser(browser());
   ASSERT_TRUE(app_browser->is_app());
   ASSERT_FALSE(app_browser->is_type_panel());
 #endif
 }