Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2063003003:
Implement "scope" Web Manifest parsing (Closed)
|
Created:
4 years, 6 months ago by pkotwicz Modified:
4 years, 6 months ago CC:
chromium-reviews, darin-cc_chromium.org, dominickn, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-manifest_chromium.org, mlamouri+watch-content_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement "scope" Web Manifest parsing
The parsing algorithm is defined in
https://www.w3.org/TR/appmanifest/#scope-member
This CL also sets start_url to the document URL if
the start_url cannot be parsed as per the spec
BUG=619739
Committed: https://crrev.com/c099a894d1c0569d4bd20374ed8773ebcba6c989
Cr-Commit-Position: refs/heads/master@{#401640}
Patch Set 1 : Merge branch 'webapk_manifest' into webapk_manifest_scope0 #
Total comments: 14
Patch Set 2 : Merge branch 'webapk_manifest_scope00' into webapk_manifest_scope0 #
Total comments: 1
Patch Set 3 : Merge branch 'webapk_manifest_scope00' into webapk_manifest_scope0 #
Total comments: 3
Patch Set 4 : Merge branch 'webapk_manifest_scope00' into webapk_manifest_scope0 #
Messages
Total messages: 44 (22 generated)
Description was changed from ========== Implement "scope" Web Manifest parsing BUG=619739 ========== to ========== Implement "scope" Web Manifest parsing The parsing algorithm is defined in https://www.w3.org/TR/appmanifest/#scope-member BUG=619739 ==========
pkotwicz@chromium.org changed reviewers: + hanxi@chromium.org
Patchset #1 (id:1) has been deleted
pkotwicz@chromium.org changed reviewers: + hartmanng@chromium.org
Xi, can you please take a look? Glenn, can you please double check the scope validation logic. I think that the spec requires start_url to be defined whenever scope is defined but I am unsure
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #1 (id:20001) has been deleted
On 2016/06/14 17:45:50, pkotwicz wrote: > I think that the > spec requires start_url to be defined whenever scope is defined but I am unsure According to my understanding of the spec, I think the current ParseStartURL() isn't quite correct. According to https://www.w3.org/TR/appmanifest/#start_url-member, if the start URL is invalid, unparseable, or unset, then the start_url should default to the document URL. The current code returns GURL() on failure. In ParseScope() you are supposed to be able to assume that you have a start_url, but in the current code I'm not sure you can.
I think rietveld killed my draft comments, give me a minute to rewrite them...
https://codereview.chromium.org/2063003003/diff/80001/content/renderer/manife... File content/renderer/manifest/manifest_parser.cc (right): https://codereview.chromium.org/2063003003/diff/80001/content/renderer/manife... content/renderer/manifest/manifest_parser.cc:190: if (!scope.is_valid()) the spec suggests a developer warning here as well, I think https://codereview.chromium.org/2063003003/diff/80001/content/renderer/manife... content/renderer/manifest/manifest_parser.cc:193: if (scope.GetOrigin() != document_url_.GetOrigin()) { 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. https://codereview.chromium.org/2063003003/diff/80001/content/renderer/manife... content/renderer/manifest/manifest_parser.cc:200: !base::StartsWith(start_url.path(), scope.path(), 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 https://codereview.chromium.org/2063003003/diff/80001/content/renderer/manife... File content/renderer/manifest/manifest_parser_unittest.cc (right): https://codereview.chromium.org/2063003003/diff/80001/content/renderer/manife... content/renderer/manifest/manifest_parser_unittest.cc:290: TEST_F(ManifestParserTest, ScopeParseRules) { 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.
https://codereview.chromium.org/2063003003/diff/80001/content/renderer/manife... File content/renderer/manifest/manifest_parser_unittest.cc (right): https://codereview.chromium.org/2063003003/diff/80001/content/renderer/manife... content/renderer/manifest/manifest_parser_unittest.cc:342: GURL("http://foo.com/manifest.json"), 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. https://codereview.chromium.org/2063003003/diff/80001/content/renderer/manife... content/renderer/manifest/manifest_parser_unittest.cc:356: GURL("http://foo.com/manifest.json"), Same above. https://codereview.chromium.org/2063003003/diff/80001/content/renderer/manife... content/renderer/manifest/manifest_parser_unittest.cc:373: GURL("http://foo.com/manifest.json"), Same above. https://codereview.chromium.org/2063003003/diff/80001/content/renderer/manife... content/renderer/manifest/manifest_parser_unittest.cc:398: GURL("http://foo.com/secret/manifest.json"), Same as above.
Xi and Glenn can you please take another look? Thank you Glenn for pointing out the issue with the start_url parsing https://codereview.chromium.org/2063003003/diff/80001/content/renderer/manife... File content/renderer/manifest/manifest_parser.cc (right): https://codereview.chromium.org/2063003003/diff/80001/content/renderer/manife... content/renderer/manifest/manifest_parser.cc:190: if (!scope.is_valid()) You are right. I am following the pattern in ManifestParser::ParseStartURL(). Fixing ParseStartURL() is out of the scope of this CL https://codereview.chromium.org/2063003003/diff/80001/content/renderer/manife... content/renderer/manifest/manifest_parser.cc:200: !base::StartsWith(start_url.path(), scope.path(), It looks like GURL::path() returns a canonicalized path. GURL::path() returns "/" for both of the URLs that you have provided https://codereview.chromium.org/2063003003/diff/80001/content/renderer/manife... File content/renderer/manifest/manifest_parser_unittest.cc (right): https://codereview.chromium.org/2063003003/diff/80001/content/renderer/manife... content/renderer/manifest/manifest_parser_unittest.cc:290: TEST_F(ManifestParserTest, ScopeParseRules) { Added a test for '..' scope https://codereview.chromium.org/2063003003/diff/80001/content/renderer/manife... content/renderer/manifest/manifest_parser_unittest.cc:342: GURL("http://foo.com/manifest.json"), Thank you for pointing it out. I have fixed this in https://codereview.chromium.org/2064853003/
Description was changed from ========== Implement "scope" Web Manifest parsing The parsing algorithm is defined in https://www.w3.org/TR/appmanifest/#scope-member BUG=619739 ========== to ========== Implement "scope" Web Manifest parsing The parsing algorithm is defined in https://www.w3.org/TR/appmanifest/#scope-member This CL also sets start_url to the document URL if the start_url cannot be parsed as per the spec BUG=619739 ==========
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
lgtm with nits. https://codereview.chromium.org/2063003003/diff/140001/content/renderer/manif... File content/renderer/manifest/manifest_parser.cc (right): https://codereview.chromium.org/2063003003/diff/140001/content/renderer/manif... content/renderer/manifest/manifest_parser.cc:200: // should be used as the start URL. If the start_url could not be parsed, start URL v.s start_url, Please keep these terminology in consistent.
On 2016/06/15 15:20:57, Xi Han wrote: > lgtm with nits. > > https://codereview.chromium.org/2063003003/diff/140001/content/renderer/manif... > File content/renderer/manifest/manifest_parser.cc (right): > > https://codereview.chromium.org/2063003003/diff/140001/content/renderer/manif... > content/renderer/manifest/manifest_parser.cc:200: // should be used as the start > URL. If the start_url could not be parsed, > start URL v.s start_url, Please keep these terminology in consistent. lgtm2, with a small request https://codereview.chromium.org/2063003003/diff/80001/content/renderer/manife... File content/renderer/manifest/manifest_parser.cc (right): https://codereview.chromium.org/2063003003/diff/80001/content/renderer/manife... content/renderer/manifest/manifest_parser.cc:190: if (!scope.is_valid()) On 2016/06/15 01:29:31, pkotwicz wrote: > You are right. I am following the pattern in ManifestParser::ParseStartURL(). > Fixing ParseStartURL() is out of the scope of this CL Fair enough, but maybe add a TODO here? https://codereview.chromium.org/2063003003/diff/80001/content/renderer/manife... content/renderer/manifest/manifest_parser.cc:200: !base::StartsWith(start_url.path(), scope.path(), On 2016/06/15 01:29:31, pkotwicz wrote: > It looks like GURL::path() returns a canonicalized path. GURL::path() returns > "/" for both of the URLs that you have provided sounds good, thanks for checking.
I guess "lgtm2" doesn't count, so LGTM
pkotwicz@chromium.org changed reviewers: + mlamouri@chromium.org
mlamouri@ PTAL
+dominickn@ FYI lgtm with comments applied. https://codereview.chromium.org/2063003003/diff/160001/content/public/common/... File content/public/common/manifest.cc (right): https://codereview.chromium.org/2063003003/diff/160001/content/public/common/... content/public/common/manifest.cc:47: scope.is_empty() && ditto https://codereview.chromium.org/2063003003/diff/160001/content/public/common/... File content/public/common/manifest.h (right): https://codereview.chromium.org/2063003003/diff/160001/content/public/common/... content/public/common/manifest.h:86: GURL scope; There is no intent in keeping the list alphabetically ordered. We usually add stuff to the end. https://codereview.chromium.org/2063003003/diff/160001/content/renderer/manif... File content/renderer/manifest/manifest_parser.cc (right): https://codereview.chromium.org/2063003003/diff/160001/content/renderer/manif... content/renderer/manifest/manifest_parser.cc:191: // TODO(pkotwicz): Emit developer warning when URL is invalid. I would prefer if you could do this in this CL. It should be only one call :)
Patchset #6 (id:220001) has been deleted
Patchset #5 (id:200001) has been deleted
Patchset #4 (id:180001) has been deleted
Patchset #4 (id:240001) has been deleted
mlamouri@ can you please take another look? I changed the code to emit a developer warning in ManifestParser::ParseURL() This affects the parsing of 'start_url', 'scope', 'icons' and 'related_applications' The spec mentions that a developer warning should be emitted if the URL is invalid for 'start_url' and 'scope' but not for 'icons' and 'related_applications'. Thus, my change is not spec compliant. I think that this is OK. Based on gurl_unittest.cc and url_util_unittest.cc there are very few strings for which ManifestParser::ParseURL() fails.
still lgtm. Thanks :)
The CQ bit was checked by pkotwicz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hanxi@chromium.org, hartmanng@chromium.org Link to the patchset: https://codereview.chromium.org/2063003003/#ps260001 (title: "Merge branch 'webapk_manifest_scope00' into webapk_manifest_scope0")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2063003003/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
sievers@ for content/ rubberstamp
sievers@ for content/ rubberstamp
pkotwicz@chromium.org changed reviewers: + sievers@chromium.org
sievers@ for content/ rubberstamp
On 2016/06/23 13:39:31, pkotwicz wrote: > sievers@ for content/ rubberstamp lgtm
The CQ bit was checked by pkotwicz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2063003003/260001
Message was sent while issue was closed.
Description was changed from ========== Implement "scope" Web Manifest parsing The parsing algorithm is defined in https://www.w3.org/TR/appmanifest/#scope-member This CL also sets start_url to the document URL if the start_url cannot be parsed as per the spec BUG=619739 ========== to ========== Implement "scope" Web Manifest parsing The parsing algorithm is defined in https://www.w3.org/TR/appmanifest/#scope-member This CL also sets start_url to the document URL if the start_url cannot be parsed as per the spec BUG=619739 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Implement "scope" Web Manifest parsing The parsing algorithm is defined in https://www.w3.org/TR/appmanifest/#scope-member This CL also sets start_url to the document URL if the start_url cannot be parsed as per the spec BUG=619739 ========== to ========== Implement "scope" Web Manifest parsing The parsing algorithm is defined in https://www.w3.org/TR/appmanifest/#scope-member This CL also sets start_url to the document URL if the start_url cannot be parsed as per the spec BUG=619739 Committed: https://crrev.com/c099a894d1c0569d4bd20374ed8773ebcba6c989 Cr-Commit-Position: refs/heads/master@{#401640} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/c099a894d1c0569d4bd20374ed8773ebcba6c989 Cr-Commit-Position: refs/heads/master@{#401640} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
