Parse "purpose" member from Web 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 a2d12554fabe47faaac556f47ac0733f2fa65d6e..b2688a1dd96bb4597f8bf6dd52244e181764ce69 100644
--- a/content/renderer/manifest/manifest_parser_unittest.cc
+++ b/content/renderer/manifest/manifest_parser_unittest.cc
@@ -878,6 +878,89 @@ TEST_F(ManifestParserTest, IconSizesParseRules) {
   }
 }
 
+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.
   {