Move AppIsolation out of Extension Class

TBR=ben@chromium.org, joi@chromium.org
(ben - gypis, joi - public/browser/ comment update)
BUG=159265


Review URL: https://chromiumcodereview.appspot.com/12770031

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@191571 0039d316-1c4b-4281-b951-d872f2087c98

13 files changed, 192 insertions, 68 deletions

diff --git a/chrome/browser/chrome_content_browser_client.cc b/chrome/browser/chrome_content_browser_client.cc
index 45b0879..81548c6 100644
--- a/chrome/browser/chrome_content_browser_client.cc
+++ b/chrome/browser/chrome_content_browser_client.cc
@@ -89,6 +89,7 @@
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/extension_process_policy.h"
 #include "chrome/common/extensions/extension_set.h"
+#include "chrome/common/extensions/manifest_handlers/app_isolation_info.h"
 #include "chrome/common/extensions/permissions/socket_permission.h"
 #include "chrome/common/logging_chrome.h"
 #include "chrome/common/pref_names.h"
@@ -323,7 +324,8 @@ RenderProcessHostPrivilege GetPrivilegeRequiredByUrl(
   if (url.SchemeIs(extensions::kExtensionScheme)) {
     const Extension* extension =
         service->extensions()->GetByID(url.host());
-    if (extension && extension->is_storage_isolated())
+    if (extension &&
+        extensions::AppIsolationInfo::HasIsolatedStorage(extension))
       return PRIV_ISOLATED;
     if (extension && extension->is_hosted_app())
       return PRIV_HOSTED;
@@ -346,7 +348,8 @@ RenderProcessHostPrivilege GetProcessPrivilege(
   for (std::set<std::string>::iterator iter = extension_ids.begin();
        iter != extension_ids.end(); ++iter) {
     const Extension* extension = service->GetExtensionById(*iter, false);
-    if (extension && extension->is_storage_isolated())
+    if (extension &&
+        extensions::AppIsolationInfo::HasIsolatedStorage(extension))
       return PRIV_ISOLATED;
     if (extension && extension->is_hosted_app())
       return PRIV_HOSTED;
@@ -597,7 +600,8 @@ void ChromeContentBrowserClient::GetStoragePartitionConfigForSite(
       if (extension_service) {
         extension = extension_service->extensions()->
             GetExtensionOrAppByURL(ExtensionURLInfo(site));
-        if (extension && extension->is_storage_isolated()) {
+        if (extension &&
+            extensions::AppIsolationInfo::HasIsolatedStorage(extension)) {
           is_isolated = true;
         }
       }
diff --git a/chrome/browser/extensions/chrome_manifest_parser.cc b/chrome/browser/extensions/chrome_manifest_parser.cc
index 75c2a4d..94ddd81 100644
--- a/chrome/browser/extensions/chrome_manifest_parser.cc
+++ b/chrome/browser/extensions/chrome_manifest_parser.cc
@@ -8,6 +8,7 @@
 #include "chrome/browser/extensions/extension_web_ui.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_notification_types.h"
+#include "chrome/common/extensions/manifest_handlers/app_isolation_info.h"
 #include "chrome/common/extensions/manifest_handlers/kiosk_enabled_info.h"
 #include "chrome/common/extensions/manifest_handlers/offline_enabled_info.h"
 #include "chrome/common/extensions/manifest_handlers/requirements_handler.h"
@@ -19,6 +20,7 @@ namespace extensions {
 
 ChromeManifestParser::ChromeManifestParser(Profile* profile)
     : profile_(profile) {
+  (new AppIsolationHandler)->Register();
   (new DevToolsPageHandler)->Register();
   (new KioskEnabledHandler)->Register();
   (new HomepageURLHandler)->Register();
diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index e6f28b7..9993a94 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -87,6 +87,7 @@
 #include "chrome/common/extensions/features/feature.h"
 #include "chrome/common/extensions/incognito_handler.h"
 #include "chrome/common/extensions/manifest.h"
+#include "chrome/common/extensions/manifest_handlers/app_isolation_info.h"
 #include "chrome/common/extensions/manifest_url_handler.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/startup_metric_utils.h"
@@ -293,10 +294,11 @@ const Extension* ExtensionService::GetIsolatedAppForRenderer(
 
   const extensions::Extension* extension =
       extensions_.GetByID(*(extension_ids.begin()));
-  // We still need to check is_storage_isolated(),
+  // We still need to check if the extension has isolated storage,
   // because it's common for there to be one extension in a process
-  // with is_storage_isolated() == false.
-  if (extension && extension->is_storage_isolated())
+  // without isolated storage.
+  if (extension &&
+      extensions::AppIsolationInfo::HasIsolatedStorage(extension))
     return extension;
 
   return NULL;
@@ -841,7 +843,8 @@ bool ExtensionService::UninstallExtension(
 
   GURL launch_web_url_origin(extension->launch_web_url());
   launch_web_url_origin = launch_web_url_origin.GetOrigin();
-  bool is_storage_isolated = extension->is_storage_isolated();
+  bool is_storage_isolated =
+      extensions::AppIsolationInfo::HasIsolatedStorage(extension);
 
   if (is_storage_isolated) {
     BrowserContext::AsyncObliterateStoragePartition(
@@ -3034,7 +3037,7 @@ void ExtensionService::GarbageCollectIsolatedStorage() {
       new base::hash_set<base::FilePath>());
   for (ExtensionSet::const_iterator it = extensions_.begin();
        it != extensions_.end(); ++it) {
-    if ((*it)->is_storage_isolated()) {
+    if (extensions::AppIsolationInfo::HasIsolatedStorage(*it)) {
       active_paths->insert(
           BrowserContext::GetStoragePartitionForSite(
               profile_,
diff --git a/chrome/chrome_common.gypi b/chrome/chrome_common.gypi
index 4bfaa77..9008438 100644
--- a/chrome/chrome_common.gypi
+++ b/chrome/chrome_common.gypi
@@ -250,6 +250,8 @@
         'common/extensions/manifest_handler.h',
         'common/extensions/manifest_handler_helpers.cc',
         'common/extensions/manifest_handler_helpers.h',
+        'common/extensions/manifest_handlers/app_isolation_info.cc',
+        'common/extensions/manifest_handlers/app_isolation_info.h',
         'common/extensions/manifest_handlers/content_scripts_handler.cc',
         'common/extensions/manifest_handlers/content_scripts_handler.h',
         'common/extensions/manifest_handlers/kiosk_enabled_info.cc',
diff --git a/chrome/common/extensions/extension.cc b/chrome/common/extensions/extension.cc
index 964159e..6d6bace 100644
--- a/chrome/common/extensions/extension.cc
+++ b/chrome/common/extensions/extension.cc
@@ -1124,7 +1124,6 @@ Extension::Extension(const base::FilePath& path,
       converted_from_user_script_(false),
       manifest_(manifest.release()),
       finished_parsing_manifest_(false),
-      is_storage_isolated_(false),
       launch_container_(extension_misc::LAUNCH_TAB),
       launch_width_(0),
       launch_height_(0),
@@ -1217,9 +1216,6 @@ bool Extension::InitFromValue(int flags, string16* error) {
     return false;
   }
 
-  if (!LoadAppIsolation(error))
-    return false;
-
   if (!LoadSharedFeatures(error))
     return false;
 
@@ -1245,48 +1241,6 @@ bool Extension::InitFromValue(int flags, string16* error) {
   return true;
 }
 
-bool Extension::LoadAppIsolation(string16* error) {
-  // Platform apps always get isolated storage.
-  if (is_platform_app()) {
-    is_storage_isolated_ = true;
-    return true;
-  }
-
-  // Other apps only get it if it is requested _and_ experimental APIs are
-  // enabled.
-  if (!initial_api_permissions()->count(APIPermission::kExperimental)
-      || !is_app())
-    return true;
-
-  const Value* tmp_isolation = NULL;
-  if (!manifest_->Get(keys::kIsolation, &tmp_isolation))
-    return true;
-
-  const ListValue* isolation_list = NULL;
-  if (!tmp_isolation->GetAsList(&isolation_list)) {
-    *error = ASCIIToUTF16(errors::kInvalidIsolation);
-    return false;
-  }
-
-  for (size_t i = 0; i < isolation_list->GetSize(); ++i) {
-    std::string isolation_string;
-    if (!isolation_list->GetString(i, &isolation_string)) {
-      *error = ErrorUtils::FormatErrorMessageUTF16(
-          errors::kInvalidIsolationValue,
-          base::UintToString(i));
-      return false;
-    }
-
-    // Check for isolated storage.
-    if (isolation_string == values::kIsolatedStorage) {
-      is_storage_isolated_ = true;
-    } else {
-      DLOG(WARNING) << "Did not recognize isolation type: " << isolation_string;
-    }
-  }
-  return true;
-}
-
 bool Extension::LoadRequiredFeatures(string16* error) {
   if (!LoadName(error) ||
       !LoadVersion(error))
diff --git a/chrome/common/extensions/extension.h b/chrome/common/extensions/extension.h
index 07df1a2..e1d0ae9 100644
--- a/chrome/common/extensions/extension.h
+++ b/chrome/common/extensions/extension.h
@@ -453,7 +453,6 @@ class Extension : public base::RefCountedThreadSafe<Extension> {
   bool is_hosted_app() const;
   bool is_legacy_packaged_app() const;
   bool is_extension() const;
-  bool is_storage_isolated() const { return is_storage_isolated_; }
   bool can_be_incognito_enabled() const;
   void AddWebExtentPattern(const URLPattern& pattern);
   const URLPatternSet& web_extent() const { return extent_; }
@@ -643,9 +642,6 @@ class Extension : public base::RefCountedThreadSafe<Extension> {
   // parts of the initialization process need information from previous parts).
   base::ThreadChecker thread_checker_;
 
-  // Whether this extension requests isolated storage.
-  bool is_storage_isolated_;
-
   // The local path inside the extension to use with the launcher.
   std::string launch_local_path_;
 
diff --git a/chrome/common/extensions/extension_process_policy.cc b/chrome/common/extensions/extension_process_policy.cc
index 00256c49..b38f397 100644
--- a/chrome/common/extensions/extension_process_policy.cc
+++ b/chrome/common/extensions/extension_process_policy.cc
@@ -4,7 +4,9 @@
 
 #include "chrome/common/extensions/extension_process_policy.h"
 
+#include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/extension_set.h"
+#include "chrome/common/extensions/manifest_handlers/app_isolation_info.h"
 
 namespace extensions {
 
@@ -41,10 +43,10 @@ bool CrossesExtensionProcessBoundary(
   if (should_consider_workaround) {
     bool old_url_is_hosted_app = old_url_extension &&
         !old_url_extension->web_extent().is_empty() &&
-        !old_url_extension->is_storage_isolated();
+        !AppIsolationInfo::HasIsolatedStorage(old_url_extension);
     bool new_url_is_normal_or_hosted = !new_url_extension ||
         (!new_url_extension->web_extent().is_empty() &&
-        !new_url_extension->is_storage_isolated());
+         !AppIsolationInfo::HasIsolatedStorage(new_url_extension));
     bool either_is_web_store =
         (old_url_extension &&
         old_url_extension->id() == extension_misc::kWebStoreAppId) ||
diff --git a/chrome/common/extensions/manifest_handlers/app_isolation_info.cc b/chrome/common/extensions/manifest_handlers/app_isolation_info.cc
new file mode 100644
index 0000000..7f7e121
--- /dev/null
+++ b/chrome/common/extensions/manifest_handlers/app_isolation_info.cc
@@ -0,0 +1,98 @@
+// Copyright (c) 2013 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_handlers/app_isolation_info.h"
+
+#include "base/memory/scoped_ptr.h"
+#include "base/string16.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/utf_string_conversions.h"
+#include "base/values.h"
+#include "chrome/common/extensions/extension_manifest_constants.h"
+#include "chrome/common/extensions/permissions/api_permission_set.h"
+#include "extensions/common/error_utils.h"
+
+namespace keys = extension_manifest_keys;
+
+namespace extensions {
+
+AppIsolationInfo::AppIsolationInfo(bool isolated_storage)
+    : has_isolated_storage(isolated_storage) {
+}
+
+AppIsolationInfo::~AppIsolationInfo() {
+}
+
+// static
+bool AppIsolationInfo::HasIsolatedStorage(const Extension* extension) {
+  AppIsolationInfo* info = static_cast<AppIsolationInfo*>(
+      extension->GetManifestData(keys::kIsolation));
+  return info ? info->has_isolated_storage : false;
+}
+
+AppIsolationHandler::AppIsolationHandler() {
+}
+
+AppIsolationHandler::~AppIsolationHandler() {
+}
+
+bool AppIsolationHandler::Parse(Extension* extension, string16* error) {
+  // Platform apps always get isolated storage.
+  if (extension->is_platform_app()) {
+    extension->SetManifestData(keys::kIsolation, new AppIsolationInfo(true));
+    return true;
+  }
+
+  // Other apps only get it if it is requested _and_ experimental APIs are
+  // enabled.
+  if (!extension->is_app() ||
+      !extension->initial_api_permissions()->count(
+          APIPermission::kExperimental)) {
+    return true;
+  }
+
+  const Value* tmp_isolation = NULL;
+  // We should only be parsing if the extension has the key in the manifest,
+  // or is a platform app (which we already handled).
+  DCHECK(extension->manifest()->Get(keys::kIsolation, &tmp_isolation));
+
+  const ListValue* isolation_list = NULL;
+  if (!tmp_isolation->GetAsList(&isolation_list)) {
+    *error = ASCIIToUTF16(extension_manifest_errors::kInvalidIsolation);
+    return false;
+  }
+
+  bool has_isolated_storage = false;
+  for (size_t i = 0; i < isolation_list->GetSize(); ++i) {
+    std::string isolation_string;
+    if (!isolation_list->GetString(i, &isolation_string)) {
+      *error = ErrorUtils::FormatErrorMessageUTF16(
+          extension_manifest_errors::kInvalidIsolationValue,
+          base::UintToString(i));
+      return false;
+    }
+
+    // Check for isolated storage.
+    if (isolation_string == extension_manifest_values::kIsolatedStorage) {
+      has_isolated_storage = true;
+    } else {
+      DLOG(WARNING) << "Did not recognize isolation type: " << isolation_string;
+    }
+  }
+
+  if (has_isolated_storage)
+    extension->SetManifestData(keys::kIsolation, new AppIsolationInfo(true));
+
+  return true;
+}
+
+bool AppIsolationHandler::AlwaysParseForType(Manifest::Type type) const {
+  return type == Manifest::TYPE_PLATFORM_APP;
+}
+
+const std::vector<std::string> AppIsolationHandler::Keys() const {
+  return SingleKey(keys::kIsolation);
+}
+
+}  // namespace extensions
diff --git a/chrome/common/extensions/manifest_handlers/app_isolation_info.h b/chrome/common/extensions/manifest_handlers/app_isolation_info.h
new file mode 100644
index 0000000..7ceb94e
--- /dev/null
+++ b/chrome/common/extensions/manifest_handlers/app_isolation_info.h
@@ -0,0 +1,44 @@
+// Copyright (c) 2013 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.
+
+#ifndef CHROME_COMMON_EXTENSIONS_MANIFEST_HANDLERS_APP_ISOLATION_INFO_H_
+#define CHROME_COMMON_EXTENSIONS_MANIFEST_HANDLERS_APP_ISOLATION_INFO_H_
+
+#include <string>
+#include <vector>
+
+#include "chrome/common/extensions/extension.h"
+#include "chrome/common/extensions/manifest.h"
+#include "chrome/common/extensions/manifest_handler.h"
+
+namespace extensions {
+
+struct AppIsolationInfo : public Extension::ManifestData {
+  explicit AppIsolationInfo(bool isolated_storage);
+  virtual ~AppIsolationInfo();
+
+  static bool HasIsolatedStorage(const Extension* extension);
+
+  // Whether this extension requests isolated storage.
+  bool has_isolated_storage;
+};
+
+// Parses the "isolation" manifest key.
+class AppIsolationHandler : public ManifestHandler {
+ public:
+  AppIsolationHandler();
+  virtual ~AppIsolationHandler();
+
+  virtual bool Parse(Extension* extension, string16* error) OVERRIDE;
+  virtual bool AlwaysParseForType(Manifest::Type type) const OVERRIDE;
+
+ private:
+  virtual const std::vector<std::string> Keys() const OVERRIDE;
+
+  DISALLOW_COPY_AND_ASSIGN(AppIsolationHandler);
+};
+
+}  // namespace extensions
+
+#endif  // CHROME_COMMON_EXTENSIONS_MANIFEST_HANDLERS_APP_ISOLATION_INFO_H_
diff --git a/chrome/common/extensions/manifest_tests/extension_manifests_isolatedapp_unittest.cc b/chrome/common/extensions/manifest_tests/extension_manifests_isolatedapp_unittest.cc
index 6181ad0..80bd8bb 100644
--- a/chrome/common/extensions/manifest_tests/extension_manifests_isolatedapp_unittest.cc
+++ b/chrome/common/extensions/manifest_tests/extension_manifests_isolatedapp_unittest.cc
@@ -8,18 +8,31 @@
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/extension_manifest_constants.h"
+#include "chrome/common/extensions/manifest_handlers/app_isolation_info.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace errors = extension_manifest_errors;
 
-TEST_F(ExtensionManifestTest, IsolatedApps) {
+namespace extensions {
+
+class IsolatedAppsManifestTest : public ExtensionManifestTest {
+ protected:
+  virtual void SetUp() OVERRIDE {
+    testing::Test::SetUp();
+    (new AppIsolationHandler)->Register();
+  }
+};
+
+TEST_F(IsolatedAppsManifestTest, IsolatedApps) {
   // Requires --enable-experimental-extension-apis
   LoadAndExpectError("isolated_app_valid.json",
                      errors::kExperimentalFlagRequired);
 
   CommandLine::ForCurrentProcess()->AppendSwitch(
       switches::kEnableExperimentalExtensionApis);
-  scoped_refptr<extensions::Extension> extension2(
+  scoped_refptr<Extension> extension2(
       LoadAndExpectSuccess("isolated_app_valid.json"));
-  EXPECT_TRUE(extension2->is_storage_isolated());
+  EXPECT_TRUE(AppIsolationInfo::HasIsolatedStorage(extension2));
 }
+
+}  // namespace extensions
diff --git a/chrome/common/extensions/manifest_tests/extension_manifests_platformapp_unittest.cc b/chrome/common/extensions/manifest_tests/extension_manifests_platformapp_unittest.cc
index 7dbb9d0..1fdab49 100644
--- a/chrome/common/extensions/manifest_tests/extension_manifests_platformapp_unittest.cc
+++ b/chrome/common/extensions/manifest_tests/extension_manifests_platformapp_unittest.cc
@@ -10,6 +10,7 @@
 #include "chrome/common/extensions/csp_handler.h"
 #include "chrome/common/extensions/extension_manifest_constants.h"
 #include "chrome/common/extensions/incognito_handler.h"
+#include "chrome/common/extensions/manifest_handlers/app_isolation_info.h"
 #include "chrome/common/extensions/manifest_tests/extension_manifest_test.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -18,17 +19,20 @@ namespace errors = extension_manifest_errors;
 namespace extensions {
 
 class PlatformAppsManifestTest : public ExtensionManifestTest {
+ protected:
   virtual void SetUp() OVERRIDE {
+    testing::Test::SetUp();
     (new BackgroundManifestHandler)->Register();
     (new CSPHandler(true))->Register();  // platform app.
-    (new IncognitoHandler())->Register();
+    (new IncognitoHandler)->Register();
+    (new AppIsolationHandler)->Register();
   }
 };
 
 TEST_F(PlatformAppsManifestTest, PlatformApps) {
-  scoped_refptr<extensions::Extension> extension =
+  scoped_refptr<Extension> extension =
       LoadAndExpectSuccess("init_valid_platform_app.json");
-  EXPECT_TRUE(extension->is_storage_isolated());
+  EXPECT_TRUE(AppIsolationInfo::HasIsolatedStorage(extension));
   EXPECT_TRUE(IncognitoInfo::IsSplitMode(extension));
 
   extension =
diff --git a/chrome/renderer/chrome_content_renderer_client.cc b/chrome/renderer/chrome_content_renderer_client.cc
index 27fd285..2bfee18 100644
--- a/chrome/renderer/chrome_content_renderer_client.cc
+++ b/chrome/renderer/chrome_content_renderer_client.cc
@@ -28,6 +28,7 @@
 #include "chrome/common/extensions/extension_set.h"
 #include "chrome/common/extensions/incognito_handler.h"
 #include "chrome/common/extensions/manifest_handler.h"
+#include "chrome/common/extensions/manifest_handlers/app_isolation_info.h"
 #include "chrome/common/extensions/manifest_handlers/sandboxed_page_info.h"
 #include "chrome/common/extensions/manifest_url_handler.h"
 #include "chrome/common/extensions/web_accessible_resources_handler.h"
@@ -140,6 +141,7 @@ const char kAdViewTagName[] = "ADVIEW";
 // Explicitly register all extension ManifestHandlers needed to parse
 // fields used in the renderer.
 void RegisterExtensionManifestHandlers() {
+  (new extensions::AppIsolationHandler)->Register();
   (new extensions::BackgroundManifestHandler)->Register();
   (new extensions::CSPHandler(false))->Register();  // not platform app.
   (new extensions::CSPHandler(true))->Register();  // platform app.
diff --git a/content/public/browser/browser_context.h b/content/public/browser/browser_context.h
index 9567b1a..b6ba322 100644
--- a/content/public/browser/browser_context.h
+++ b/content/public/browser/browser_context.h
@@ -117,8 +117,8 @@ class CONTENT_EXPORT BrowserContext : public base::SupportsUserData {
 
   // Returns the request context appropriate for the given renderer. If the
   // renderer process doesn't have an associated installed app, or if the
-  // installed app's is_storage_isolated() returns false, this is equivalent to
-  // calling GetRequestContext().
+  // installed app doesn't have isolated storage, this is equivalent to calling
+  // GetRequestContext().
   virtual net::URLRequestContextGetter* GetRequestContextForRenderProcess(
       int renderer_child_id) = 0;