[fileapi] FileSystem type must be available to DOM code in order to implement Entry.toURI
Created attachment 80817 [details] Patch
This is a pre-condition for implementing Entry.toURI() as specified in http://dev.w3.org/2009/dap/file-system/file-dir-sys.html.
Looks reasonable. The info's a little redundant in the current Chromium implementation [it's in the root path as well], but we don't want to assume that, and I'd like to take out the root path anyway in favor of a more generic identifier later.
Created attachment 80849 [details] Patch
CCing abarth to get his thoughts on the SecurityOrigin changes. I've now included everything together in one patch: it didn't end up being too big, and I think it makes everything make more sense.
Did you mean to mark the patch "review?"
So marked. There are more tests to add, I think, but yes, review would probably be useful at this point.
Comment on attachment 80849 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=80849&action=review > Source/WebCore/fileapi/FileSystemURL.cpp:66 > +KURL FileSystemURL::getOrigin(const KURL& url) > +{ > + ASSERT(url.protocolIs(kFileSystemProtocol)); > + > + KURL originUrl(ParsedURLString, url.path()); > + originUrl.setPath("/"); > + return originUrl; > +} The types on this function don't look right. An "origin" is a SecurityOrigin, not a KURL... Also, we've been centralizing all the knowledge about how to compute SecurityOrigins from URLs in the SecurityOrigin class. > Source/WebCore/fileapi/FileSystemURL.h:53 > +class FileSystemURL { > +public: > + static String createURLString(SecurityOrigin*, AsyncFileSystem::Type, const String& fullPath); > + static KURL getOrigin(const KURL&); > + static const char* fileSystemProtocol() { return kFileSystemProtocol; } > + > +private: > + static const char kFileSystemProtocol[]; > + FileSystemURL() { } > +}; Generally, we don't create classes that have all static members. What does a FileSystemURL represent? How is that different from a KURL? > Source/WebCore/page/SecurityOrigin.cpp:68 > +// FIXME: Should this be part of SchemeRegistry? Yes. > Source/WebCore/page/SecurityOrigin.cpp:69 > +static bool schemeRequiresRequestCapabilityForDisplay(const String& scheme) We probably don't want to use the term "Capability" here because Capability has a special meaning in security, and this doesn't quite match that meaning. > Source/WebCore/page/SecurityOrigin.cpp:152 > return adoptRef(new SecurityOrigin(BlobURL::getOrigin(url), sandboxFlags)); I see that you're copying the pattern here from BlobURL, but that's not a good pattern. We should fix Blob URL not to use this style either.
Comment on attachment 80849 [details] Patch The SecurityOrigin-related bits of this patch generally look good, but we should iterate on the style issues.
Comment on attachment 80849 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=80849&action=review >> Source/WebCore/fileapi/FileSystemURL.h:53 >> +}; > > Generally, we don't create classes that have all static members. What does a FileSystemURL represent? How is that different from a KURL? As you guessed, this is simply mirroring the code layout of BlobURL. createURLString() should be able to move entirely to Entry.cpp (it's not called elsewhere). See my question in SecurityOrigin.cpp about what I should do with getOrigin(). >> Source/WebCore/page/SecurityOrigin.cpp:68 >> +// FIXME: Should this be part of SchemeRegistry? > > Yes. Will move once we've agreed on a name. >> Source/WebCore/page/SecurityOrigin.cpp:69 >> +static bool schemeRequiresRequestCapabilityForDisplay(const String& scheme) > > We probably don't want to use the term "Capability" here because Capability has a special meaning in security, and this doesn't quite match that meaning. I disliked this name too; do you have an alternate suggestion? SchemeRegistry::shouldTreatURLSchemeDisplayAsRequestRestricted? More words doesn't necessarily make it clearer to me. >> Source/WebCore/page/SecurityOrigin.cpp:152 >> return adoptRef(new SecurityOrigin(BlobURL::getOrigin(url), sandboxFlags)); > > I see that you're copying the pattern here from BlobURL, but that's not a good pattern. We should fix Blob URL not to use this style either. Do you have a pattern in mind? Should the code that's currently in FileSystemURL::getOrigin() just move into the SecurityOrigin constructor?
Comment on attachment 80849 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=80849&action=review >>> Source/WebCore/page/SecurityOrigin.cpp:69 >>> +static bool schemeRequiresRequestCapabilityForDisplay(const String& scheme) >> >> We probably don't want to use the term "Capability" here because Capability has a special meaning in security, and this doesn't quite match that meaning. > > I disliked this name too; do you have an alternate suggestion? SchemeRegistry::shouldTreatURLSchemeDisplayAsRequestRestricted? More words doesn't necessarily make it clearer to me. Yeah, all these functions are poorly named. Maybe SchemeRegistry::canDisplayOnlyIfCanRequest ? >>> Source/WebCore/page/SecurityOrigin.cpp:152 >>> return adoptRef(new SecurityOrigin(BlobURL::getOrigin(url), sandboxFlags)); >> >> I see that you're copying the pattern here from BlobURL, but that's not a good pattern. We should fix Blob URL not to use this style either. > > Do you have a pattern in mind? Should the code that's currently in FileSystemURL::getOrigin() just move into the SecurityOrigin constructor? Yeah, or just into a free function in an anonymous namespace in SecurityOrigin.cpp.
Created attachment 80867 [details] Removed FileSystemURL, moved some code to SchemeRegistry
Comment on attachment 80867 [details] Removed FileSystemURL, moved some code to SchemeRegistry View in context: https://bugs.webkit.org/attachment.cgi?id=80867&action=review > Source/WebCore/fileapi/Entry.cpp:95 > + String uri = "filesystem:"; > + uri += filesystem()->scriptExecutionContext()->securityOrigin()->toString(); > + uri += "/"; > + uri += (m_fileSystem->asyncFileSystem()->type() == AsyncFileSystem::Temporary ? "temporary" : "persistent"); > + uri += m_fullPath; > + return uri; Consider using StringBuilder instead of +=. That should save a bunch of allocations. > Source/WebCore/page/SecurityOrigin.cpp:85 > + if (originURL.isValid()) { If the originURL isn't valid, will we trigger the "m_isUnique = true" assignment on line 95? If not, we should probably do that explicitly. > Source/WebCore/page/SecurityOrigin.cpp:88 > + m_port = originURL.port(); Do we need to do something with default ports, or will that happen automagically for us?
Created attachment 80923 [details] Use StringBuilder and set m_isUnique for invalid URLs
Comment on attachment 80867 [details] Removed FileSystemURL, moved some code to SchemeRegistry View in context: https://bugs.webkit.org/attachment.cgi?id=80867&action=review >> Source/WebCore/fileapi/Entry.cpp:95 >> + return uri; > > Consider using StringBuilder instead of +=. That should save a bunch of allocations. Done. I was thinking std::string, now I know how to treat WTF::String. >> Source/WebCore/page/SecurityOrigin.cpp:85 >> + if (originURL.isValid()) { > > If the originURL isn't valid, will we trigger the "m_isUnique = true" assignment on line 95? If not, we should probably do that explicitly. Added my own explicit setting in the else branch (the other one wasn't going to trigger). >> Source/WebCore/page/SecurityOrigin.cpp:88 >> + m_port = originURL.port(); > > Do we need to do something with default ports, or will that happen automagically for us? That'll be handled by the last if statement in the constructor. It worries me a little that the ordering is so important here, but it's still short enough to fit on one screen.
Comment on attachment 80923 [details] Use StringBuilder and set m_isUnique for invalid URLs View in context: https://bugs.webkit.org/attachment.cgi?id=80923&action=review The SecurityOrigin-related parts of this patch look great! I'll let someone else review the filesystem parts. > Source/WebCore/fileapi/Entry.idl:46 > + DOMString toURI(); Its too bad the web platform is so inconsistent w.r.t URL and URI. Not really a problem with this patch, just rambling.
Created attachment 80975 [details] Adds test to exercise canDisplay
Comment on attachment 80975 [details] Adds test to exercise canDisplay View in context: https://bugs.webkit.org/attachment.cgi?id=80975&action=review > LayoutTests/fast/filesystem/filesystem-uri-origin.html:1 > +<a style="display:none" href="filesystem:http://www.webkit.org:8080/temporary/a/b/c/file.txt">foo</a> Here you have webkit.org as the filesystem origin. In directory-entry-to-uri-expected.txt you have file:// as the origin. In an iframe test I see http://localhost:8000. Please make sure these tests will work both on the webkit bots and when run locally. > Source/WebCore/fileapi/Entry.cpp:95 > + uriBuilder.append(m_fileSystem->asyncFileSystem()->type() == AsyncFileSystem::Temporary ? "temporary" : "persistent"); Do the strings "temporary" and "persistent" exist anywhere as constants? It would be nice not to have them inline here. At the very least they should be shared with the code that cracks the URI.
Comment on attachment 80975 [details] Adds test to exercise canDisplay View in context: https://bugs.webkit.org/attachment.cgi?id=80975&action=review >> Source/WebCore/fileapi/Entry.cpp:95 >> + uriBuilder.append(m_fileSystem->asyncFileSystem()->type() == AsyncFileSystem::Temporary ? "temporary" : "persistent"); > > Do the strings "temporary" and "persistent" exist anywhere as constants? It would be nice not to have them inline here. At the very least they should be shared with the code that cracks the URI. Same for the protocol name "filesystem"? The changes in FileSystem callback chains to include type parameter lgtm.
Comment on attachment 80975 [details] Adds test to exercise canDisplay View in context: https://bugs.webkit.org/attachment.cgi?id=80975&action=review >> LayoutTests/fast/filesystem/filesystem-uri-origin.html:1 >> +<a style="display:none" href="filesystem:http://www.webkit.org:8080/temporary/a/b/c/file.txt">foo</a> > > Here you have webkit.org as the filesystem origin. In directory-entry-to-uri-expected.txt you have file:// as the origin. In an iframe test I see http://localhost:8000. Please make sure these tests will work both on the webkit bots and when run locally. The file:/// URIs are there to avoid needing to be an http test. Would you rather I use http tests for those toURI() tests? The iframe test uses localhost to distinguish it from 127.0.0.1 (where the hosted page is running). The use of webkit.org:8080 here is purely ornamental, since this URI is never followed; it's simply being used to set up the dom so that the origin is available. Changing this to localhost or similar wouldn't change anything... >>> Source/WebCore/fileapi/Entry.cpp:95 >>> + uriBuilder.append(m_fileSystem->asyncFileSystem()->type() == AsyncFileSystem::Temporary ? "temporary" : "persistent"); >> >> Do the strings "temporary" and "persistent" exist anywhere as constants? It would be nice not to have them inline here. At the very least they should be shared with the code that cracks the URI. > > Same for the protocol name "filesystem"? > > The changes in FileSystem callback chains to include type parameter lgtm. Any suggestions about where these strings would live? Right now, the URI cracking code is in Chromium. Should it move into WebCore?
(In reply to comment #20) > (From update of attachment 80975 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=80975&action=review > > >> LayoutTests/fast/filesystem/filesystem-uri-origin.html:1 > >> +<a style="display:none" href="filesystem:http://www.webkit.org:8080/temporary/a/b/c/file.txt">foo</a> > > > > Here you have webkit.org as the filesystem origin. In directory-entry-to-uri-expected.txt you have file:// as the origin. In an iframe test I see http://localhost:8000. Please make sure these tests will work both on the webkit bots and when run locally. > > The file:/// URIs are there to avoid needing to be an http test. Would you rather I use http tests for those toURI() tests? Nope; just wanted to make sure that it was all going to work. That's fine. > The iframe test uses localhost to distinguish it from 127.0.0.1 (where the hosted page is running). > > The use of webkit.org:8080 here is purely ornamental, since this URI is never followed; it's simply being used to set up the dom so that the origin is available. Changing this to localhost or similar wouldn't change anything... > > >>> Source/WebCore/fileapi/Entry.cpp:95 > >>> + uriBuilder.append(m_fileSystem->asyncFileSystem()->type() == AsyncFileSystem::Temporary ? "temporary" : "persistent"); > >> > >> Do the strings "temporary" and "persistent" exist anywhere as constants? It would be nice not to have them inline here. At the very least they should be shared with the code that cracks the URI. > > > > Same for the protocol name "filesystem"? > > > > The changes in FileSystem callback chains to include type parameter lgtm. > > Any suggestions about where these strings would live? Right now, the URI cracking code is in Chromium. Should it move into WebCore? Hmm...tricky, since it deals with security origin code. Let's at least make sure that the strings don't exist in more than one place for chromium or more than one place for webkit. Perhaps for WebCore it should be in AsyncFileSystem, along with Type? Kinuko, what do you think?
I actually had a followup question about tests: these clearly won't work without FILE_SYSTEM being enabled. Is there anything I need to do to mark them as ignored/disabled when run outside the chromium configuration?
(In reply to comment #22) > I actually had a followup question about tests: these clearly won't work without FILE_SYSTEM being enabled. Is there anything I need to do to mark them as ignored/disabled when run outside the chromium configuration? Yeah, you can add them to the Skipped lists for ports that don't support this feature. For example, LayoutTests/platform/mac/Skipped
I'm pretty certain the fast/filesystem directory is already in those skip lists so that individual tests need not be added.
I've also added an http test, which will need to be skipped. I've added it locally, will update the patch after the next round of comments is done.
Any further comments? Either about where to put the string constants, or anything else?
Comment on attachment 80975 [details] Adds test to exercise canDisplay Again this generally looks good to me (as for changes in FileSystem API). A few more cosmetic comments: View in context: https://bugs.webkit.org/attachment.cgi?id=80975&action=review > Source/WebCore/ChangeLog:6 > + https://bugs.webkit.org/show_bug.cgi?id=53529 It might expect to see a few more lines that explain what changes are to be made by this patch. (Also some of the individual files/changes listed below may deserve brief one-liner comments?) >>>>> Source/WebCore/fileapi/Entry.cpp:95 >>>>> + uriBuilder.append(m_fileSystem->asyncFileSystem()->type() == AsyncFileSystem::Temporary ? "temporary" : "persistent"); >>>> >>>> Do the strings "temporary" and "persistent" exist anywhere as constants? It would be nice not to have them inline here. At the very least they should be shared with the code that cracks the URI. >>> >>> Same for the protocol name "filesystem"? >>> >>> The changes in FileSystem callback chains to include type parameter lgtm. >> >> Any suggestions about where these strings would live? Right now, the URI cracking code is in Chromium. Should it move into WebCore? > > Hmm...tricky, since it deals with security origin code. Let's at least make sure that the strings don't exist in more than one place for chromium or more than one place for webkit. Perhaps for WebCore it should be in AsyncFileSystem, along with Type? Kinuko, what do you think? (Sorry I thought I had commented on this but seems like I failed to hit 'publish' button!) The URI format will be on the spec right? Then I think it's ok to construct the URI in WebCore and crack it in chromium (i.e. in embedder's code). For WebCore side having constants in AsyncFileSystem sounds good to me.
Comment on attachment 80975 [details] Adds test to exercise canDisplay View in context: https://bugs.webkit.org/attachment.cgi?id=80975&action=review >> Source/WebCore/ChangeLog:6 >> + https://bugs.webkit.org/show_bug.cgi?id=53529 > > It might expect to see a few more lines that explain what changes are to be made by this patch. > (Also some of the individual files/changes listed below may deserve brief one-liner comments?) Updated all the ChangeLogs with more info, thanks for the suggestion (this is my first real (non-cleanup) WebKit patch) >>>>>> Source/WebCore/fileapi/Entry.cpp:95 >>>>>> + uriBuilder.append(m_fileSystem->asyncFileSystem()->type() == AsyncFileSystem::Temporary ? "temporary" : "persistent"); >>>>> >>>>> Do the strings "temporary" and "persistent" exist anywhere as constants? It would be nice not to have them inline here. At the very least they should be shared with the code that cracks the URI. >>>> >>>> Same for the protocol name "filesystem"? >>>> >>>> The changes in FileSystem callback chains to include type parameter lgtm. >>> >>> Any suggestions about where these strings would live? Right now, the URI cracking code is in Chromium. Should it move into WebCore? >> >> Hmm...tricky, since it deals with security origin code. Let's at least make sure that the strings don't exist in more than one place for chromium or more than one place for webkit. Perhaps for WebCore it should be in AsyncFileSystem, along with Type? Kinuko, what do you think? > > (Sorry I thought I had commented on this but seems like I failed to hit 'publish' button!) > > The URI format will be on the spec right? Then I think it's ok to construct the URI in WebCore and crack it in chromium (i.e. in embedder's code). For WebCore side having constants in AsyncFileSystem sounds good to me. Hmm, the trouble with putting these in AsyncFileSystem is that there are already two implementations of AsyncFileSystem, so there's not a single place on which to hang const char[] string constants. WebCore/platform/AsyncFileSystem.cpp is not used by Chromium; we use WebKit/chromium/src/AsyncFileSystemChromium.cpp. And in fact, AsyncFileSystem already has this duplicated string constant problem: AsyncFileSystem.cpp uses the strings "Temporary" and "Persistent" (part of the filesystem name), while in Chromium, those live in webkit/fileapi/file_system_path_manager.cc. In short, I'm not sure what's to be gained from moving these constants out of Entry.cpp and into two other CPP files, given that this is the only reference in all of WebKit. Is there another place I'm missing where they would be used? That might make it clearer where they should be (if they do need centralization).
Created attachment 81663 [details] Patch
Comment on attachment 81663 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=81663&action=review > Source/WebCore/fileapi/Entry.cpp:97 > + return uriBuilder.toString(); Somehow it didn't come to me up until now, but shouldn't we put this in EntryBase (base class of Entry and EntrySync) rather than in Entry? The spec seems to have toURI method also in EntrySync. http://dev.w3.org/2009/dap/file-system/file-dir-sys.html#widl-EntrySync-toURI And as for string constants, you're right about AsyncFileSystem (its .cpp is not referred in chromium build). In addition to the fact you pointed out, the URI stuff is not platform dependent stuff but spec based fact, so we should put them in WebCore/fileapi as a common implementation. If we're going to put this code in EntryBase, I'd think about putting the constants in DOMFileSystemBase.{h,cpp}. Or if we're sure we won't use the constants from anywhere else in webcore, it'd be also ok to leave them as literals. So my take is to put them in DOMFileSystemBase or just leave them as is. Thanks for taking your extra time on this issue!
(In reply to comment #30) > (From update of attachment 81663 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=81663&action=review > > > Source/WebCore/fileapi/Entry.cpp:97 > > + return uriBuilder.toString(); > > Somehow it didn't come to me up until now, but shouldn't we put this in EntryBase (base class of Entry and EntrySync) rather than in Entry? > The spec seems to have toURI method also in EntrySync. > http://dev.w3.org/2009/dap/file-system/file-dir-sys.html#widl-EntrySync-toURI Hooking this up to EntrySync will require a bit more work, since neither EntryBase nor EntrySync currently have access to the scriptExecutionContext->securityOrigin(). Getting EntryBase access to that would make this patch significantly bigger, I fear. Might it make sense to do that in a followup? > And as for string constants, you're right about AsyncFileSystem (its .cpp is not referred in chromium build). In addition to the fact you pointed out, the URI stuff is not platform dependent stuff but spec based fact, so we should put them in WebCore/fileapi as a common implementation. > If we're going to put this code in EntryBase, I'd think about putting the constants in DOMFileSystemBase.{h,cpp}. > Or if we're sure we won't use the constants from anywhere else in webcore, it'd be also ok to leave them as literals. > So my take is to put them in DOMFileSystemBase or just leave them as is. > Thanks for taking your extra time on this issue!
Comment on attachment 81663 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=81663&action=review >>> Source/WebCore/fileapi/Entry.cpp:97 >>> + return uriBuilder.toString(); >> >> Somehow it didn't come to me up until now, but shouldn't we put this in EntryBase (base class of Entry and EntrySync) rather than in Entry? >> The spec seems to have toURI method also in EntrySync. >> http://dev.w3.org/2009/dap/file-system/file-dir-sys.html#widl-EntrySync-toURI >> >> And as for string constants, you're right about AsyncFileSystem (its .cpp is not referred in chromium build). In addition to the fact you pointed out, the URI stuff is not platform dependent stuff but spec based fact, so we should put them in WebCore/fileapi as a common implementation. >> If we're going to put this code in EntryBase, I'd think about putting the constants in DOMFileSystemBase.{h,cpp}. >> Or if we're sure we won't use the constants from anywhere else in webcore, it'd be also ok to leave them as literals. >> So my take is to put them in DOMFileSystemBase or just leave them as is. >> Thanks for taking your extra time on this issue! > > Hooking this up to EntrySync will require a bit more work, since neither EntryBase nor EntrySync currently have access to the scriptExecutionContext->securityOrigin(). Getting EntryBase access to that would make this patch significantly bigger, I fear. Might it make sense to do that in a followup? I see, yes it makes sense. Let's do it separately.
Comment on attachment 81663 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=81663&action=review > Source/WebCore/page/SecurityOrigin.cpp:86 > + m_protocol = originURL.protocol().lower(); I suspect that these calls to .lower() may be redundant. Doesn't KURL canonicalize the protocol and host to lowercase?
> Doesn't KURL canonicalize the protocol and host to lowercase? As of about two days ago, yes. Before that, no. Let's keep the toLower for a bit while we make sure that change sticks.
Comment on attachment 81663 [details] Patch OK, makes sense. R=me
Comment on attachment 81663 [details] Patch Rejecting attachment 81663 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'build-..." exit_code: 2 Last 500 characters of output: ...................................................................................................................................................................... compositing ........... compositing/animation ... compositing/color-matching .. compositing/geometry ...... compositing/geometry/empty-embed-rects.html -> crashed Exiting early after 1 failures. 962 tests run. 103.11s total testing time 961 test cases (99%) succeeded 1 test case (<1%) crashed 2 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/7886413
Looks like a flaky crash...Darin, can you resubmit to the commit queue?
Comment on attachment 81663 [details] Patch suspicious.
Comment on attachment 81663 [details] Patch Rejecting attachment 81663 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'build-..." exit_code: 2 Last 500 characters of output: ...................................................................................................................................................................... compositing ........... compositing/animation ... compositing/color-matching .. compositing/geometry ...... compositing/geometry/empty-embed-rects.html -> crashed Exiting early after 1 failures. 962 tests run. 103.00s total testing time 961 test cases (99%) succeeded 1 test case (<1%) crashed 2 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/7886508
(In reply to comment #37) > Looks like a flaky crash...Darin, can you resubmit to the commit queue? The commit-queue should be robust to flaky crashes... Have you tried to reproduce locally?
The commit-queue never lies. :) I'm certain this is a crash from this patch.
Created attachment 82163 [details] Patch for commit, no layout test crashes
I see this diff between the patches: bool SchemeRegistry::canDisplayOnlyIfCanRequest(const String& scheme) { + if (scheme.isEmpty()) + return false; return canDisplayOnlyIfCanRequestSchemes().contains(scheme); } LGTM!
Indeed the commit queue was right. Very little code in this change exists without ENABLE_FILE_SYSTEM, but the bit that does was crashing when called with an empty scheme (as we apparently do for <embed>s). I've fixed that in this change, and also sent out https://bugs.webkit.org/show_bug.cgi?id=54304 to add safety checks to the rest of the methods in SchemeRegistry.
Comment on attachment 82163 [details] Patch for commit, no layout test crashes Clearing flags on attachment: 82163 Committed r78362: <http://trac.webkit.org/changeset/78362>
All reviewed patches have been landed. Closing bug.