Implement "scope" Web Manifest parsing

content/renderer/manifest/manifest_parser.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 <stddef.h>
 
 #include "base/json/json_reader.h"
 #include "base/memory/ptr_util.h"
@@ skipping 46 matching lines @@
     AddErrorInfo("root element must be a valid JSON object.", true);
     ManifestUmaUtil::ParseFailed();
     failed_ = true;
     return;
   }
   DCHECK(dictionary);
 
   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);
   manifest_.related_applications = ParseRelatedApplications(*dictionary);
   manifest_.prefer_related_applications =
       ParsePreferRelatedApplications(*dictionary);
   manifest_.theme_color = ParseThemeColor(*dictionary);
   manifest_.background_color = ParseBackgroundColor(*dictionary);
   manifest_.gcm_sender_id = ParseGCMSenderID(*dictionary);
 
@@ skipping 99 matching lines @@
 
   if (start_url.GetOrigin() != document_url_.GetOrigin()) {
     AddErrorInfo("property 'start_url' ignored, should be "
                  "same origin as document.");
     return GURL();
   }
 
   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);
   if (display.is_null())
     return blink::WebDisplayModeUndefined;
 
   if (base::LowerCaseEqualsASCII(display.string(), "fullscreen"))
     return blink::WebDisplayModeFullscreen;
   else if (base::LowerCaseEqualsASCII(display.string(), "standalone"))
     return blink::WebDisplayModeStandalone;
@@ skipping 181 matching lines @@
 }
 
 void ManifestParser::AddErrorInfo(const std::string& error_msg,
                                   bool critical,
                                   int error_line,
                                   int error_column) {
   errors_.push_back({error_msg, critical, error_line, error_column});
 }
 
 } // namespace content