Bug 82592 - Expose DataTransferItem.getAsEntry() to allow users access dropped files as FileEntry
Summary: Expose DataTransferItem.getAsEntry() to allow users access dropped files as F...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kinuko Yasuda
URL:
Keywords:
Depends on:
Blocks: 76809
  Show dependency treegraph
 
Reported: 2012-03-29 03:15 PDT by Kinuko Yasuda
Modified: 2012-04-05 00:30 PDT (History)
9 users (show)

See Also:


Attachments
Patch (42.43 KB, patch)
2012-03-29 17:56 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (41.57 KB, patch)
2012-03-29 18:53 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (10.04 MB, application/zip)
2012-03-29 21:26 PDT, WebKit Review Bot
no flags Details
Patch (40.49 KB, patch)
2012-03-30 04:28 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (41.61 KB, patch)
2012-03-30 07:52 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (40.69 KB, patch)
2012-04-02 07:23 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (40.76 KB, patch)
2012-04-02 22:24 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (40.78 KB, patch)
2012-04-03 07:36 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (40.77 KB, patch)
2012-04-03 20:21 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (41.12 KB, patch)
2012-04-04 21:53 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kinuko Yasuda 2012-03-29 03:15:04 PDT
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
Comment 1 Kinuko Yasuda 2012-03-29 17:56:00 PDT
Created attachment 134705 [details]
Patch
Comment 2 Kinuko Yasuda 2012-03-29 18:53:17 PDT
Created attachment 134711 [details]
Patch
Comment 3 Daniel Cheng 2012-03-29 19:16:37 PDT
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.
Comment 4 WebKit Review Bot 2012-03-29 21:26:23 PDT
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
Comment 5 WebKit Review Bot 2012-03-29 21:26:30 PDT
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
Comment 6 Kinuko Yasuda 2012-03-29 22:17:24 PDT
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.
Comment 7 Kinuko Yasuda 2012-03-30 04:28:52 PDT
Created attachment 134772 [details]
Patch
Comment 8 Kinuko Yasuda 2012-03-30 04:47:35 PDT
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.
Comment 9 Kinuko Yasuda 2012-03-30 07:52:52 PDT
Created attachment 134816 [details]
Patch
Comment 10 Daniel Cheng 2012-03-30 09:59:27 PDT
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.
Comment 11 Kinuko Yasuda 2012-04-02 07:23:39 PDT
Created attachment 135093 [details]
Patch
Comment 12 Kinuko Yasuda 2012-04-02 07:27:40 PDT
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.
Comment 13 Daniel Cheng 2012-04-02 13:21:04 PDT
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?
Comment 14 Kinuko Yasuda 2012-04-02 22:24:33 PDT
Created attachment 135275 [details]
Patch
Comment 15 Kinuko Yasuda 2012-04-02 22:28:29 PDT
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.
Comment 16 Daniel Cheng 2012-04-03 02:01:17 PDT
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.
Comment 17 Kinuko Yasuda 2012-04-03 07:36:03 PDT
Created attachment 135327 [details]
Patch
Comment 18 Kinuko Yasuda 2012-04-03 07:41:55 PDT
(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.
Comment 19 Kinuko Yasuda 2012-04-03 16:56:44 PDT
 > > 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,
Comment 20 David Levin 2012-04-03 17:03:19 PDT
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.
Comment 21 Kinuko Yasuda 2012-04-03 20:21:17 PDT
Created attachment 135496 [details]
Patch
Comment 22 Kinuko Yasuda 2012-04-03 20:27:28 PDT
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.
Comment 23 Tony Chang 2012-04-04 10:03:38 PDT
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.
Comment 24 Eric U. 2012-04-04 17:58:37 PDT
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.
Comment 25 Kinuko Yasuda 2012-04-04 21:52:08 PDT
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.
Comment 26 Kinuko Yasuda 2012-04-04 21:53:18 PDT
Created attachment 135754 [details]
Patch
Comment 27 WebKit Review Bot 2012-04-05 00:29:48 PDT
Comment on attachment 135754 [details]
Patch

Clearing flags on attachment: 135754

Committed r113297: <http://trac.webkit.org/changeset/113297>
Comment 28 WebKit Review Bot 2012-04-05 00:30:08 PDT
All reviewed patches have been landed.  Closing bug.