Summary: | WebAppManifest scope should default to the containing directory of start_url if 'scope' is not specified | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Quesada <david_quesada> | ||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | achristensen, cdumez, commit-queue, dbates, rniwa, webkit-bug-importer, youennf | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
David Quesada
2018-01-31 16:47:53 PST
Created attachment 332910 [details]
Patch
Comment on attachment 332910 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332910&action=review > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:211 > + URL result { url }; > + > + result.removeQueryAndFragmentIdentifier(); > + > + auto path = result.path(); > + if (path.endsWith('/')) > + return result; > + > + auto lastSlash = path.reverseFind('/'); > + if (lastSlash == WTF::notFound) { > + result.setPath({ }); > + return result; > + } > + > + result.setPath(path.substring(0, lastSlash + 1)); > + return result; Just do: URL(url, "./"). What's what service worker code does. Also, we should this help function scopeURL or computeScopeURL or something. "containing URL" is not really a nominal terminology. (In reply to Ryosuke Niwa from comment #3) > Comment on attachment 332910 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=332910&action=review > > > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:211 > > + URL result { url }; > > + > > + result.removeQueryAndFragmentIdentifier(); > > + > > + auto path = result.path(); > > + if (path.endsWith('/')) > > + return result; > > + > > + auto lastSlash = path.reverseFind('/'); > > + if (lastSlash == WTF::notFound) { > > + result.setPath({ }); > > + return result; > > + } > > + > > + result.setPath(path.substring(0, lastSlash + 1)); > > + return result; > > Just do: URL(url, "./"). What's what service worker code does. That's a lot cleaner. I'll use that instead. > Also, we should this help function scopeURL or computeScopeURL or something. > "containing URL" is not really a nominal terminology. Created attachment 332973 [details]
Patch v2
Comment on attachment 332973 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=332973&action=review > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:198 > - return { }; > + return { startURL, "./" }; Hm... I would have preferred to have a helper function like you did earlier. (In reply to Ryosuke Niwa from comment #6) > Comment on attachment 332973 [details] > Patch v2 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=332973&action=review > > > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:198 > > - return { }; > > + return { startURL, "./" }; > > Hm... I would have preferred to have a helper function like you did earlier. I'm open to adding back a helper function. How does `void defaultScopeURL(const URL& startURL)` sound? Comment on attachment 332973 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=332973&action=review >>> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:198 >>> + return { startURL, "./" }; >> >> Hm... I would have preferred to have a helper function like you did earlier. > > I'm open to adding back a helper function. How does `void defaultScopeURL(const URL& startURL)` sound? Sure that sounds good. Alternatively, you could just compute it once at the top of the function as defaultScopeURL local variable since this isn't a perf sensitive code. Created attachment 333011 [details]
Patch for landing
Comment on attachment 333011 [details] Patch for landing Clearing flags on attachment: 333011 Committed r228036: <https://trac.webkit.org/changeset/228036> All reviewed patches have been landed. Closing bug. |