Implement "scope" Web Manifest parsing

content/renderer/manifest/manifest_parser.cc

Index: content/renderer/manifest/manifest_parser.cc
diff --git a/content/renderer/manifest/manifest_parser.cc b/content/renderer/manifest/manifest_parser.cc
index 8be16c8b0055d59b49e2c9175008bb864e9ca4de..b1964c4d361b639661ba34326001229ed248b16b 100644
--- a/content/renderer/manifest/manifest_parser.cc
+++ b/content/renderer/manifest/manifest_parser.cc
@@ -64,6 +64,7 @@ void ManifestParser::Parse() {
   manifest_.name = ParseName(*dictionary);
   manifest_.short_name = ParseShortName(*dictionary);
   manifest_.start_url = ParseStartURL(*dictionary);
+  manifest_.scope = ParseScope(*dictionary, manifest_.start_url);
   manifest_.display = ParseDisplay(*dictionary);
   manifest_.orientation = ParseOrientation(*dictionary);
   manifest_.icons = ParseIcons(*dictionary);
@@ -183,6 +184,29 @@ GURL ManifestParser::ParseStartURL(const base::DictionaryValue& dictionary) {
   return start_url;
 }
 
+GURL ManifestParser::ParseScope(const base::DictionaryValue& dictionary,
+                                const GURL& start_url) {
+  GURL scope = ParseURL(dictionary, "scope", manifest_url_);
+  if (!scope.is_valid())

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

the spec suggests a developer warning here as well, I think

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

You are right. I am following the pattern in ManifestParser::ParseStartURL().
Fixing ParseStartURL() is out of the scope of this CL

[hartmanng, 2016/06/15 15:39:30]

Fair enough, but maybe add a TODO here?

+    return GURL();
+
+  if (scope.GetOrigin() != document_url_.GetOrigin()) {

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

This is most likely correct, but note that the spec does have a specific
definition of what it means to be in the "same origin"
(https://html.spec.whatwg.org/#same-origin) - might be worth checking to make
sure GURL.GetOrigin() conforms to that.

+    AddErrorInfo("property 'scope' ignored, should be "
+                 "same origin as document.");
+    return GURL();
+  }
+
+  if (scope.GetOrigin() != start_url.GetOrigin() ||
+      !base::StartsWith(start_url.path(), scope.path(),

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

As long as GURL.path() returns a canonicalized path, this should be correct. May
be worth testing some edge cases like

start_url: http://foo.com/
scope: http://foo.com

to make sure that the URL strings are normalized properly before just doing a
string compare

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

It looks like GURL::path() returns a canonicalized path. GURL::path() returns
"/" for both of the URLs that you have provided

[hartmanng, 2016/06/15 15:39:29]

sounds good, thanks for checking.

+                        base::CompareCase::SENSITIVE)) {
+    AddErrorInfo("property 'scope' ignored. 'start_url' should be within scope "
+                 "of scope URL.");
+    return GURL();
+  }
+
+  return scope;
+}
+
 blink::WebDisplayMode ManifestParser::ParseDisplay(
     const base::DictionaryValue& dictionary) {
   base::NullableString16 display = ParseString(dictionary, "display", Trim);