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) {

[mlamouri (slow - plz ping), 2015/07/16 14:53:31]

I wanted you to remove the first smoke test, not both, please add:
  // Smoke test.
  {
    Manifest manifest = ParseManifest("{ \"theme_color\": \"#FF0000\" }");
    EXPECT_EQ(manifest.theme_color, 0xFFFF0000);
    EXPECT_FALSE(manifest.IsEmpty());
    EXPECT_EQ(0u, GetErrorCount());
  }

[Lalit Maganti, 2015/07/16 15:12:28]

Done.

+  // Don't parse if theme_color isn't a string.

[mlamouri (slow - plz ping), 2015/07/16 14:53:31]

You removed the whitespace test too. Could you add it back?

[Lalit Maganti, 2015/07/16 15:12:28]

Done.

+  {
+    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.
+  {
+    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.

[mlamouri (slow - plz ping), 2015/07/16 14:53:31]

Could you add more test for this case? Things closer to the format like:
bleu
FF00FF

(the first is a typo for blue, the second is missing #)

[Lalit Maganti, 2015/07/16 15:12:28]

Done.

+  {
+    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,"
+              " invalid color specified.",
+              errors()[0]);
+  }
+
+  // Parse fails if multiple values for theme_color are given.
+  {
+    Manifest manifest = ParseManifest("{ \"theme_color\": \"#ABC#DEF\" }");

[mlamouri (slow - plz ping), 2015/07/16 14:53:31]

This sounds more like a wrong format. You could keep this test but please add a
test like:

  Manifest manifest = ParseManifest("{ \"theme_color\": \"#AABBCC #DDEEFF\" }");

[Lalit Maganti, 2015/07/16 15:12:28]

Done.

+    EXPECT_EQ(manifest.theme_color, Manifest::kInvalidOrMissingThemeColor);
+    EXPECT_EQ(1u, GetErrorCount());
+    EXPECT_EQ("Manifest parsing error: property 'theme_color' ignored,"
+              " invalid color specified.",
+              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