Bug 76551 – Cleanup: Move chrome-specific filesystem type handling code (for FileSystem API) under chromium directory

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Kinuko Yasuda]

Move chrome-specific filesystem type handling code (for FileSystem API) under chromium directory.

The FileSystem API [1] draft specifies TEMPORARY and PERSISTENT filesystem types but current FileSystem API code under WebCore/fileapi also has definitions and code for chromium-specific EXTERNAL type.
We should probably move such code under platform/chromium.

[1] http://www.w3.org/TR/file-system-api/

[Kinuko Yasuda]

Created attachment 122953 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Kinuko Yasuda]

(In reply to comment #2)
> Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

Note for reviewers: the change in the public API part is only adding one-line comment.  Also all the changes are relatively mechanical as this is a cleanup patch.

[Darin Fisher (:fishd, Google)]

Comment on attachment 122953 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122953&action=review

> Source/WebCore/platform/AsyncFileSystem.h:66
> +    static const char kPersistentPathPrefix[];

I thought it was not WebKit style to use the kFoo naming scheme for constants.  Maybe you should not change it in this patch though as that would probably bloat the patch too much.

[Kinuko Yasuda]

(In reply to comment #4)
> (From update of attachment 122953 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122953&action=review
> 
> > Source/WebCore/platform/AsyncFileSystem.h:66
> > +    static const char kPersistentPathPrefix[];
> 
> I thought it was not WebKit style to use the kFoo naming scheme for constants.  Maybe you should not change it in this patch though as that would probably bloat the patch too much.

Thanks, I will fix it later separately.

[Kinuko Yasuda]

Committed r105395: <http://trac.webkit.org/changeset/105395>

[Sadrul Habib Chowdhury]

This changeset is causing failure in chromeos (browser_test:FileBrowserTest, and LocalFileSystem) :-(

[Kinuko Yasuda]

Sorry I have removed too much code for external filesystem.  Allow me to make another shot.

[Kinuko Yasuda]

Created attachment 123526 [details]
Patch

[Kinuko Yasuda]

Re-created the patch.  Changes from the previous one (that broke the chromeos build) are:

1. not escaping URLs returned by toURL() (as we didn't use to)
2. returning the URL synthesized by concatenating 'filesystem:' + origin + '/external/' + path rather than using the root URL returned by requestFileSystem for external fs, as it does not work for external fs (while I wanted to make this change for other types).

This time I've confirmed that this doesn't break the chromeos tests on trybot:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chromeos&number=4120
(We have other failures but they are unrelated)

Darin, (or other reviewers) sorry for bothering you again but could you PTAL?  Thanks.

[Kinuko Yasuda]

Comment on attachment 123526 [details]
Patch

Obsoleting this change for now as this is conflicting with other changes.

[Kinuko Yasuda]

Created attachment 124605 [details]
Patch

[Kinuko Yasuda]

Rebased.  Again this change is relatively mechanical.

I've confirmed that this patch runs ok with the ChromeOS build:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chromeos&number=4408

[David Levin]

Comment on attachment 124605 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124605&action=review

Looks ok, but there are a few things I don't understand. If we could clear those up, then I coudl r+.

> Source/WebKit/chromium/src/AsyncFileSystemChromium.cpp:94
> +    return type == Temporary || type == Persistent || type == static_cast<Type>(WebKit::WebFileSystem::TypeExternal);

How do you know that TypeExternal won't overlap in value with the other two?

> Source/WebKit/chromium/src/AsyncFileSystemChromium.cpp:117
> +        result.append(fullPath);

This was doing encodeWithURLEscapeSequence(fullPath) before. Why isn't that needed anymore?

> Source/WebKit/chromium/src/AsyncFileSystemChromium.cpp:123
> +    return virtualPathToFileSystemURL(fullPath);

It looks like virtualPathToFileSystemURL doesn't do what toURL did before. Why is this change ok?

[Kinuko Yasuda]

Comment on attachment 124605 [details]
Patch

Thanks, please find my response inline. Both of the points you asked have some subte point. I can add more comments or update the code if the code itself doesn't tell much.

View in context: https://bugs.webkit.org/attachment.cgi?id=124605&action=review

>> Source/WebKit/chromium/src/AsyncFileSystemChromium.cpp:94
>> +    return type == Temporary || type == Persistent || type == static_cast<Type>(WebKit::WebFileSystem::TypeExternal);
> 
> How do you know that TypeExternal won't overlap in value with the other two?

Good question.  The only place we have all the types defined is in WebFileSystem after this change and thus WebFileSystem::Type{Temporary, Persistent, External} should have all different values, and we have assertions for the two enums AsyncFileSystem::Type and WebFileSystem::Type* have same values for temporary and persistent.
(I'm a bit afraid if it's too subtle-- if so maybe we could keep AsyncFileSystem::External defined in non-chromium directories (but no code to handle it in the common code))

>> Source/WebKit/chromium/src/AsyncFileSystemChromium.cpp:117
>> +        result.append(fullPath);
> 
> This was doing encodeWithURLEscapeSequence(fullPath) before. Why isn't that needed anymore?

Oops, the encodeWithURLEscapeSequence() has been just added last week and this patch needs to be updated as well.  I'll update this to call it again.

>> Source/WebKit/chromium/src/AsyncFileSystemChromium.cpp:123
>> +    return virtualPathToFileSystemURL(fullPath);
> 
> It looks like virtualPathToFileSystemURL doesn't do what toURL did before. Why is this change ok?

The previous code was assuming both returns the same URL (since the URLs generated by toURL and virtualPathToFileSystemURL are handled by the same chrome code), so I thought it'd be good just to have the single implementation for them.

[David Levin]

(In reply to comment #15)
> (From update of attachment 124605 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124605&action=review
> 
> >> Source/WebKit/chromium/src/AsyncFileSystemChromium.cpp:94
> >> +    return type == Temporary || type == Persistent || type == static_cast<Type>(WebKit::WebFileSystem::TypeExternal);
> > 
> > How do you know that TypeExternal won't overlap in value with the other two?
> 
> Good question.  The only place we have all the types defined is in WebFileSystem after this change and thus WebFileSystem::Type{Temporary, Persistent, External} should have all different values, and we have assertions for the two enums AsyncFileSystem::Type and WebFileSystem::Type* have same values for temporary and persistent.

This is ok. Just making sure.

> 
> >> Source/WebKit/chromium/src/AsyncFileSystemChromium.cpp:123
> >> +    return virtualPathToFileSystemURL(fullPath);
> > 
> > It looks like virtualPathToFileSystemURL doesn't do what toURL did before. Why is this change ok?
> 
> The previous code was assuming both returns the same URL (since the URLs generated by toURL and virtualPathToFileSystemURL are handled by the same chrome code), so I thought it'd be good just to have the single implementation for them.

I wasn't clear or I don't understand. Here is the previous code for toURL: 

Look at the code path for Persistent:

64     result.append("filesystem:");
65     result.append(originString);
66     result.append("/");
67     switch (m_fileSystem->asyncFileSystem()->type()) {
68     case AsyncFileSystem::Temporary:
69         result.append(DOMFileSystemBase::kTemporaryPathPrefix);
70         break;
71     case AsyncFileSystem::Persistent:
72         result.append(DOMFileSystemBase::kPersistentPathPrefix);
73         break;
74     case AsyncFileSystem::External:
75         result.append(DOMFileSystemBase::kExternalPathPrefix);
76         break;
77     }
78     result.append(encodeWithURLEscapeSequences(m_fullPath));
79     return result.toString();

I should end up with
filesystem:[originString]/[DOMFileSystemBase::kPersistentPathPrefix][encodeWithURLEscapeSequences(m_fullPath]

With the new code I get,
[m_filesystemRootURL.path()][encodeWithURLEscapeSequences(m_fullPath.substring(1))]

Does m_filesystemRootURL.path() ==  filesystem:[originString]/[DOMFileSystemBase::kPersistentPathPrefix] ?

Seems like it couldn't because how would the origin string be in there.

Also, the new code lost this 
61       if (originString == "null")
62         return String();

[Kinuko Yasuda]

Comment on attachment 124605 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124605&action=review

>>>> Source/WebKit/chromium/src/AsyncFileSystemChromium.cpp:123
>>>> +    return virtualPathToFileSystemURL(fullPath);
>>> 
>>> It looks like virtualPathToFileSystemURL doesn't do what toURL did before. Why is this change ok?
>> 
>> The previous code was assuming both returns the same URL (since the URLs generated by toURL and virtualPathToFileSystemURL are handled by the same chrome code), so I thought it'd be good just to have the single implementation for them.
> 
> I wasn't clear or I don't understand. Here is the previous code for toURL: 
> 
> Look at the code path for Persistent:
> 
> 64     result.append("filesystem:");
> 65     result.append(originString);
> 66     result.append("/");
> 67     switch (m_fileSystem->asyncFileSystem()->type()) {
> 68     case AsyncFileSystem::Temporary:
> 69         result.append(DOMFileSystemBase::kTemporaryPathPrefix);
> 70         break;
> 71     case AsyncFileSystem::Persistent:
> 72         result.append(DOMFileSystemBase::kPersistentPathPrefix);
> 73         break;
> 74     case AsyncFileSystem::External:
> 75         result.append(DOMFileSystemBase::kExternalPathPrefix);
> 76         break;
> 77     }
> 78     result.append(encodeWithURLEscapeSequences(m_fullPath));
> 79     return result.toString();
> 
> I should end up with
> filesystem:[originString]/[DOMFileSystemBase::kPersistentPathPrefix][encodeWithURLEscapeSequences(m_fullPath]
> 
> With the new code I get,
> [m_filesystemRootURL.path()][encodeWithURLEscapeSequences(m_fullPath.substring(1))]
> 
> Does m_filesystemRootURL.path() ==  filesystem:[originString]/[DOMFileSystemBase::kPersistentPathPrefix] ?
> 
> Seems like it couldn't because how would the origin string be in there.
> 
> Also, the new code lost this 
> 61       if (originString == "null")
> 62         return String();

We're resetting the [m_filesystemRootURL.path()][encodeWithURLEscapeSequences(m_fullPath.substring(1))] as the resulting URL's path part, so the resulting URL will still have filesystem:[originString] part.

KURL url = m_filesystemRootURL;
url.setPath(...);

As for 'originString == "null"' case, thanks for catching it, I'll update the code.

[Kinuko Yasuda]

Created attachment 124650 [details]
Patch

Added origin==null check and encodeWithURLEscapeSequences in toURL().  Also added some more comments in the new toURL().

[David Levin]

Comment on attachment 124650 [details]
Patch

Thanks.

[Kinuko Yasuda]

Committed r106542: <http://trac.webkit.org/changeset/106542>