Implement "scope" Web Manifest parsing

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 b86a0e7e6ca5c8492036ccbad00caa4b158178c7..23fb4238952c589ea2e2010c8f20496c49d42644 100644
--- a/content/renderer/manifest/manifest_parser_unittest.cc
+++ b/content/renderer/manifest/manifest_parser_unittest.cc
@@ -287,6 +287,121 @@ TEST_F(ManifestParserTest, StartURLParseRules) {
   }
 }
 
+TEST_F(ManifestParserTest, ScopeParseRules) {

[hartmanng, 2016/06/14 18:29:43]

these tests look good, but I suggest adding in some where the scope contains
"..". The spec doesn't technically disallow that, IIUC.

So if your manifest is at http://foo.com/landing/manifest.json, your scope could
be set as ".." and therefore resolve to http://foo.com/. Seems weird, but as far
as I can tell, that's the expected behaviour according to the current spec.

[pkotwicz, 2016/06/15 01:29:31]

Added a test for '..' scope

+  // Smoke test.
+  {
+    Manifest manifest = ParseManifest(
+        "{ \"scope\": \"land\", \"start_url\": \"land/landing.html\" }");
+    ASSERT_EQ(manifest.scope.spec(),
+              default_document_url.Resolve("land").spec());
+    ASSERT_FALSE(manifest.IsEmpty());
+    EXPECT_EQ(0u, GetErrorCount());
+  }
+
+  // Whitespaces.
+  {
+    Manifest manifest = ParseManifest(
+        "{ \"scope\": \"  land  \", \"start_url\": \"land/landing.html\" }");
+    ASSERT_EQ(manifest.scope.spec(),
+              default_document_url.Resolve("land").spec());
+    EXPECT_EQ(0u, GetErrorCount());
+  }
+
+  // Don't parse if property isn't a string.
+  {
+    Manifest manifest = ParseManifest("{ \"scope\": {} }");
+    ASSERT_TRUE(manifest.scope.is_empty());
+    EXPECT_EQ(1u, GetErrorCount());
+    EXPECT_EQ("property 'scope' ignored, type string expected.", errors()[0]);
+  }
+
+  // Don't parse if property isn't a string.
+  {
+    Manifest manifest = ParseManifest("{ \"scope\": 42 }");
+    ASSERT_TRUE(manifest.scope.is_empty());
+    EXPECT_EQ(1u, GetErrorCount());
+    EXPECT_EQ("property 'scope' ignored, type string expected.", errors()[0]);
+  }
+
+  // Absolute scope, start URL is in scope.
+  {
+    Manifest manifest = ParseManifestWithURLs(
+        "{ \"scope\": \"http://foo.com/land\", "
+        "\"start_url\": \"http://foo.com/land/landing.html\" }",
+        GURL("http://foo.com/manifest.json"),
+        GURL("http://foo.com/index.html"));
+    ASSERT_EQ(manifest.scope.spec(), "http://foo.com/land");
+    EXPECT_EQ(0u, GetErrorCount());
+  }
+
+  // Absolute scope, start URL is not in scope.
+  {
+    Manifest manifest =
+        ParseManifestWithURLs("{ \"scope\": \"http://foo.com/land\", "
+                              "\"start_url\": \"http://foo.com/index.html\" }",
+                              GURL("http://foo.com/manifest.json"),

[Xi Han, 2016/06/14 19:07:01]

The last parameter is the manifest_url, not the second last:
https://cs.chromium.org/chromium/src/content/renderer/manifest/manifest_parse...

If you use the same |ParseManifestWithURLs| function, should switch the
document_url and manifest_url.

[pkotwicz, 2016/06/15 01:29:31]

Thank you for pointing it out. I have fixed this in
https://codereview.chromium.org/2064853003/

+                              GURL("http://foo.com/index.html"));
+    ASSERT_TRUE(manifest.scope.is_empty());
+    EXPECT_EQ(1u, GetErrorCount());
+    EXPECT_EQ("property 'scope' ignored. 'start_url' should be within scope "
+              "of scope URL.",
+              errors()[0]);
+  }
+
+  // Absolute scope, start URL has different origin than scope URL.
+  {
+    Manifest manifest =
+        ParseManifestWithURLs("{ \"scope\": \"http://foo.com/land\", "
+                              "\"start_url\": \"http://bar.com/land/landing.html\" }",
+                              GURL("http://foo.com/manifest.json"),

[Xi Han, 2016/06/14 19:07:01]

Same above.

+                              GURL("http://foo.com/index.html"));
+    ASSERT_TRUE(manifest.scope.is_empty());
+    ASSERT_EQ(2u, GetErrorCount());
+    EXPECT_EQ(
+        "property 'start_url' ignored, should be same origin as document.",
+        errors()[0]);
+    EXPECT_EQ("property 'scope' ignored. 'start_url' should be within scope "
+              "of scope URL.",
+              errors()[1]);
+  }
+
+  // scope and start URL have diferent origin than document URL.
+  {
+    Manifest manifest =
+        ParseManifestWithURLs("{ \"scope\": \"http://foo.com/land\", "
+                              "\"start_url\": \"http://foo.com/land/landing.html\" }",
+                              GURL("http://foo.com/manifest.json"),

[Xi Han, 2016/06/14 19:07:01]

Same above.

+                              GURL("http://bar.com/index.html"));
+    ASSERT_TRUE(manifest.scope.is_empty());
+    ASSERT_EQ(2u, GetErrorCount());
+    EXPECT_EQ(
+        "property 'start_url' ignored, should be same origin as document.",
+        errors()[0]);
+    EXPECT_EQ("property 'scope' ignored, should be same origin as document.",
+              errors()[1]);
+  }
+
+  // No start URL.
+  {
+    Manifest manifest = ParseManifest("{ \"scope\": \"land\" }");
+    ASSERT_EQ(1u, GetErrorCount());
+    EXPECT_EQ("property 'scope' ignored. 'start_url' should be within scope "
+              "of scope URL.",
+              errors()[0]);
+  }
+
+  // Resolving has to happen based on the manifest_url.
+  {
+    Manifest manifest =
+        ParseManifestWithURLs("{ \"scope\": \"land\", "
+                              "\"start_url\": \"landing.html\" }",
+                              GURL("http://foo.com/secret/manifest.json"),

[Xi Han, 2016/06/14 19:07:01]

Same as above.

+                              GURL("http://foo.com/index.html"));
+    ASSERT_EQ(manifest.scope.spec(), "http://foo.com/secret/land");
+    EXPECT_EQ(0u, GetErrorCount());
+  }
+}
+
 TEST_F(ManifestParserTest, DisplayParserRules) {
   // Smoke test.
   {