[Webkit-unassigned] [Bug 53529] [fileapi] Add support for filesystem: URI handling

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=53529





--- Comment #27 from Kinuko Yasuda <kinuko at chromium.org>  2011-02-07 19:47:29 PST ---
(From update of attachment 80975)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.