content: make theme_color more resilient to casting issues

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"
 
 namespace content {
 
+namespace {
+
+uint32_t ExtractColor(int64_t color) {
+  return reinterpret_cast<uint32_t&>(color);
+}
+
+} // anonymous namespace
+
 class ManifestParserTest : public testing::Test  {
  protected:
   ManifestParserTest() {}
   ~ManifestParserTest() override {}
 
   Manifest ParseManifestWithURLs(const base::StringPiece& data,
                                  const GURL& document_url,
                                  const GURL& manifest_url) {
     ManifestParser parser(data, document_url, manifest_url);
     parser.Parse();
@@ skipping 958 matching lines @@
         ParseManifest("{ \"prefer_related_applications\": false }");
     EXPECT_FALSE(manifest.prefer_related_applications);
     EXPECT_EQ(0u, GetErrorCount());
   }
 }
 
 TEST_F(ManifestParserTest, ThemeColorParserRules) {
   // Smoke test.
   {
     Manifest manifest = ParseManifest("{ \"theme_color\": \"#FF0000\" }");
-    EXPECT_EQ(manifest.theme_color, 0xFFFF0000);
+    EXPECT_EQ(ExtractColor(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(ExtractColor(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.",
@@ skipping 87 matching lines @@
     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(ExtractColor(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(ExtractColor(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(ExtractColor(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(ExtractColor(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(ExtractColor(manifest.theme_color), 0xFFFF0000);
     EXPECT_EQ(0u, GetErrorCount());
   }

[Bernhard Bauer, 2015/07/31 10:07:45]

Out of curiosity, do we allow transparent colors here?

[Lalit Maganti, 2015/07/31 10:13:17]

As far as I know there is no check against it here because we simply want to
store whatever is in the manifest. Higher up, we may well disallow transparent
colors as it doesn't make any sense to theme the UI transparent.

[Bernhard Bauer, 2015/07/31 10:26:55]

OK, in that case would it make sense to add tests for transparent values (can be
in a followup CL)? That would exercise the code paths for a) parsing argb color
values, and b) converting values with the MSB _not_ set (which probably is the
less problematic case, but it's still good to have coverage).

[mlamouri (slow - plz ping), 2015/08/05 13:35:33]

We could add a test with AA=00 in there, right?

 }
 
 TEST_F(ManifestParserTest, GCMSenderIDParseRules) {
   // Smoke test.
   {
     Manifest manifest = ParseManifest("{ \"gcm_sender_id\": \"foo\" }");
     EXPECT_TRUE(base::EqualsASCII(manifest.gcm_sender_id.string(), "foo"));
     EXPECT_EQ(0u, GetErrorCount());
   }
 
@@ skipping 17 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