manifest: add theme_color value to the manifest

content/renderer/manifest/manifest_parser_unittest.cc

 // Copyright 2014 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 "content/renderer/manifest/manifest_parser.h"
 
 #include "base/strings/string_util.h"
 #include "content/public/common/manifest.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ skipping 60 matching lines @@
   EXPECT_EQ("Manifest parsing error: Line: 1, column: 1, Unexpected token.",
             errors()[0]);
 
   // A parsing error is equivalent to an empty manifest.
   ASSERT_TRUE(manifest.IsEmpty());
   ASSERT_TRUE(manifest.name.is_null());
   ASSERT_TRUE(manifest.short_name.is_null());
   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, ValidNoContentParses) {
   Manifest manifest = ParseManifest("{}");
 
   // Empty Manifest is not a parsing error.
   EXPECT_EQ(0u, GetErrorCount());
 
   // Check that all the fields are null in that case.
   ASSERT_TRUE(manifest.IsEmpty());
   ASSERT_TRUE(manifest.name.is_null());
   ASSERT_TRUE(manifest.short_name.is_null());
   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.",
             errors()[0]);
   EXPECT_EQ("Manifest parsing error: property 'short_name' ignored,"
             " type string expected.",
             errors()[1]);
   EXPECT_EQ("Manifest parsing error: property 'start_url' ignored,"
             " type string expected.",
             errors()[2]);
   EXPECT_EQ("Manifest parsing error: unknown 'display' value ignored.",
             errors()[3]);
   EXPECT_EQ("Manifest parsing error: property 'orientation' ignored,"
             " type string expected.",
             errors()[4]);
   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) {
   // Smoke test.
   {
     Manifest manifest = ParseManifest("{ \"name\": \"foo\" }");
     ASSERT_TRUE(base::EqualsASCII(manifest.name.string(), "foo"));
     ASSERT_FALSE(manifest.IsEmpty());
     EXPECT_EQ(0u, GetErrorCount());
   }
@@ skipping 209 matching lines @@
   }
 
   // Case insensitive.
   {
     Manifest manifest = ParseManifest("{ \"display\": \"BROWSER\" }");
     EXPECT_EQ(manifest.display, Manifest::DISPLAY_MODE_BROWSER);
     EXPECT_EQ(0u, GetErrorCount());
   }
 }
 
+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.
   {
     Manifest manifest = ParseManifest("{ \"orientation\": \"natural\" }");
     EXPECT_EQ(manifest.orientation, blink::WebScreenOrientationLockNatural);
     EXPECT_FALSE(manifest.IsEmpty());
     EXPECT_EQ(0u, GetErrorCount());
   }
 
   // Trim whitespaces.
@@ skipping 644 matching lines @@
     Manifest manifest = ParseManifest("{ \"gcm_sender_id\": 42 }");
     EXPECT_TRUE(manifest.gcm_sender_id.is_null());
     EXPECT_EQ(1u, GetErrorCount());
     EXPECT_EQ("Manifest parsing error: property 'gcm_sender_id' ignored,"
               " type string expected.",
               errors()[0]);
   }
 }
 
 } // namespace content