Parse "purpose" member from Web 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 <stdint.h>
 
 #include <memory>
 
@@ skipping 860 matching lines @@
   {
     Manifest manifest = ParseManifest("{ \"icons\": [ {\"src\": \"\","
         "\"sizes\": \"x 40xx 1x2x3 x42 42xx42\" } ] }");
     EXPECT_EQ(manifest.icons[0].sizes.size(), 0u);
     EXPECT_EQ(1u, GetErrorCount());
     EXPECT_EQ("found icon with no valid size.",
               errors()[0]);
   }
 }
 
+TEST_F(ManifestParserTest, IconPurposeParseRules) {
+  // Smoke test.
+  {
+    Manifest manifest = ParseManifest("{ \"icons\": [ {\"src\": \"\","
+        "\"purpose\": \"any\" } ] }");
+    EXPECT_EQ(manifest.icons[0].purpose.size(), 1u);
+    EXPECT_EQ(0u, GetErrorCount());
+  }
+
+  // Trim whitespaces.
+  {
+    Manifest manifest = ParseManifest("{ \"icons\": [ {\"src\": \"\","
+        "\"purpose\": \"  any  \" } ] }");
+    EXPECT_EQ(manifest.icons[0].purpose.size(), 1u);
+    EXPECT_EQ(0u, GetErrorCount());
+  }
+
+  // Ignore purpose if property isn't present.
+  {
+    Manifest manifest = ParseManifest("{ \"icons\": [ {\"src\": \"\" } ] }");
+    EXPECT_EQ(manifest.icons[0].purpose.size(), 0u);
+    EXPECT_EQ(0u, GetErrorCount());
+  }
+
+  // Ignore purpose if property isn't a string (is a number).
+  {
+    Manifest manifest = ParseManifest("{ \"icons\": [ {\"src\": \"\","
+        "\"purpose\": 42 } ] }");
+    EXPECT_EQ(manifest.icons[0].purpose.size(), 0u);
+    EXPECT_EQ(1u, GetErrorCount());

[pkotwicz, 2016/10/18 14:13:56]

When the remainder of the test will crash (as opposed to just failing) if the
condition if false, we use ASSERT_EQ() instead of EXPECT_EQ()

ASSERT_EQ() makes the test stop & fail if the condition fails
EXPECT_EQ() makes the test fail but subsequent conditions are still evaluated.
I know that the remainder of this file does not use ASSERT_EQ() but we might as
well make new tests better.

In the case of this test.
EXPECT_EQ(1u, GetErrorCount()) should be ASSERT_EQ(1u, GetErrorCount())
because the array access on line 911 will fail if the condition on line 910
fails

[F, 2016/10/18 15:34:46]

Done.

+    EXPECT_EQ("property 'purpose' ignored, type string expected.",
+              errors()[0]);
+  }
+
+  // Ignore purpose if property isn't a string (is a dictionary).
+  {
+    Manifest manifest = ParseManifest("{ \"icons\": [ {\"src\": \"\","
+        "\"purpose\": {} } ] }");
+    EXPECT_EQ(manifest.icons[0].purpose.size(), 0u);
+    EXPECT_EQ(1u, GetErrorCount());
+    EXPECT_EQ("property 'purpose' ignored, type string expected.",
+              errors()[0]);
+  }
+
+  // Smoke test: value correctly parsed.
+  {
+    Manifest manifest = ParseManifest("{ \"icons\": [ {\"src\": \"\","
+        "\"purpose\": \"Any Badge\" } ] }");

[pkotwicz, 2016/10/18 14:13:56]

You should add:
ASSERT_EQ(manifest.icons[0].purpose.size(), 2u);

[F, 2016/10/18 15:34:46]

Done.

+    EXPECT_EQ(manifest.icons[0].purpose[0], Manifest::Icon::IconPurpose::ANY);
+    EXPECT_EQ(manifest.icons[0].purpose[1], Manifest::Icon::IconPurpose::BADGE);
+    EXPECT_EQ(0u, GetErrorCount());
+  }
+
+  // Twice the same value is parsed twice.
+  {
+    Manifest manifest = ParseManifest("{ \"icons\": [ {\"src\": \"\","
+        "\"purpose\": \"badge badge\" } ] }");

[pkotwicz, 2016/10/18 14:13:56]

You should add:
ASSERT_EQ(manifest.icons[0].purpose.size(), 2u);

[F, 2016/10/18 15:34:46]

Done.

+    EXPECT_EQ(manifest.icons[0].purpose[0], Manifest::Icon::IconPurpose::BADGE);
+    EXPECT_EQ(manifest.icons[0].purpose[1], Manifest::Icon::IconPurpose::BADGE);
+    EXPECT_EQ(0u, GetErrorCount());
+  }
+
+  // Invalid icon purpose is ignored.
+  {
+    Manifest manifest = ParseManifest("{ \"icons\": [ {\"src\": \"\","
+        "\"purpose\": \"badge notification\" } ] }");

[pkotwicz, 2016/10/18 14:13:56]

You should add:
ASSERT_EQ(manifest.icons[0].purpose.size(), 1u);

[F, 2016/10/18 15:34:46]

Done.

+    EXPECT_EQ(manifest.icons[0].purpose[0], Manifest::Icon::IconPurpose::BADGE);
+    EXPECT_EQ(1u, GetErrorCount());
+    EXPECT_EQ("found icon with invalid purpose.",
+              errors()[0]);
+  }
+
+  // 'any' is correctly added when the purpose is empty.

[pkotwicz, 2016/10/18 14:13:56]

How about: 'any' is added when developer-supplied-purpose is invalid.

[F, 2016/10/18 15:34:46]

Done.

+  {
+    Manifest manifest = ParseManifest("{ \"icons\": [ {\"src\": \"\","
+        "\"purpose\": \"notification\" } ] }");
+    EXPECT_EQ(manifest.icons[0].purpose[0], Manifest::Icon::IconPurpose::ANY);
+    EXPECT_EQ(1u, GetErrorCount());
+    EXPECT_EQ("found icon with invalid purpose.",
+              errors()[0]);
+  }

[pkotwicz, 2016/10/18 14:13:56]

Maybe add a test case for: \"purpose\": \"any  badge\" to ensure that
whitespaces between elements are trimmed properly

[F, 2016/10/18 15:34:46]

Done.

+}
+
 TEST_F(ManifestParserTest, RelatedApplicationsParseRules) {
   // If no application, empty list.
   {
     Manifest manifest = ParseManifest(
         "{ \"related_applications\": []}");
     EXPECT_EQ(manifest.related_applications.size(), 0u);
     EXPECT_TRUE(manifest.IsEmpty());
     EXPECT_EQ(0u, GetErrorCount());
   }
 
@@ skipping 523 matching lines @@
   {
     Manifest manifest = ParseManifest("{ \"gcm_sender_id\": 42 }");
     EXPECT_TRUE(manifest.gcm_sender_id.is_null());
     EXPECT_EQ(1u, GetErrorCount());
     EXPECT_EQ("property 'gcm_sender_id' ignored, type string expected.",
               errors()[0]);
   }
 }
 
 } // namespace content