Bug 54585 – [fileapi] Implement EntrySync.toURI by moving Entry::toURI to EntryBase

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Adam Klein]

[fileapi] Implement EntrySync.toURI by moving Entry::toURI to EntryBase

[Adam Klein]

Created attachment 82689 [details]
Patch

[Adam Klein]

Still trying to get the added test to pass; if you have any hints about worker tests, let me know.

[Adam Klein]

Apparently none of the worker tests are currently able to run, so this should be fully ready for review.

[Eric U.]

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.

[Adam Klein]

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?

[Eric U.]

(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.

[Kinuko Yasuda]

Comment on attachment 82689 [details]
Patch

lgtm.

[Adam Barth]

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

[Adam Klein]

Created attachment 83337 [details]
Patch for commit queue

[Adam Klein]

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.

[WebKit Commit Bot]

Comment on attachment 83337 [details]
Patch for commit queue

Clearing flags on attachment: 83337

Committed r79778: <http://trac.webkit.org/changeset/79778>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.