[fileapi] Implement EntrySync.toURI by moving Entry::toURI to EntryBase
Created attachment 82689 [details] Patch
Still trying to get the added test to pass; if you have any hints about worker tests, let me know.
Apparently none of the worker tests are currently able to run, so this should be fully ready for review.
Comment on attachment 82689 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=82689&action=review LGTM otherwise. > LayoutTests/fast/filesystem/resources/file-entry-to-uri-sync.js:11 > +shouldBe("testFileEntry.toURI()", "'filesystem:file:///temporary/testFileEntry.txt'"); Are we forcing all URLs to start with three slashes, then--two for the protocol, and one for "it's from the root"? It seems like the third one should be superfluous, as the root is implied. I realize this is beyond the scope of this CL, but apparently I missed it in the last one.
Comment on attachment 82689 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=82689&action=review >> LayoutTests/fast/filesystem/resources/file-entry-to-uri-sync.js:11 >> +shouldBe("testFileEntry.toURI()", "'filesystem:file:///temporary/testFileEntry.txt'"); > > Are we forcing all URLs to start with three slashes, then--two for the protocol, and one for "it's from the root"? It seems like the third one should be superfluous, as the root is implied. I realize this is beyond the scope of this CL, but apparently I missed it in the last one. I think the three slashes in a row here are due simply to the way the file: URI scheme works (and how SecurityOrigin::toString deals with it). The first two slashes are part of the security origin: "file://". The third slash separates the origin from the filesystem type ("permanent" or "temporary"). Without this, "normal" http-based filesystme URIs would be unparseable: that last slash keeps these from reading "http://www.google.comtemporary/foo/bar.txt". It just so happens that the "host" part of the origin is empty for file: URIs. Does that make sense? Or am I missing your point?
(In reply to comment #5) > (From update of attachment 82689 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=82689&action=review > > >> LayoutTests/fast/filesystem/resources/file-entry-to-uri-sync.js:11 > >> +shouldBe("testFileEntry.toURI()", "'filesystem:file:///temporary/testFileEntry.txt'"); > > > > Are we forcing all URLs to start with three slashes, then--two for the protocol, and one for "it's from the root"? It seems like the third one should be superfluous, as the root is implied. I realize this is beyond the scope of this CL, but apparently I missed it in the last one. > > I think the three slashes in a row here are due simply to the way the file: URI scheme works (and how SecurityOrigin::toString deals with it). The first two slashes are part of the security origin: "file://". The third slash separates the origin from the filesystem type ("permanent" or "temporary"). Without this, "normal" http-based filesystme URIs would be unparseable: that last slash keeps these from reading "http://www.google.comtemporary/foo/bar.txt". > > It just so happens that the "host" part of the origin is empty for file: URIs. Does that make sense? Or am I missing your point? Oh, OK--nevermind then. Looks like it's not a problem.
Comment on attachment 82689 [details] Patch lgtm.
Comment on attachment 82689 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=82689&action=review > Source/WebCore/fileapi/DOMFileSystem.cpp:54 > - : DOMFileSystemBase(name, asyncFileSystem) > + : DOMFileSystemBase(context, name, asyncFileSystem) > , ActiveDOMObject(context, this) It's too bad we end up with two pointers to the script execution context, but I don't see a way around it. > Source/WebCore/fileapi/EntryBase.cpp:59 > + StringBuilder uriBuilder; nit: I would have just called this "result" or something. > Source/WebCore/fileapi/EntryBase.cpp:63 > + uriBuilder.append(m_fileSystem->asyncFileSystem()->type() == AsyncFileSystem::Temporary ? "temporary" : "persistent"); These should use the constants, but I guess that hasn't landed yet
Created attachment 83337 [details] Patch for commit queue
Comment on attachment 82689 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=82689&action=review >> Source/WebCore/fileapi/DOMFileSystem.cpp:54 >> , ActiveDOMObject(context, this) > > It's too bad we end up with two pointers to the script execution context, but I don't see a way around it. Yeah, given the way the type hierarchy works, I don't see a better way. I could just hold onto a ScriptOrigin*, but that strikes me as conceptually worse. >> Source/WebCore/fileapi/EntryBase.cpp:59 >> + StringBuilder uriBuilder; > > nit: I would have just called this "result" or something. Fixed, agreed this name is better. Wish StringBuilder was a bit more like the Java version and allowed me to chain. >> Source/WebCore/fileapi/EntryBase.cpp:63 >> + uriBuilder.append(m_fileSystem->asyncFileSystem()->type() == AsyncFileSystem::Temporary ? "temporary" : "persistent"); > > These should use the constants, but I guess that hasn't landed yet Will use the constants once the other 54774 lands; for now I just want to avoid a merge conflict between the two patches.
Comment on attachment 83337 [details] Patch for commit queue Clearing flags on attachment: 83337 Committed r79778: <http://trac.webkit.org/changeset/79778>
All reviewed patches have been landed. Closing bug.