Bug 82592 – Expose DataTransferItem.getAsEntry() to allow users access dropped files as FileEntry

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Kinuko Yasuda]

Expose DataTransferItem.getAsEntry() to allow users access dropped files as FileEntry or DirectoryEntry in FileSystem API
as discussed in: http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2011-November/033814.html

[Kinuko Yasuda]

Created attachment 134705 [details]
Patch

[Kinuko Yasuda]

Created attachment 134711 [details]
Patch

[Daniel Cheng]

Comment on attachment 134711 [details]
Patch

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

> Source/WebCore/Modules/filesystem/DataTransferItemFileSystemChromium.cpp:111
> +void DataTransferItemFileSystem::webkitGetAsEntry(DataTransferItem* dataTransferItem, PassRefPtr<EntryCallback> callback)

This might be a layering violation, but for consistency with the current code, how about something like this?

1) This static method should simply call static_cast<DataTransferItemPolicyWrapper*>(item)->getAsEntry(callback).
2) DataTransferItemPolicyWrapper::getAsEntry performs checks that callback is valid and that the clipboard is not invalidated (this is missing in the current patch).
3) DataTransferItemPolicyWrapper::getAsEntry calls a Modules/filesystem helper to create the actual Entry. At this point, we can pass in the ScriptExecutionContext (since DTIPW has a pointer to Clipboard, which can extract SEC from) and the original callback. That way, we also avoid the need to carry around redundant ScriptExecutionContext pointers.

Thoughts?

> Source/WebCore/Modules/filesystem/DataTransferItemFileSystemChromium.cpp:139
> +        m_filesystem = DOMFileSystem::create(scriptExecutionContext, filesystemName, PlatformSupport::createIsolatedFileSystem(scriptExecutionContext->securityOrigin()->toString(), filesystemId));

Just curious: if I call this multiple times with the exact same arguments, will the DOMFileSystem object I get back refer to the same isolated file system?

> Source/WebCore/Modules/filesystem/DataTransferItemFileSystemChromium.h:72
> +class DataTransferItemListFileSystemData {

As far as I can tell, this isn't actually directly related to DataTransferItem at all. Maybe call it LazyIsolatedFileSystem?

> LayoutTests/editing/pasteboard/data-transfer-items-drag-drop-entry.html:47
> +    layoutTestController.dumpAsText();

I think you included a pixel result by accident.

> LayoutTests/editing/pasteboard/data-transfer-items-drag-drop-entry.html:57
> +    eventSender.leapForward(500);

100 instead of 500 should work too.

[WebKit Review Bot]

Comment on attachment 134711 [details]
Patch

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

New failing tests:
editing/pasteboard/data-transfer-items-drag-drop-entry.html

[WebKit Review Bot]

Created attachment 134721 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Kinuko Yasuda]

Comment on attachment 134711 [details]
Patch

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

>> Source/WebCore/Modules/filesystem/DataTransferItemFileSystemChromium.cpp:111
>> +void DataTransferItemFileSystem::webkitGetAsEntry(DataTransferItem* dataTransferItem, PassRefPtr<EntryCallback> callback)
> 
> This might be a layering violation, but for consistency with the current code, how about something like this?
> 
> 1) This static method should simply call static_cast<DataTransferItemPolicyWrapper*>(item)->getAsEntry(callback).
> 2) DataTransferItemPolicyWrapper::getAsEntry performs checks that callback is valid and that the clipboard is not invalidated (this is missing in the current patch).
> 3) DataTransferItemPolicyWrapper::getAsEntry calls a Modules/filesystem helper to create the actual Entry. At this point, we can pass in the ScriptExecutionContext (since DTIPW has a pointer to Clipboard, which can extract SEC from) and the original callback. That way, we also avoid the need to carry around redundant ScriptExecutionContext pointers.
> 
> Thoughts?

I like the simplicity, and my first patch was very similar to your idea.
What concern me are:
- making the core DataTransferItem code handle getAsEntry() sounds slightly inverse to what we're doing for modularization in other parts. (We're already in chromium/ port directory so it might not be an issue though)
- adding new code accessing WebCore objects under platform worries me. It's inevitable while we have all clipboard/drag-and-drop code here but I want to make the dependency minimal.

By the way as for ScriptExecutionContext alternatively we can pass it from the binding layer, so we can avoid carrying around the context in anyways. (Will do in the next patch)

>> Source/WebCore/Modules/filesystem/DataTransferItemFileSystemChromium.cpp:139
>> +        m_filesystem = DOMFileSystem::create(scriptExecutionContext, filesystemName, PlatformSupport::createIsolatedFileSystem(scriptExecutionContext->securityOrigin()->toString(), filesystemId));
> 
> Just curious: if I call this multiple times with the exact same arguments, will the DOMFileSystem object I get back refer to the same isolated file system?

We may get different instance in terms of C++ object but it'd be referring the same file system in the backend as far as we use the same filesystemId in the same renderer process.

>> Source/WebCore/Modules/filesystem/DataTransferItemFileSystemChromium.h:72
>> +class DataTransferItemListFileSystemData {
> 
> As far as I can tell, this isn't actually directly related to DataTransferItem at all. Maybe call it LazyIsolatedFileSystem?

Sounds good, will do.

[Kinuko Yasuda]

Created attachment 134772 [details]
Patch

[Kinuko Yasuda]

Comment on attachment 134711 [details]
Patch

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

>>> Source/WebCore/Modules/filesystem/DataTransferItemFileSystemChromium.cpp:111
>>> +void DataTransferItemFileSystem::webkitGetAsEntry(DataTransferItem* dataTransferItem, PassRefPtr<EntryCallback> callback)
>> 
>> This might be a layering violation, but for consistency with the current code, how about something like this?
>> 
>> 1) This static method should simply call static_cast<DataTransferItemPolicyWrapper*>(item)->getAsEntry(callback).
>> 2) DataTransferItemPolicyWrapper::getAsEntry performs checks that callback is valid and that the clipboard is not invalidated (this is missing in the current patch).
>> 3) DataTransferItemPolicyWrapper::getAsEntry calls a Modules/filesystem helper to create the actual Entry. At this point, we can pass in the ScriptExecutionContext (since DTIPW has a pointer to Clipboard, which can extract SEC from) and the original callback. That way, we also avoid the need to carry around redundant ScriptExecutionContext pointers.
>> 
>> Thoughts?
> 
> I like the simplicity, and my first patch was very similar to your idea.
> What concern me are:
> - making the core DataTransferItem code handle getAsEntry() sounds slightly inverse to what we're doing for modularization in other parts. (We're already in chromium/ port directory so it might not be an issue though)
> - adding new code accessing WebCore objects under platform worries me. It's inevitable while we have all clipboard/drag-and-drop code here but I want to make the dependency minimal.
> 
> By the way as for ScriptExecutionContext alternatively we can pass it from the binding layer, so we can avoid carrying around the context in anyways. (Will do in the next patch)

For now I kept the same/similar structure as I thought in general we should try respecting the module boundary.  This time I also introduced Supplemental/Supplementable fixtures so that we have less ifdefs in WebCore.

As for the policy control, we call PolicyWrapper::getAsFile() internally and let getAsEntry() fail if we couldn't get a File, so we must be providing the same guard for clipboard invalidation.

>>> Source/WebCore/Modules/filesystem/DataTransferItemFileSystemChromium.h:72
>>> +class DataTransferItemListFileSystemData {
>> 
>> As far as I can tell, this isn't actually directly related to DataTransferItem at all. Maybe call it LazyIsolatedFileSystem?
> 
> Sounds good, will do.

Done.  (Renamed to DraggedIsolatedFileSystem and made it 'Supplement' object of ChromiumDataObject)

>> LayoutTests/editing/pasteboard/data-transfer-items-drag-drop-entry.html:47
>> +    layoutTestController.dumpAsText();
> 
> I think you included a pixel result by accident.

Yep... removed.

>> LayoutTests/editing/pasteboard/data-transfer-items-drag-drop-entry.html:57
>> +    eventSender.leapForward(500);
> 
> 100 instead of 500 should work too.

Done.

[Kinuko Yasuda]

Created attachment 134816 [details]
Patch

[Daniel Cheng]

Comment on attachment 134816 [details]
Patch

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

> Source/WebCore/Modules/filesystem/chromium/DataTransferItemFileSystemChromium.cpp:109
> +    // For dragged files getAsFile must be pretty lightweight.

Lines 109-115 can move above line 103. Also, to match getAsString(), we shouldn't invoke the callback at all if the Clipboard has been invalidated.

> Source/WebCore/Modules/filesystem/chromium/DataTransferItemFileSystemChromium.cpp:116
> +    ASSERT(file && file->isFile());

Don't need to assert non-null here.

[Kinuko Yasuda]

Created attachment 135093 [details]
Patch

[Kinuko Yasuda]

Comment on attachment 134816 [details]
Patch

Thanks, updated.

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

>> Source/WebCore/Modules/filesystem/chromium/DataTransferItemFileSystemChromium.cpp:109
>> +    // For dragged files getAsFile must be pretty lightweight.
> 
> Lines 109-115 can move above line 103. Also, to match getAsString(), we shouldn't invoke the callback at all if the Clipboard has been invalidated.

Done.

>> Source/WebCore/Modules/filesystem/chromium/DataTransferItemFileSystemChromium.cpp:116
>> +    ASSERT(file && file->isFile());
> 
> Don't need to assert non-null here.

Done.

[Daniel Cheng]

Comment on attachment 135093 [details]
Patch

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

> Source/WebCore/Modules/filesystem/chromium/DataTransferItemFileSystemChromium.cpp:107
> +        DOMFileSystem::scheduleCallback(scriptExecutionContext, callback, adoptRef(static_cast<Entry*>(0)));

To match getAsString(), we should not call the callback at all--we can just return.

> Source/WebCore/Modules/filesystem/chromium/DraggedIsolatedFileSystem.cpp:50
> +    if (!scriptExecutionContext || m_filesystemId.isEmpty())

I'm not completely familiar with filesystem code, so these are just questions to understand:
Is it possible for ScriptExecutionContext to be NULL here? Also, why would m_filesystemId be unset?

[Kinuko Yasuda]

Created attachment 135275 [details]
Patch

[Kinuko Yasuda]

Comment on attachment 135093 [details]
Patch

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

>> Source/WebCore/Modules/filesystem/chromium/DataTransferItemFileSystemChromium.cpp:107
>> +        DOMFileSystem::scheduleCallback(scriptExecutionContext, callback, adoptRef(static_cast<Entry*>(0)));
> 
> To match getAsString(), we should not call the callback at all--we can just return.

Good catch, changed.

>> Source/WebCore/Modules/filesystem/chromium/DraggedIsolatedFileSystem.cpp:50
>> +    if (!scriptExecutionContext || m_filesystemId.isEmpty())
> 
> I'm not completely familiar with filesystem code, so these are just questions to understand:
> Is it possible for ScriptExecutionContext to be NULL here? Also, why would m_filesystemId be unset?

Here ScriptExecutionContext is given by the IDL binding layer, so I assume it shouldn't be NULL--- I changed the if to ASSERT for ScriptExecutionContext.

m_filesystemId is set by chromium when files are dropped, so if the chromium port is working as expected it shouldn't be empty, but since it's outside the WebKit codebase I'm checking the value here.

[Daniel Cheng]

Comment on attachment 135275 [details]
Patch

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

> Source/WebCore/Modules/filesystem/chromium/DraggedIsolatedFileSystem.cpp:50
> +    if (m_filesystemId.isEmpty())

Are we already setting the filesystem ID in chromium? If so, would it make sense to put ASSERT_NOT_REACHED() in here as well?

I'm not an official reviewer, and I'm not super familiar with the Filesystem API or the Module refactoring, but in general, it looks good to me.

[Kinuko Yasuda]

Created attachment 135327 [details]
Patch

[Kinuko Yasuda]

(In reply to comment #16)
> (From update of attachment 135275 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135275&action=review
> 
> > Source/WebCore/Modules/filesystem/chromium/DraggedIsolatedFileSystem.cpp:50
> > +    if (m_filesystemId.isEmpty())
> 
> Are we already setting the filesystem ID in chromium? If so, would it make sense to put ASSERT_NOT_REACHED() in here as well?

Yes we do so in chromium... ok I added the assertion.

> I'm not an official reviewer, and I'm not super familiar with the Filesystem API or the Module refactoring, but in general, it looks good to me.

Thanks!  I'll wait official reviewers to take a look at it.

[Kinuko Yasuda]

 > > I'm not an official reviewer, and I'm not super familiar with the Filesystem API or the Module refactoring, but in general, it looks good to me.
> 
> Thanks!  I'll wait official reviewers to take a look at it.

David, EricU: can you take a look at the Module/filesystem changes?
Tony, could you take a look at the drag-and-drop part?
Thanks,

[David Levin]

Comment on attachment 135327 [details]
Patch

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

Just a few quick comments from my quick look over it. (It wasn't though yet.)

> Source/WebCore/Modules/filesystem/chromium/DraggedIsolatedFileSystem.cpp:63
> +    DEFINE_STATIC_LOCAL(AtomicString, name, ("DraggedIsolatedFileSystem"));

Can this be called on multiple threads?

If not, please assert (isMainThread, etc.).

If so, then there is no way that an AtomicString allocated on one thread is valid on another.

> Source/WebCore/platform/chromium/ChromiumDataObject.cpp:2
> + * Copyright (c) 2012 Google Inc. All rights reserved.

In general just append years in WebKit (unlike chromium).

> Source/WebCore/platform/chromium/ClipboardChromium.h:46
> +// A wrapper class that invalidates a DataTransferItem when the associated Clipboard object goes out of scope.

indent

> Source/WebCore/platform/chromium/ClipboardChromium.h:52
> +        virtual String kind() const OVERRIDE;

I didn't know WebKit did OVERRIDE.

[Kinuko Yasuda]

Created attachment 135496 [details]
Patch

[Kinuko Yasuda]

Comment on attachment 135327 [details]
Patch

Thanks, updated.

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

>> Source/WebCore/Modules/filesystem/chromium/DraggedIsolatedFileSystem.cpp:63
>> +    DEFINE_STATIC_LOCAL(AtomicString, name, ("DraggedIsolatedFileSystem"));
> 
> Can this be called on multiple threads?
> 
> If not, please assert (isMainThread, etc.).
> 
> If so, then there is no way that an AtomicString allocated on one thread is valid on another.

Added the isMainThread assertion as it should be called only on the main thread.

>> Source/WebCore/platform/chromium/ChromiumDataObject.cpp:2
>> + * Copyright (c) 2012 Google Inc. All rights reserved.
> 
> In general just append years in WebKit (unlike chromium).

Fixed.

>> Source/WebCore/platform/chromium/ClipboardChromium.h:46
>> +// A wrapper class that invalidates a DataTransferItem when the associated Clipboard object goes out of scope.
> 
> indent

Fixed.

>> Source/WebCore/platform/chromium/ClipboardChromium.h:52
>> +        virtual String kind() const OVERRIDE;
> 
> I didn't know WebKit did OVERRIDE.

We seem to have introduced it relatively recently.

[Tony Chang]

Comment on attachment 135496 [details]
Patch

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

d&d code LGTM.  I'll let David do the final review.

> Source/WebCore/ChangeLog:4
> +        Expose DataTransferItem.getAsEntry() to allow users access dropped files as FileEntry
> +        https://bugs.webkit.org/show_bug.cgi?id=82592

Can you include a link to the discussion or spec in the changelog? Also, maybe mention that it's webkit prefixed for now.

> Source/WebCore/Modules/filesystem/DataTransferItemFileSystem.h:41
> +class EntryCallback;
> +class DataTransferItem;

Nit: Sort.

> Source/WebCore/platform/chromium/ClipboardChromium.h:42
> +    class ClipboardChromium;
> +    class ChromiumDataObjectItem;

Nit: Sort.

[Eric U.]

Comment on attachment 135496 [details]
Patch

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

> Source/WebCore/Modules/filesystem/DataTransferItemFileSystem.idl:37
> +        [CallWith=ScriptExecutionContext] void webkitGetAsEntry(in [Callback,Optional=DefaultIsUndefined] EntryCallback callback);

Should there be an error callback as well?

> Source/WebCore/Modules/filesystem/chromium/DataTransferItemFileSystemChromium.cpp:77
> +        m_callback->handleEvent(0);

We're losing the error code here.

[Kinuko Yasuda]

Comment on attachment 135496 [details]
Patch

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

>> Source/WebCore/ChangeLog:4
>> +        https://bugs.webkit.org/show_bug.cgi?id=82592
> 
> Can you include a link to the discussion or spec in the changelog? Also, maybe mention that it's webkit prefixed for now.

Done.

>> Source/WebCore/Modules/filesystem/DataTransferItemFileSystem.h:41
>> +class DataTransferItem;
> 
> Nit: Sort.

Fixed.

>> Source/WebCore/Modules/filesystem/DataTransferItemFileSystem.idl:37
>> +        [CallWith=ScriptExecutionContext] void webkitGetAsEntry(in [Callback,Optional=DefaultIsUndefined] EntryCallback callback);
> 
> Should there be an error callback as well?

I wondered if we should do so, but I thought it'd be better to make it aligned with other dataTrasnferItem methods (e.g. getAsString() simply returns empty string on error).

>> Source/WebCore/Modules/filesystem/chromium/DataTransferItemFileSystemChromium.cpp:77
>> +        m_callback->handleEvent(0);
> 
> We're losing the error code here.

Please see my response above... well maybe I think we should return an Entry instance which lets any succeeding operations failure with the error code?  It will give apps better consistency and idea about the error.
I added FIXME comment about that.

>> Source/WebCore/platform/chromium/ClipboardChromium.h:42
>> +    class ChromiumDataObjectItem;
> 
> Nit: Sort.

Fixed.

[Kinuko Yasuda]

Created attachment 135754 [details]
Patch

[WebKit Review Bot]

Comment on attachment 135754 [details]
Patch

Clearing flags on attachment: 135754

Committed r113297: <http://trac.webkit.org/changeset/113297>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.