Bug 76826 – [chromium] Add isolated filesystem type and WebDragData::filesystem_id for drag-and-drop using File/DirectoryEntry.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Kinuko Yasuda]

[chromium] Add isolated filesystem type and WebDragData::filesystem_id for drag-and-drop using File/DirectoryEntry.

[Kinuko Yasuda]

Created attachment 134071 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Comment on attachment 134071 [details]
Patch

Attachment 134071 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12142577

[Kinuko Yasuda]

Created attachment 134087 [details]
Patch

Fixed build.

[Tony Chang]

Daniel, do you want to review the changes to WebDragData?

[Daniel Cheng]

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.

[David Levin]

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?

[Kinuko Yasuda]

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.)

[Kinuko Yasuda]

Created attachment 134168 [details]
Patch

[David Levin]

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.)

[Kinuko Yasuda]

Created attachment 134177 [details]
Patch

[Kinuko Yasuda]

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.

[Adam Barth]

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.

[Kinuko Yasuda]

Created attachment 134265 [details]
Patch

[Kinuko Yasuda]

Created attachment 134266 [details]
Patch

[Kinuko Yasuda]

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.

[Adam Barth]

> 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.  :-/

[Adam Barth]

Comment on attachment 134266 [details]
Patch

WebKit API changes LGTM.  Thanks!

[Kinuko Yasuda]

(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!

[Kinuko Yasuda]

Committed r112510: <http://trac.webkit.org/changeset/112510>