[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 #28 from Adam Klein <adamk at chromium.org>  2011-02-08 10:53:03 PST ---
(From update of attachment 80975)
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).

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