Summary: | Cleanup: Move chrome-specific filesystem type handling code (for FileSystem API) under chromium directory | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kinuko Yasuda <kinuko> | ||||||||||
Component: | Platform | Assignee: | Kinuko Yasuda <kinuko> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | ericu, fishd, levin, sadrul, webkit.review.bot, zelidrag | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Kinuko Yasuda
2012-01-18 09:37:45 PST
Created attachment 122953 [details]
Patch
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API. (In reply to comment #2) > Please wait for approval from fishd@chromium.org 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. 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. (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. Committed r105395: <http://trac.webkit.org/changeset/105395> This changeset is causing failure in chromeos (browser_test:FileBrowserTest, and LocalFileSystem) :-( Sorry I have removed too much code for external filesystem. Allow me to make another shot. Created attachment 123526 [details]
Patch
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. Comment on attachment 123526 [details]
Patch
Obsoleting this change for now as this is conflicting with other changes.
Created attachment 124605 [details]
Patch
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 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? 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. (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(); 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. Created attachment 124650 [details]
Patch
Added origin==null check and encodeWithURLEscapeSequences in toURL(). Also added some more comments in the new toURL().
Comment on attachment 124650 [details]
Patch
Thanks.
Committed r106542: <http://trac.webkit.org/changeset/106542> |