Include the mis-matching context along with the contexts a Feature is available

in within the message result of Feature::GetAvailabilityMessage.

BUG=b/9335685
R=mpcomplete@chromium.org

Review URL: https://codereview.chromium.org/27272002

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

8 files changed, 132 insertions, 58 deletions

diff --git a/chrome/common/extensions/api/identity/extension_manifests_auth_unittest.cc b/chrome/common/extensions/api/identity/extension_manifests_auth_unittest.cc
index 997ff6d..1d327f9 100644
--- a/chrome/common/extensions/api/identity/extension_manifests_auth_unittest.cc
+++ b/chrome/common/extensions/api/identity/extension_manifests_auth_unittest.cc
@@ -147,8 +147,8 @@ TEST_F(OAuth2ManifestTest, OAuth2SectionParsing) {
     EXPECT_EQ(1U, extension->install_warnings().size());
     const extensions::InstallWarning& warning =
         extension->install_warnings()[0];
-    EXPECT_EQ("'oauth2' is only allowed for extensions, legacy packaged apps "
-                  "and packaged apps, and this is a hosted app.",
+    EXPECT_EQ("'oauth2' is only allowed for extensions, legacy packaged apps, "
+                  "and packaged apps, but this is a hosted app.",
               warning.message);
     EXPECT_EQ("", OAuth2Info::GetOAuth2Info(extension.get()).client_id);
     EXPECT_TRUE(OAuth2Info::GetOAuth2Info(extension.get()).scopes.empty());
diff --git a/chrome/common/extensions/features/complex_feature.cc b/chrome/common/extensions/features/complex_feature.cc
index 57c9ad4..5b11019 100644
--- a/chrome/common/extensions/features/complex_feature.cc
+++ b/chrome/common/extensions/features/complex_feature.cc
@@ -72,13 +72,14 @@ bool ComplexFeature::IsInternal() const {
 
 std::string ComplexFeature::GetAvailabilityMessage(AvailabilityResult result,
                                                    Manifest::Type type,
-                                                   const GURL& url) const {
+                                                   const GURL& url,
+                                                   Context context) const {
   if (result == IS_AVAILABLE)
     return std::string();
 
   // TODO(justinlin): Form some kind of combined availabilities/messages from
   // SimpleFeatures.
-  return features_[0]->GetAvailabilityMessage(result, type, url);
+  return features_[0]->GetAvailabilityMessage(result, type, url, context);
 }
 
 bool ComplexFeature::IsIdInWhitelist(const std::string& extension_id) const {
diff --git a/chrome/common/extensions/features/complex_feature.h b/chrome/common/extensions/features/complex_feature.h
index 3cfa451..45cf6a2 100644
--- a/chrome/common/extensions/features/complex_feature.h
+++ b/chrome/common/extensions/features/complex_feature.h
@@ -43,7 +43,8 @@ class ComplexFeature : public Feature {
   virtual std::string GetAvailabilityMessage(
       AvailabilityResult result,
       Manifest::Type type,
-      const GURL& url) const OVERRIDE;
+      const GURL& url,
+      Context context) const OVERRIDE;
 
   virtual std::set<Context>* GetContexts() OVERRIDE;
 
diff --git a/chrome/common/extensions/features/simple_feature.cc b/chrome/common/extensions/features/simple_feature.cc
index 4f71552..694b90d 100644
--- a/chrome/common/extensions/features/simple_feature.cc
+++ b/chrome/common/extensions/features/simple_feature.cc
@@ -154,8 +154,9 @@ void ParseURLPatterns(const base::DictionaryValue* value,
   }
 }
 
-// Gets a human-readable name for the given extension type.
-std::string GetDisplayTypeName(Manifest::Type type) {
+// Gets a human-readable name for the given extension type, suitable for giving
+// to developers in an error message.
+std::string GetDisplayName(Manifest::Type type) {
   switch (type) {
     case Manifest::TYPE_UNKNOWN:
       return "unknown";
@@ -174,9 +175,53 @@ std::string GetDisplayTypeName(Manifest::Type type) {
     case Manifest::TYPE_SHARED_MODULE:
       return "shared module";
   }
+  NOTREACHED();
+  return "";
+}
 
+// Gets a human-readable name for the given context type, suitable for giving
+// to developers in an error message.
+std::string GetDisplayName(Feature::Context context) {
+  switch (context) {
+    case Feature::UNSPECIFIED_CONTEXT:
+      return "unknown";
+    case Feature::BLESSED_EXTENSION_CONTEXT:
+      // "privileged" is vague but hopefully the developer will understand that
+      // means background or app window.
+      return "privileged page";
+    case Feature::UNBLESSED_EXTENSION_CONTEXT:
+      // "iframe" is a bit of a lie/oversimplification, but that's the most
+      // common unblessed context.
+      return "extension iframe";
+    case Feature::CONTENT_SCRIPT_CONTEXT:
+      return "content script";
+    case Feature::WEB_PAGE_CONTEXT:
+      return "web page";
+  }
   NOTREACHED();
-  return std::string();
+  return "";
+}
+
+// Gets a human-readable list of the display names (pluralized, comma separated
+// with the "and" in the correct place) for each of |enum_types|.
+template <typename EnumType>
+std::string ListDisplayNames(const std::vector<EnumType> enum_types) {
+  std::string display_name_list;
+  for (size_t i = 0; i < enum_types.size(); ++i) {
+    // Pluralize type name.
+    display_name_list += GetDisplayName(enum_types[i]) + "s";
+    // Comma-separate entries, with an Oxford comma if there is more than 2
+    // total entries.
+    if (enum_types.size() > 2) {
+      if (i < enum_types.size() - 2)
+        display_name_list += ", ";
+      else if (i == enum_types.size() - 2)
+        display_name_list += ", and ";
+    } else if (enum_types.size() == 2 && i == 0) {
+      display_name_list += " and ";
+    }
+  }
+  return display_name_list;
 }
 
 std::string HashExtensionId(const std::string& extension_id) {
@@ -332,11 +377,8 @@ Feature::Availability SimpleFeature::IsAvailableToContext(
       return result;
   }
 
-  if (!contexts_.empty() && contexts_.find(context) == contexts_.end()) {
-    return extension ?
-        CreateAvailability(INVALID_CONTEXT, extension->GetType()) :
-        CreateAvailability(INVALID_CONTEXT);
-  }
+  if (!contexts_.empty() && contexts_.find(context) == contexts_.end())
+    return CreateAvailability(INVALID_CONTEXT, context);
 
   if (!matches_.is_empty() && !matches_.MatchesURL(url))
     return CreateAvailability(INVALID_URL, url);
@@ -345,7 +387,10 @@ Feature::Availability SimpleFeature::IsAvailableToContext(
 }
 
 std::string SimpleFeature::GetAvailabilityMessage(
-    AvailabilityResult result, Manifest::Type type, const GURL& url) const {
+    AvailabilityResult result,
+    Manifest::Type type,
+    const GURL& url,
+    Context context) const {
   switch (result) {
     case IS_AVAILABLE:
       return std::string();
@@ -356,33 +401,20 @@ std::string SimpleFeature::GetAvailabilityMessage(
     case INVALID_URL:
       return base::StringPrintf("'%s' is not allowed on %s.",
                                 name().c_str(), url.spec().c_str());
-    case INVALID_TYPE: {
-      std::string allowed_type_names;
-      // Turn the set of allowed types into a vector so that it's easier to
-      // inject the appropriate separator into the display string.
-      std::vector<Manifest::Type> extension_types(
-          extension_types_.begin(), extension_types_.end());
-      for (size_t i = 0; i < extension_types.size(); i++) {
-        // Pluralize type name.
-        allowed_type_names += GetDisplayTypeName(extension_types[i]) + "s";
-        if (i == extension_types_.size() - 2) {
-          allowed_type_names += " and ";
-        } else if (i != extension_types_.size() - 1) {
-          allowed_type_names += ", ";
-        }
-      }
-
+    case INVALID_TYPE:
       return base::StringPrintf(
-          "'%s' is only allowed for %s, and this is a %s.",
+          "'%s' is only allowed for %s, but this is a %s.",
           name().c_str(),
-          allowed_type_names.c_str(),
-          GetDisplayTypeName(type).c_str());
-    }
+          ListDisplayNames(std::vector<Manifest::Type>(
+              extension_types_.begin(), extension_types_.end())).c_str(),
+          GetDisplayName(type).c_str());
     case INVALID_CONTEXT:
       return base::StringPrintf(
-          "'%s' is not allowed for specified context type content script, "
-          " extension page, web page, etc.).",
-          name().c_str());
+          "'%s' is only allowed to run in %s, but this is a %s",
+          name().c_str(),
+          ListDisplayNames(std::vector<Context>(
+              contexts_.begin(), contexts_.end())).c_str(),
+          GetDisplayName(context).c_str());
     case INVALID_LOCATION:
       return base::StringPrintf(
           "'%s' is not allowed for specified install location.",
@@ -407,7 +439,7 @@ std::string SimpleFeature::GetAvailabilityMessage(
           name().c_str());
     case UNSUPPORTED_CHANNEL:
       return base::StringPrintf(
-          "'%s' requires Google Chrome %s channel or newer, and this is the "
+          "'%s' requires Google Chrome %s channel or newer, but this is the "
               "%s channel.",
           name().c_str(),
           GetChannelName(channel_).c_str(),
@@ -421,19 +453,30 @@ std::string SimpleFeature::GetAvailabilityMessage(
 Feature::Availability SimpleFeature::CreateAvailability(
     AvailabilityResult result) const {
   return Availability(
-      result, GetAvailabilityMessage(result, Manifest::TYPE_UNKNOWN, GURL()));
+      result, GetAvailabilityMessage(result, Manifest::TYPE_UNKNOWN, GURL(),
+                                     UNSPECIFIED_CONTEXT));
 }
 
 Feature::Availability SimpleFeature::CreateAvailability(
     AvailabilityResult result, Manifest::Type type) const {
-  return Availability(result, GetAvailabilityMessage(result, type, GURL()));
+  return Availability(result, GetAvailabilityMessage(result, type, GURL(),
+                                                     UNSPECIFIED_CONTEXT));
 }
 
 Feature::Availability SimpleFeature::CreateAvailability(
     AvailabilityResult result,
     const GURL& url) const {
   return Availability(
-      result, GetAvailabilityMessage(result, Manifest::TYPE_UNKNOWN, url));
+      result, GetAvailabilityMessage(result, Manifest::TYPE_UNKNOWN, url,
+                                     UNSPECIFIED_CONTEXT));
+}
+
+Feature::Availability SimpleFeature::CreateAvailability(
+    AvailabilityResult result,
+    Context context) const {
+  return Availability(
+      result, GetAvailabilityMessage(result, Manifest::TYPE_UNKNOWN, GURL(),
+                                     context));
 }
 
 std::set<Feature::Context>* SimpleFeature::GetContexts() {
diff --git a/chrome/common/extensions/features/simple_feature.h b/chrome/common/extensions/features/simple_feature.h
index 9013e18..aa6be16 100644
--- a/chrome/common/extensions/features/simple_feature.h
+++ b/chrome/common/extensions/features/simple_feature.h
@@ -81,7 +81,8 @@ class SimpleFeature : public Feature {
 
   virtual std::string GetAvailabilityMessage(AvailabilityResult result,
                                              Manifest::Type type,
-                                             const GURL& url) const OVERRIDE;
+                                             const GURL& url,
+                                             Context context) const OVERRIDE;
 
   virtual std::set<Context>* GetContexts() OVERRIDE;
 
@@ -97,6 +98,8 @@ class SimpleFeature : public Feature {
                                   Manifest::Type type) const;
   Availability CreateAvailability(AvailabilityResult result,
                                   const GURL& url) const;
+  Availability CreateAvailability(AvailabilityResult result,
+                                  Context context) const;
 
  private:
   // For clarity and consistency, we handle the default value of each of these
diff --git a/chrome/common/extensions/features/simple_feature_unittest.cc b/chrome/common/extensions/features/simple_feature_unittest.cc
index 03145a5..d4f4c0a 100644
--- a/chrome/common/extensions/features/simple_feature_unittest.cc
+++ b/chrome/common/extensions/features/simple_feature_unittest.cc
@@ -172,6 +172,7 @@ TEST_F(ExtensionSimpleFeatureTest, PackageType) {
 
 TEST_F(ExtensionSimpleFeatureTest, Context) {
   SimpleFeature feature;
+  feature.set_name("somefeature");
   feature.GetContexts()->insert(Feature::BLESSED_EXTENSION_CONTEXT);
   feature.extension_types()->insert(Manifest::TYPE_LEGACY_PACKAGED_APP);
   feature.platforms()->insert(Feature::CHROMEOS_PLATFORM);
@@ -199,20 +200,44 @@ TEST_F(ExtensionSimpleFeatureTest, Context) {
 
   feature.extension_types()->clear();
   feature.extension_types()->insert(Manifest::TYPE_THEME);
-  EXPECT_EQ(Feature::INVALID_TYPE, feature.IsAvailableToContext(
-      extension.get(), Feature::BLESSED_EXTENSION_CONTEXT,
-      Feature::CHROMEOS_PLATFORM).result());
+  {
+    Feature::Availability availability = feature.IsAvailableToContext(
+        extension.get(), Feature::BLESSED_EXTENSION_CONTEXT,
+        Feature::CHROMEOS_PLATFORM);
+    EXPECT_EQ(Feature::INVALID_TYPE, availability.result());
+    EXPECT_EQ("'somefeature' is only allowed for themes, "
+              "but this is a legacy packaged app.",
+              availability.message());
+  }
+
   feature.extension_types()->clear();
   feature.extension_types()->insert(Manifest::TYPE_LEGACY_PACKAGED_APP);
-
   feature.GetContexts()->clear();
   feature.GetContexts()->insert(Feature::UNBLESSED_EXTENSION_CONTEXT);
-  EXPECT_EQ(Feature::INVALID_CONTEXT, feature.IsAvailableToContext(
-      extension.get(), Feature::BLESSED_EXTENSION_CONTEXT,
-      Feature::CHROMEOS_PLATFORM).result());
+  feature.GetContexts()->insert(Feature::CONTENT_SCRIPT_CONTEXT);
+  {
+    Feature::Availability availability = feature.IsAvailableToContext(
+        extension.get(), Feature::BLESSED_EXTENSION_CONTEXT,
+        Feature::CHROMEOS_PLATFORM);
+    EXPECT_EQ(Feature::INVALID_CONTEXT, availability.result());
+    EXPECT_EQ("'somefeature' is only allowed to run in extension iframes and "
+              "content scripts, but this is a privileged page",
+              availability.message());
+  }
+
+  feature.GetContexts()->insert(Feature::WEB_PAGE_CONTEXT);
+  {
+    Feature::Availability availability = feature.IsAvailableToContext(
+        extension.get(), Feature::BLESSED_EXTENSION_CONTEXT,
+        Feature::CHROMEOS_PLATFORM);
+    EXPECT_EQ(Feature::INVALID_CONTEXT, availability.result());
+    EXPECT_EQ("'somefeature' is only allowed to run in extension iframes, "
+              "content scripts, and web pages, but this is a privileged page",
+              availability.message());
+  }
+
   feature.GetContexts()->clear();
   feature.GetContexts()->insert(Feature::BLESSED_EXTENSION_CONTEXT);
-
   feature.set_location(Feature::COMPONENT_LOCATION);
   EXPECT_EQ(Feature::INVALID_LOCATION, feature.IsAvailableToContext(
       extension.get(), Feature::BLESSED_EXTENSION_CONTEXT,
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 2c364ab..8af8e89 100644
--- a/chrome/common/extensions/manifest_tests/extension_manifests_platformapp_unittest.cc
+++ b/chrome/common/extensions/manifest_tests/extension_manifests_platformapp_unittest.cc
@@ -48,18 +48,18 @@ TEST_F(PlatformAppsManifestTest, PlatformApps) {
     Testcase(
         "init_invalid_platform_app_1.json",
         "'app.launch' is only allowed for hosted apps and legacy packaged "
-            "apps, and this is a packaged app."),
+            "apps, but this is a packaged app."),
     Testcase(
         "init_invalid_platform_app_4.json",
-        "'background' is only allowed for extensions, hosted apps and legacy "
-            "packaged apps, and this is a packaged app."),
+        "'background' is only allowed for extensions, hosted apps, and legacy "
+            "packaged apps, but this is a packaged app."),
     Testcase(
         "init_invalid_platform_app_5.json",
-        "'background' is only allowed for extensions, hosted apps and legacy "
-            "packaged apps, and this is a packaged app."),
+        "'background' is only allowed for extensions, hosted apps, and legacy "
+            "packaged apps, but this is a packaged app."),
     Testcase("incognito_invalid_platform_app.json",
         "'incognito' is only allowed for extensions and legacy packaged apps, "
-            "and this is a packaged app."),
+            "but this is a packaged app."),
   };
   RunTestcases(
       warning_testcases, arraysize(warning_testcases), EXPECT_TYPE_WARNING);
@@ -71,7 +71,7 @@ TEST_F(PlatformAppsManifestTest, PlatformAppContentSecurityPolicy) {
     Testcase(
         "init_platform_app_csp_warning_1.json",
         "'content_security_policy' is only allowed for extensions and legacy "
-            "packaged apps, and this is a packaged app."),
+            "packaged apps, but this is a packaged app."),
     Testcase(
         "init_platform_app_csp_warning_2.json",
         "'app.content_security_policy' is not allowed for specified extension "
diff --git a/extensions/common/features/feature.h b/extensions/common/features/feature.h
index 0ffd91d..0caebf6 100644
--- a/extensions/common/features/feature.h
+++ b/extensions/common/features/feature.h
@@ -144,7 +144,8 @@ class Feature {
 
   virtual std::string GetAvailabilityMessage(AvailabilityResult result,
                                              Manifest::Type type,
-                                             const GURL& url) const = 0;
+                                             const GURL& url,
+                                             Context context) const = 0;
 
   virtual bool IsIdInWhitelist(const std::string& extension_id) const = 0;