manifest: add theme_color value to the manifest

content/renderer/manifest/manifest_parser_unittest.cc

Index: content/renderer/manifest/manifest_parser_unittest.cc
diff --git a/content/renderer/manifest/manifest_parser_unittest.cc b/content/renderer/manifest/manifest_parser_unittest.cc
index 39384f8be7598d7b36e2f375f1d4d458af4ac02a..00ac9acfea5beb414fc902e3df35fda16a0f3a01 100644
--- a/content/renderer/manifest/manifest_parser_unittest.cc
+++ b/content/renderer/manifest/manifest_parser_unittest.cc
@@ -78,6 +78,7 @@ TEST_F(ManifestParserTest, EmptyStringNull) {
   ASSERT_TRUE(manifest.start_url.is_empty());
   ASSERT_EQ(manifest.display, Manifest::DISPLAY_MODE_UNSPECIFIED);
   ASSERT_EQ(manifest.orientation, blink::WebScreenOrientationLockDefault);
+  ASSERT_EQ(manifest.theme_color, Manifest::kInvalidOrMissingThemeColor);
   ASSERT_TRUE(manifest.gcm_sender_id.is_null());
 }
 
@@ -94,15 +95,16 @@ TEST_F(ManifestParserTest, ValidNoContentParses) {
   ASSERT_TRUE(manifest.start_url.is_empty());
   ASSERT_EQ(manifest.display, Manifest::DISPLAY_MODE_UNSPECIFIED);
   ASSERT_EQ(manifest.orientation, blink::WebScreenOrientationLockDefault);
+  ASSERT_EQ(manifest.theme_color, Manifest::kInvalidOrMissingThemeColor);
   ASSERT_TRUE(manifest.gcm_sender_id.is_null());
 }
 
 TEST_F(ManifestParserTest, MultipleErrorsReporting) {
   Manifest manifest = ParseManifest("{ \"name\": 42, \"short_name\": 4,"
-      "\"orientation\": {}, \"display\": \"foo\", \"start_url\": null,"
-      "\"icons\": {} }");
+      "\"orientation\": {}, \"display\": \"foo\","
+      "\"start_url\": null, \"icons\": {}, \"theme_color\": 42 }");
 
-  EXPECT_EQ(6u, GetErrorCount());
+  EXPECT_EQ(7u, GetErrorCount());
 
   EXPECT_EQ("Manifest parsing error: property 'name' ignored,"
             " type string expected.",
@@ -121,6 +123,9 @@ TEST_F(ManifestParserTest, MultipleErrorsReporting) {
   EXPECT_EQ("Manifest parsing error: property 'icons' ignored, "
             "type array expected.",
             errors()[5]);
+  EXPECT_EQ("Manifest parsing error: property 'theme_color' ignored,"
+            " type string expected.",
+            errors()[6]);
 }
 
 TEST_F(ManifestParserTest, NameParseRules) {
@@ -350,6 +355,129 @@ TEST_F(ManifestParserTest, DisplayParserRules) {
   }
 }
 
+TEST_F(ManifestParserTest, ThemeColorParserRules) {
+  // Smoke test.
+  {
+    Manifest manifest = ParseManifest("{ \"theme_color\": \"#FF0000\" }");
+    EXPECT_EQ(manifest.theme_color, 0xFFFF0000);
+    EXPECT_FALSE(manifest.IsEmpty());
+    EXPECT_EQ(0u, GetErrorCount());
+  }
+
+  // Trim whitespaces.
+  {
+    Manifest manifest = ParseManifest("{ \"theme_color\": \"  blue   \" }");
+    EXPECT_EQ(manifest.theme_color, 0xFF0000FF);
+    EXPECT_EQ(0u, GetErrorCount());
+  }
+
+  // Don't parse if theme_color isn't a string.
+  {
+    Manifest manifest = ParseManifest("{ \"theme_color\": {} }");
+    EXPECT_EQ(manifest.theme_color, Manifest::kInvalidOrMissingThemeColor);
+    EXPECT_EQ(1u, GetErrorCount());
+    EXPECT_EQ("Manifest parsing error: property 'theme_color' ignored,"
+              " type string expected.",
+              errors()[0]);
+  }
+
+  // Don't parse if theme_color isn't a string.

[nasko, 2015/07/17 09:48:53]

If this is meant to be exhaustive test for different JSON data types that aren't
expected, then it is missing some (boolean, array, null).

[Lalit Maganti, 2015/07/17 10:30:00]

Done.

+  {
+    Manifest manifest = ParseManifest("{ \"theme_color\": 42 }");
+    EXPECT_EQ(manifest.theme_color, Manifest::kInvalidOrMissingThemeColor);
+    EXPECT_EQ(1u, GetErrorCount());
+    EXPECT_EQ("Manifest parsing error: property 'theme_color' ignored,"
+              " type string expected.",
+              errors()[0]);
+  }
+
+  // Parse fails if string is not in a known format.
+  {
+    Manifest manifest = ParseManifest("{ \"theme_color\": \"foo(bar)\" }");
+    EXPECT_EQ(manifest.theme_color, Manifest::kInvalidOrMissingThemeColor);
+    EXPECT_EQ(1u, GetErrorCount());
+    EXPECT_EQ("Manifest parsing error: property 'theme_color' ignored,"
+              " 'foo(bar)' is not a valid color.",
+              errors()[0]);
+  }
+
+  // Parse fails if string is not in a known format.
+  {
+    Manifest manifest = ParseManifest("{ \"theme_color\": \"bleu\" }");
+    EXPECT_EQ(manifest.theme_color, Manifest::kInvalidOrMissingThemeColor);
+    EXPECT_EQ(1u, GetErrorCount());
+    EXPECT_EQ("Manifest parsing error: property 'theme_color' ignored, 'bleu'"
+              " is not a valid color.",
+              errors()[0]);
+  }
+
+  // Parse fails if string is not in a known format.
+  {
+    Manifest manifest = ParseManifest("{ \"theme_color\": \"FF00FF\" }");
+    EXPECT_EQ(manifest.theme_color, Manifest::kInvalidOrMissingThemeColor);
+    EXPECT_EQ(1u, GetErrorCount());
+    EXPECT_EQ("Manifest parsing error: property 'theme_color' ignored, 'FF00FF'"
+              " is not a valid color.",
+              errors()[0]);
+  }
+
+  // Parse fails if multiple values for theme_color are given.
+  {
+    Manifest manifest = ParseManifest("{ \"theme_color\": \"#ABC #DEF\" }");
+    EXPECT_EQ(manifest.theme_color, Manifest::kInvalidOrMissingThemeColor);
+    EXPECT_EQ(1u, GetErrorCount());
+    EXPECT_EQ("Manifest parsing error: property 'theme_color' ignored, "
+              "'#ABC #DEF' is not a valid color.",
+              errors()[0]);
+  }
+
+  // Parse fails if multiple values for theme_color are given.

[nasko, 2015/07/17 09:48:53]

What happens if there are multiple theme_color instances? Do we pick
first/last/random? It should be tested to ensure we don't regress the
expectation we set.

[Lalit Maganti, 2015/07/17 10:30:00]

This depends on the JSON parser used - the JSON spec does not specify the order
and websites should not be assuming an order being set. Since this is a unit
test for manifest parser it doesn't make much sense to test the JSON parser
implementation.

[nasko, 2015/07/17 16:36:18]

Well,  there is one JSON parser that Chrome will use, so the argument about
different parsers seems moot to me. I'm fine if you think it is not worth
testing this particular case, though it helps to ensure that we don't break
poorly coded manifests.

+  {
+    Manifest manifest = ParseManifest(
+        "{ \"theme_color\": \"#AABBCC #DDEEFF\" }");
+    EXPECT_EQ(manifest.theme_color, Manifest::kInvalidOrMissingThemeColor);
+    EXPECT_EQ(1u, GetErrorCount());
+    EXPECT_EQ("Manifest parsing error: property 'theme_color' ignored, "
+              "'#AABBCC #DDEEFF' is not a valid color.",
+              errors()[0]);
+  }
+
+  // Accept CSS color keyword format.
+  {
+    Manifest manifest = ParseManifest("{ \"theme_color\": \"blue\" }");
+    EXPECT_EQ(manifest.theme_color, 0xFF0000FF);
+    EXPECT_EQ(0u, GetErrorCount());
+  }
+
+  // Accept CSS color keyword format.
+  {
+    Manifest manifest = ParseManifest("{ \"theme_color\": \"chartreuse\" }");
+    EXPECT_EQ(manifest.theme_color, 0xFF7FFF00);
+    EXPECT_EQ(0u, GetErrorCount());
+  }
+
+  // Accept CSS RGB format.
+  {
+    Manifest manifest = ParseManifest("{ \"theme_color\": \"#FFF\" }");
+    EXPECT_EQ(manifest.theme_color, 0xFFFFFFFF);
+    EXPECT_EQ(0u, GetErrorCount());
+  }
+
+  // Accept CSS RGB format.
+  {
+    Manifest manifest = ParseManifest("{ \"theme_color\": \"#ABC\" }");
+    EXPECT_EQ(manifest.theme_color, 0xFFAABBCC);
+    EXPECT_EQ(0u, GetErrorCount());
+  }
+
+  // Accept CSS RRGGBB format.
+  {
+    Manifest manifest = ParseManifest("{ \"theme_color\": \"#FF0000\" }");
+    EXPECT_EQ(manifest.theme_color, 0xFFFF0000);
+    EXPECT_EQ(0u, GetErrorCount());
+  }
+}
+
 TEST_F(ManifestParserTest, OrientationParserRules) {
   // Smoke test.
   {