Implement "scope" Web Manifest parsing

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 269 matching lines @@
   {
     Manifest manifest =
         ParseManifestWithURLs("{ \"start_url\": \"land.html\" }",
                               GURL("http://foo.com/landing/manifest.json"),
                               GURL("http://foo.com/index.html"));
     ASSERT_EQ(manifest.start_url.spec(), "http://foo.com/landing/land.html");
     EXPECT_EQ(0u, GetErrorCount());
   }
 }
 
+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.
   {
     Manifest manifest = ParseManifest("{ \"display\": \"browser\" }");
     EXPECT_EQ(manifest.display, blink::WebDisplayModeBrowser);
     EXPECT_FALSE(manifest.IsEmpty());
     EXPECT_EQ(0u, GetErrorCount());
   }
 
   // Trim whitespaces.
@@ skipping 960 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