[chromium] Add isolated filesystem type and WebDragData::filesystem_id for drag-and-drop using File/DirectoryEntry.
Created attachment 134071 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 134071 [details] Patch Attachment 134071 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12142577
Created attachment 134087 [details] Patch Fixed build.
Daniel, do you want to review the changes to WebDragData?
Comment on attachment 134087 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134087&action=review WebDragData changes look good to me. However, do we want to wrap the new WebDragData methods in #if ENABLE(FILE_SYSTEM) blocks? > Source/WebKit/chromium/src/AsyncFileSystemChromium.cpp:178 > + filesystemName.append(document->securityOrigin()->databaseIdentifier()); Unrelated, but it seems kind of odd that this is called 'databaseIdentifier', but we're using it a non-database context. It seems like it could use a better name.
Comment on attachment 134087 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134087&action=review > Source/WebKit/chromium/src/AsyncFileSystemChromium.cpp:62 > +const AsyncFileSystem::Type isolatedType = static_cast<AsyncFileSystem::Type>(WebKit::WebFileSystem::TypeIsolated); Why isn't there a AsyncFileSystem::Type value that can be assigned here? (Should there be one?) > Source/WebKit/chromium/src/AsyncFileSystemChromium.cpp:64 > +const size_t isolatedPathPrefixLength = sizeof(isolatedPathPrefix) - 1; This seems unused. Why have it? > Source/WebKit/chromium/src/AsyncFileSystemChromium.cpp:190 > + rootURL.append(filesystemId); Where does filesystemId get validated? > Source/WebKit/chromium/src/PlatformSupport.cpp:103 > +#if ENABLE(FILE_SYSTEM) This if shouldn't be in the middle of other include's. (If it is needed, it should be after all other include's. Alternatively, can we leave out the if ENABLE completely and always include those header files?) > Source/WebKit/chromium/src/WebDragData.cpp:127 > +WebString WebDragData::filesystemId() const This part of the change along with the corresponding change in Source/WebKit/chromium/public/platform/WebDragData.h seems detached from everything else. In addition, it has no implementation and requires a different approval due to the change in a chromium/public directory. Why not pull it out of this patch?
Comment on attachment 134087 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134087&action=review >> Source/WebKit/chromium/src/AsyncFileSystemChromium.cpp:62 >> +const AsyncFileSystem::Type isolatedType = static_cast<AsyncFileSystem::Type>(WebKit::WebFileSystem::TypeIsolated); > > Why isn't there a AsyncFileSystem::Type value that can be assigned here? (Should there be one?) Currently it's only used in chromium port, so I wanted to keep it in chromium local. (I could expose it if it'd look better) >> Source/WebKit/chromium/src/AsyncFileSystemChromium.cpp:64 >> +const size_t isolatedPathPrefixLength = sizeof(isolatedPathPrefix) - 1; > > This seems unused. Why have it? Will remove. >> Source/WebKit/chromium/src/AsyncFileSystemChromium.cpp:178 >> + filesystemName.append(document->securityOrigin()->databaseIdentifier()); > > Unrelated, but it seems kind of odd that this is called 'databaseIdentifier', but we're using it a non-database context. It seems like it could use a better name. True, we've been historically using this string as identifier in several storage implementation not only in database. I won't change it in this cl but storageIdentifier or something would be better. >> Source/WebKit/chromium/src/AsyncFileSystemChromium.cpp:190 >> + rootURL.append(filesystemId); > > Where does filesystemId get validated? The synthesized rootURL (as well as filesystemId) is attached to every succeeding filesystem request and is validated each time in the browser process. I'll add the comment here. >> Source/WebKit/chromium/src/PlatformSupport.cpp:103 >> +#if ENABLE(FILE_SYSTEM) > > This if shouldn't be in the middle of other include's. (If it is needed, it should be after all other include's. Alternatively, can we leave out the if ENABLE completely and always include those header files?) Will remove this ENABLE if/endif. >> Source/WebKit/chromium/src/WebDragData.cpp:127 >> +WebString WebDragData::filesystemId() const > > This part of the change along with the corresponding change in Source/WebKit/chromium/public/platform/WebDragData.h seems detached from everything else. > > In addition, it has no implementation and requires a different approval due to the change in a chromium/public directory. Why not pull it out of this patch? I wanted to include the change where the 'filesystemId' is given (since other part and the actual implementation rely on the WebDragData change). This patch also contains other chromium/public change (in WebFileSystem), so I thought it might be convenient. (As you say it may look slightly irrelevant though.)
Created attachment 134168 [details] Patch
Comment on attachment 134168 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134168&action=review ok with me. You'll need approval from one of the above listed folks for the public change. > Source/WebKit/chromium/src/WebDragData.cpp:135 > + // FIXME: implement this. Would prefer a comment here saying that filesystemId must be validated and something about how it must be validated. (It shouldn't contain / or .. etc.)
Created attachment 134177 [details] Patch
Comment on attachment 134168 [details] Patch Thanks, updated. Could someone in WebKit API reviewers take a look at the chromium/public changes? View in context: https://bugs.webkit.org/attachment.cgi?id=134168&action=review >> Source/WebKit/chromium/src/WebDragData.cpp:135 >> + // FIXME: implement this. > > Would prefer a comment here saying that filesystemId must be validated and something about how it must be validated. (It shouldn't contain / or .. etc.) Done.
Comment on attachment 134177 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134177&action=review > Source/WebCore/platform/chromium/PlatformSupport.h:153 > +#if ENABLE(FILE_SYSTEM) > + static PassRefPtr<DOMFileSystem> createIsolatedFileSystem(Document*, const String& filesystemId); > +#endif This isn't correct. Nothing in WebCore/platform is allowed to know about Documents. Code in WebCore/platform cannot depend on WebCore code outside of WebCore/platform and Document.h is outside of WebCore/platform. > Source/WebKit/chromium/public/platform/WebFileSystem.h:50 > + // Used by ChromeOS. I'd skip these comments. Rather than saying who uses these types (which might change), it might be better to explain what these types mean.
Created attachment 134265 [details] Patch
Created attachment 134266 [details] Patch
Comment on attachment 134177 [details] Patch Thanks, updated the patch. View in context: https://bugs.webkit.org/attachment.cgi?id=134177&action=review >> Source/WebCore/platform/chromium/PlatformSupport.h:153 >> +#endif > > This isn't correct. Nothing in WebCore/platform is allowed to know about Documents. Code in WebCore/platform cannot depend on WebCore code outside of WebCore/platform and Document.h is outside of WebCore/platform. Fixed. (This file seems to have a lot of methods that refer Document or other WebCore objects though, which made me think this might be an exception.) >> Source/WebKit/chromium/public/platform/WebFileSystem.h:50 >> + // Used by ChromeOS. > > I'd skip these comments. Rather than saying who uses these types (which might change), it might be better to explain what these types mean. Good point, updated the comment.
> Fixed. (This file seems to have a lot of methods that refer Document or other WebCore objects though, which made me think this might be an exception.) Yeah, we have a lot of technical debt to pay. :-/
Comment on attachment 134266 [details] Patch WebKit API changes LGTM. Thanks!
(In reply to comment #18) > (From update of attachment 134266 [details]) > WebKit API changes LGTM. Thanks! David (levin@), would you be able to take the last look to flip r? Thanks!
Committed r112510: <http://trac.webkit.org/changeset/112510>