Bug 53529 - [fileapi] Add support for filesystem: URI handling
Summary: [fileapi] Add support for filesystem: URI handling
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-01 13:38 PST by Adam Klein
Modified: 2011-02-11 13:39 PST (History)
8 users (show)

See Also:


Attachments
Patch (14.05 KB, patch)
2011-02-01 13:39 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Patch (34.53 KB, patch)
2011-02-01 16:16 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Removed FileSystemURL, moved some code to SchemeRegistry (29.47 KB, patch)
2011-02-01 18:12 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Use StringBuilder and set m_isUnique for invalid URLs (29.64 KB, patch)
2011-02-02 09:39 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Adds test to exercise canDisplay (33.45 KB, patch)
2011-02-02 14:53 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Patch (36.97 KB, patch)
2011-02-08 10:53 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Patch for commit, no layout test crashes (37.06 KB, patch)
2011-02-11 12:57 PST, Adam Klein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Klein 2011-02-01 13:38:05 PST
[fileapi] FileSystem type must be available to DOM code in order to implement Entry.toURI
Comment 1 Adam Klein 2011-02-01 13:39:48 PST
Created attachment 80817 [details]
Patch
Comment 2 Adam Klein 2011-02-01 13:41:32 PST
This is a pre-condition for implementing Entry.toURI() as specified in http://dev.w3.org/2009/dap/file-system/file-dir-sys.html.
Comment 3 Eric U. 2011-02-01 14:01:39 PST
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.
Comment 4 Adam Klein 2011-02-01 16:16:23 PST
Created attachment 80849 [details]
Patch
Comment 5 Adam Klein 2011-02-01 16:18:44 PST
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.
Comment 6 Adam Barth 2011-02-01 16:21:14 PST
Did you mean to mark the patch "review?"
Comment 7 Adam Klein 2011-02-01 16:23:14 PST
So marked. There are more tests to add, I think, but yes, review would probably be useful at this point.
Comment 8 Adam Barth 2011-02-01 16:27:24 PST
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 9 Adam Barth 2011-02-01 16:28:20 PST
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 10 Adam Klein 2011-02-01 16:40:07 PST
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 11 Adam Barth 2011-02-01 16:55:22 PST
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.
Comment 12 Adam Klein 2011-02-01 18:12:06 PST
Created attachment 80867 [details]
Removed FileSystemURL, moved some code to SchemeRegistry
Comment 13 Adam Barth 2011-02-01 19:54:06 PST
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?
Comment 14 Adam Klein 2011-02-02 09:39:48 PST
Created attachment 80923 [details]
Use StringBuilder and set m_isUnique for invalid URLs
Comment 15 Adam Klein 2011-02-02 09:42:49 PST
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 16 Adam Barth 2011-02-02 13:35:18 PST
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.
Comment 17 Adam Klein 2011-02-02 14:53:41 PST
Created attachment 80975 [details]
Adds test to exercise canDisplay
Comment 18 Eric U. 2011-02-02 16:53:38 PST
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 19 Kinuko Yasuda 2011-02-02 17:32:48 PST
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 20 Adam Klein 2011-02-02 17:38:04 PST
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?
Comment 21 Eric U. 2011-02-02 17:59:13 PST
(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?
Comment 22 Adam Klein 2011-02-03 14:07:47 PST
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?
Comment 23 Adam Barth 2011-02-03 14:14:46 PST
(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
Comment 24 Michael Nordman 2011-02-03 14:20:48 PST
I'm pretty certain the fast/filesystem directory is already in those skip lists so that individual tests need not be added.
Comment 25 Adam Klein 2011-02-03 14:22:49 PST
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.
Comment 26 Adam Klein 2011-02-07 17:17:48 PST
Any further comments?  Either about where to put the string constants, or anything else?
Comment 27 Kinuko Yasuda 2011-02-07 19:47:29 PST
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 28 Adam Klein 2011-02-08 10:53:03 PST
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).
Comment 29 Adam Klein 2011-02-08 10:53:50 PST
Created attachment 81663 [details]
Patch
Comment 30 Kinuko Yasuda 2011-02-08 18:40:05 PST
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!
Comment 31 Adam Klein 2011-02-09 10:45:46 PST
(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 32 Kinuko Yasuda 2011-02-09 16:05:36 PST
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 33 Darin Fisher (:fishd, Google) 2011-02-09 22:44:07 PST
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?
Comment 34 Adam Barth 2011-02-10 00:53:50 PST
> 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 35 Darin Fisher (:fishd, Google) 2011-02-10 10:10:36 PST
Comment on attachment 81663 [details]
Patch

OK, makes sense.  R=me
Comment 36 WebKit Commit Bot 2011-02-10 12:57:37 PST
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
Comment 37 Adam Klein 2011-02-10 15:36:25 PST
Looks like  a flaky crash...Darin, can you resubmit to the commit queue?
Comment 38 Eric Seidel (no email) 2011-02-10 15:37:56 PST
Comment on attachment 81663 [details]
Patch

suspicious.
Comment 39 WebKit Commit Bot 2011-02-10 18:44:35 PST
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
Comment 40 Adam Barth 2011-02-10 18:57:37 PST
(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?
Comment 41 Eric Seidel (no email) 2011-02-10 21:33:04 PST
The commit-queue never lies. :)  I'm certain this is a crash from this patch.
Comment 42 Adam Klein 2011-02-11 12:57:21 PST
Created attachment 82163 [details]
Patch for commit, no layout test crashes
Comment 43 Darin Fisher (:fishd, Google) 2011-02-11 13:12:21 PST
I see this diff between the patches:

 bool SchemeRegistry::canDisplayOnlyIfCanRequest(const String& scheme)
 {
+    if (scheme.isEmpty())
+        return false;
     return canDisplayOnlyIfCanRequestSchemes().contains(scheme);
 }

LGTM!
Comment 44 Adam Klein 2011-02-11 13:13:13 PST
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 45 WebKit Commit Bot 2011-02-11 13:39:19 PST
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>
Comment 46 WebKit Commit Bot 2011-02-11 13:39:27 PST
All reviewed patches have been landed.  Closing bug.