Move the parsing of app.launch related keys out of Extension class.

chrome/common/extensions/manifest_tests/extension_manifests_validapp_unittest.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 "chrome/common/extensions/manifest_tests/extension_manifest_test.h"
-
 #include "base/values.h"
 #include "base/memory/scoped_ptr.h"
+#include "chrome/common/extensions/app_launch_manifest_handler.h"
+#include "chrome/common/extensions/extension_manifest_constants.h"
+#include "chrome/common/extensions/manifest_handler.h"
+#include "chrome/common/extensions/manifest_tests/extension_manifest_test.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-TEST_F(ExtensionManifestTest, ValidApp) {
+class ValidAppManifestTest : public ExtensionManifestTest {
+  virtual void SetUp() OVERRIDE {

[Devlin, 2013/02/27 19:12:48]

while a private: level is default with classes, we still usually declare a
level.

[Joe Thomas, 2013/03/01 23:26:57]

Done.

+    ExtensionManifestTest::SetUp();
+    std::vector<std::string> app_launch_keys(
+        extensions::AppLaunchManifestHandler::keys());
+    linked_ptr<extensions::AppLaunchManifestHandler> app_launch_handler(
+        new extensions::AppLaunchManifestHandler);
+    for (size_t i = 0; i < app_launch_keys.size(); ++i)

[Devlin, 2013/02/27 19:12:48]

Just out of curiosity, is there a reason we don't allow the handlers themselves
to register, when they have multiple keys like this?  e.g.

class AppLaunchManifestHandler {
 public:
  static RegisterHandlerForKeys();
};

AppLaunchManifestHandler::RegisterHandlerForKeys() {
  static const char* keys[] = {'key1', 'key2', 'key3'};
  linked_ptr<AppLaunchManifestHandler> handler(
      new AppLaunchManifestHandler);
  for (size_t i = 0; i < arraysize(keys); ++i)
    extensions::ManifestHandler::Register(keys[i], handler);
}

class ValidAppManifestTest : public ExtensionManifestTest {
 protected:
  virtual void SetUp() OVERRIDE {
    ExtensionManifestTest::SetUp();
    extensions::AppLaunchManifestHandler::RegisterHandlerForKeys();
  }
};

It seems a bit cleaner to me, and reduces a lot of the duplicate code in these
tests' SetUp() functions.

Yoyo, thoughts?

[Yoyo Zhou, 2013/02/27 19:40:32]

It's in the works and should land today:
https://chromiumcodereview.appspot.com/12253022/

=)

+      extensions::ManifestHandler::Register(app_launch_keys[i],
+                                            app_launch_handler);
+  }
+};
+
+TEST_F(ValidAppManifestTest, ValidApp) {
   scoped_refptr<extensions::Extension> extension(
       LoadAndExpectSuccess("valid_app.json"));
   extensions::URLPatternSet expected_patterns;
   AddPattern(&expected_patterns, "http://www.google.com/mail/*");
   AddPattern(&expected_patterns, "http://www.google.com/foobar/*");
   EXPECT_EQ(expected_patterns, extension->web_extent());
-  EXPECT_EQ(extension_misc::LAUNCH_TAB, extension->launch_container());
-  EXPECT_EQ("http://www.google.com/mail/", extension->launch_web_url());
+  EXPECT_EQ(extension_misc::LAUNCH_TAB,
+            extensions::AppLaunchInfo::GetLaunchContainer(extension));
+  EXPECT_EQ("http://www.google.com/mail/",
+            extensions::AppLaunchInfo::GetLaunchWebUrl(extension));
 }
 
-TEST_F(ExtensionManifestTest, AllowUnrecognizedPermissions) {
+TEST_F(ValidAppManifestTest, AllowUnrecognizedPermissions) {
   std::string error;
   scoped_ptr<DictionaryValue> manifest(LoadManifest("valid_app.json", &error));
   ListValue* permissions = NULL;
   ASSERT_TRUE(manifest->GetList("permissions", &permissions));
   permissions->Append(new StringValue("not-a-valid-permission"));
   LoadAndExpectSuccess(Manifest(manifest.get(), ""));
 }