Bug 88293 - Support File/DirectoryEntry access for <input type=file> if FileSystem API is enabled
Summary: Support File/DirectoryEntry access for <input type=file> if FileSystem API is...
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: 89404
Blocks: 76809
  Show dependency treegraph
 
Reported: 2012-06-04 23:12 PDT by Kinuko Yasuda
Modified: 2012-06-20 05:02 PDT (History)
13 users (show)

See Also:


Attachments
Patch (36.12 KB, patch)
2012-06-07 04:51 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (599.25 KB, application/zip)
2012-06-07 09:26 PDT, WebKit Review Bot
no flags Details
Patch (35.83 KB, patch)
2012-06-07 11:21 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (33.37 KB, patch)
2012-06-12 09:44 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (32.67 KB, patch)
2012-06-14 02:07 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (32.70 KB, patch)
2012-06-14 02:32 PDT, Kinuko Yasuda
tony: review+
Details | Formatted Diff | Diff
for submit (31.88 KB, patch)
2012-06-18 00:01 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
for submit (31.92 KB, patch)
2012-06-18 04:47 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-06-04 23:12:13 PDT
Support File/DirectoryEntry access for <input type=file>

Per discussion on whatwg we should support .entries to enable flexible and asynchronous files/directories access via {File,Directory}Entry in FileSystem API (if the API is enabled).
http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2011-November/033814.html
Comment 1 Kent Tamura 2012-06-04 23:28:29 PDT
You need to announce it in webkit-dev.
http://www.webkit.org/coding/adding-features.html
Comment 2 Kinuko Yasuda 2012-06-05 00:46:56 PDT
Yup, I'm going to write to the list.
Comment 3 Kinuko Yasuda 2012-06-07 04:50:23 PDT
Announce mail: http://lists.webkit.org/pipermail/webkit-dev/2012-June/020952.html
Comment 4 Kinuko Yasuda 2012-06-07 04:51:50 PDT
Created attachment 146255 [details]
Patch
Comment 5 WebKit Review Bot 2012-06-07 09:26:16 PDT
Comment on attachment 146255 [details]
Patch

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

New failing tests:
fast/forms/file/input-file-entries.html
Comment 6 WebKit Review Bot 2012-06-07 09:26:20 PDT
Created attachment 146305 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 7 Kinuko Yasuda 2012-06-07 11:21:27 PDT
Created attachment 146337 [details]
Patch
Comment 8 Kinuko Yasuda 2012-06-12 01:02:30 PDT
David and/or EricU could you take a look at the file system related part?
Kent-san would you be able to review the HTMLInput change?
Comment 9 Kent Tamura 2012-06-12 01:48:32 PDT
Comment on attachment 146337 [details]
Patch

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

> Source/WebCore/html/FileInputType.cpp:406
> -void FileInputType::receiveDroppedFiles(const Vector<String>& paths)
> +bool FileInputType::receiveDroppedFiles(const DragData* dragData)
>  {
> +    Vector<String> paths;
> +    dragData->asFilenames(paths);
> +    if (paths.isEmpty())
> +        return false;
> +
>      HTMLInputElement* input = element();
>  #if ENABLE(DIRECTORY_UPLOAD)
>      if (input->fastHasAttribute(webkitdirectoryAttr)) {
>          receiveDropForDirectoryUpload(paths);
> -        return;
> +        return true;
>      }
>  #endif

Can you move this change into a separated patch?
It will be a no-behavior-change patch and I can set r+ easily.
Comment 10 Kinuko Yasuda 2012-06-12 02:06:13 PDT
(In reply to comment #9)
> (From update of attachment 146337 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146337&action=review
> 
> > Source/WebCore/html/FileInputType.cpp:406
> > -void FileInputType::receiveDroppedFiles(const Vector<String>& paths)
> > +bool FileInputType::receiveDroppedFiles(const DragData* dragData)
> >  {
> > +    Vector<String> paths;
> > +    dragData->asFilenames(paths);
> > +    if (paths.isEmpty())
> > +        return false;
> > +
> >      HTMLInputElement* input = element();
> >  #if ENABLE(DIRECTORY_UPLOAD)
> >      if (input->fastHasAttribute(webkitdirectoryAttr)) {
> >          receiveDropForDirectoryUpload(paths);
> > -        return;
> > +        return true;
> >      }
> >  #endif
> 
> Can you move this change into a separated patch?
> It will be a no-behavior-change patch and I can set r+ easily.

Sounds good, will do.
Comment 11 Kinuko Yasuda 2012-06-12 04:48:06 PDT
(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 146337 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=146337&action=review
> > 
> > > Source/WebCore/html/FileInputType.cpp:406
> > > -void FileInputType::receiveDroppedFiles(const Vector<String>& paths)
> > > +bool FileInputType::receiveDroppedFiles(const DragData* dragData)
> > >  {
> > > +    Vector<String> paths;
> > > +    dragData->asFilenames(paths);
> > > +    if (paths.isEmpty())
> > > +        return false;
> > > +
> > >      HTMLInputElement* input = element();
> > >  #if ENABLE(DIRECTORY_UPLOAD)
> > >      if (input->fastHasAttribute(webkitdirectoryAttr)) {
> > >          receiveDropForDirectoryUpload(paths);
> > > -        return;
> > > +        return true;
> > >      }
> > >  #endif
> > 
> > Can you move this change into a separated patch?
> > It will be a no-behavior-change patch and I can set r+ easily.
> 
> Sounds good, will do.

Filed a separate issue (bug 88860) for the InputType only changes.
Comment 12 Kinuko Yasuda 2012-06-12 09:44:18 PDT
Created attachment 147102 [details]
Patch
Comment 13 Adam Barth 2012-06-12 11:04:09 PDT
Comment on attachment 147102 [details]
Patch

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

> Source/WebCore/Modules/filesystem/chromium/HTMLInputElementFileSystemChromium.cpp:54
> +    RefPtr<DOMFileSystem> filesystem = DOMFileSystemChromium::createIsolatedFileSystem(scriptExecutionContext, input->droppedFileSystemId());

It's too bad that this has to be Chromium-specific.  Can non-Chromium file systems not create isolated file systems?
Comment 14 Eric U. 2012-06-12 14:33:32 PDT
Comment on attachment 147102 [details]
Patch

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

> Source/WebCore/platform/DragData.cpp:-75
> -

Missing implementation of droppedFileSystemId for ENABLE(FILE_SYSTEM) but not Chromium?
Comment 15 Kinuko Yasuda 2012-06-13 07:40:37 PDT
(In reply to comment #13)
> (From update of attachment 147102 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147102&action=review
> 
> > Source/WebCore/Modules/filesystem/chromium/HTMLInputElementFileSystemChromium.cpp:54
> > +    RefPtr<DOMFileSystem> filesystem = DOMFileSystemChromium::createIsolatedFileSystem(scriptExecutionContext, input->droppedFileSystemId());
> 
> It's too bad that this has to be Chromium-specific.  Can non-Chromium file systems not create isolated file systems?

Hmm good point, probably the isolated file system related code does not need to be Chromium-specific.
Other platforms can create it, though each platform would need some platform-specific code.

I've filed a new issue and uploaded a patch to move the common isolated fs code out of chromium-specific directory: https://bugs.webkit.org/show_bug.cgi?id=88997
Comment 16 Kinuko Yasuda 2012-06-14 02:07:58 PDT
Created attachment 147524 [details]
Patch
Comment 17 Kinuko Yasuda 2012-06-14 02:09:54 PDT
Comment on attachment 147102 [details]
Patch

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

>> Source/WebCore/platform/DragData.cpp:-75
>> -
> 
> Missing implementation of droppedFileSystemId for ENABLE(FILE_SYSTEM) but not Chromium?

Added default implementation to non-chromium platforms.
Comment 18 Build Bot 2012-06-14 02:18:47 PDT
Comment on attachment 147524 [details]
Patch

Attachment 147524 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12950647
Comment 19 Gyuyoung Kim 2012-06-14 02:29:55 PDT
Comment on attachment 147524 [details]
Patch

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

> Source/WebCore/platform/DragData.cpp:74
> +String DragData::droppedFileSystemId() const

Don't you need to use #if ENABLE_FILE_SYSTEM) macro ? In EFL port, we disable FILE_SYSTEM feature.
Comment 20 Kinuko Yasuda 2012-06-14 02:32:04 PDT
Created attachment 147531 [details]
Patch
Comment 21 Kinuko Yasuda 2012-06-14 02:32:42 PDT
(In reply to comment #19)
> (From update of attachment 147524 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147524&action=review
> 
> > Source/WebCore/platform/DragData.cpp:74
> > +String DragData::droppedFileSystemId() const
> 
> Don't you need to use #if ENABLE_FILE_SYSTEM) macro ? In EFL port, we disable FILE_SYSTEM feature.

Yes, absolutely we need it... Just added the #if in the new patch! Thanks.
Comment 22 Kinuko Yasuda 2012-06-14 23:02:26 PDT
Tony or David, would you be able to take a look at it?  The change left in the patch is now relatively small.
Comment 23 Tony Chang 2012-06-15 10:23:20 PDT
Comment on attachment 147531 [details]
Patch

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

Seems fine.  Please make sure you update all the build files.

> Source/WebCore/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Test: fast/forms/file/input-file-entries.html

Please link to the spec here.

> Source/WebCore/ChangeLog:12
> +        * Modules/filesystem/HTMLInputElementFileSystem.cpp: Added.
> +        * Modules/filesystem/HTMLInputElementFileSystem.h: Added.
> +        * Modules/filesystem/HTMLInputElementFileSystem.idl: Added.

Should we be updating the other build systems with these files? It looks like the other build files (GNUmakefile.list.am, Target.pri, etc) list filesystem files even if the feature isn't enabled.

> Source/WebCore/ChangeLog:17
> +        * html/FileInputType.cpp:
> +        (WebCore::FileInputType::receiveDroppedFiles):
> +        (WebCore::FileInputType::droppedFileSystemId): Added.
> +        * html/FileInputType.h:

Nit: Can you add a few comments in this section about the changes you're making?

> Source/WebCore/html/InputType.cpp:680
> +String InputType::droppedFileSystemId()
> +{
> +    ASSERT_NOT_REACHED();
> +    return String();

Nit: Should we just make this pure virtual? I guess there are already other methods that have ASSERT_NOT_REACHED(), so this is OK.

> LayoutTests/fast/forms/file/input-file-entries.html:37
> +if (layoutTestController) {

window.layoutTestController or this will raise an exception in Chrome/Safari.

> LayoutTests/fast/forms/file/input-file-entries.html:59
> +    eventSender.mouseMoveTo(10, 10);

Can we compute the middle of the file control using clientWidth/clientHeight and offsetWidth/offsetHeight?
Comment 24 Kinuko Yasuda 2012-06-18 00:01:42 PDT
Created attachment 148064 [details]
for submit
Comment 25 Kinuko Yasuda 2012-06-18 00:06:15 PDT
Thanks for your review!

(In reply to comment #23)
> (From update of attachment 147531 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147531&action=review
> 
> Seems fine.  Please make sure you update all the build files.
> 
> > Source/WebCore/ChangeLog:8
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Test: fast/forms/file/input-file-entries.html
> 
> Please link to the spec here.

I added a link to the proposal since it's still in the proposing status.

> > Source/WebCore/ChangeLog:12
> > +        * Modules/filesystem/HTMLInputElementFileSystem.cpp: Added.
> > +        * Modules/filesystem/HTMLInputElementFileSystem.h: Added.
> > +        * Modules/filesystem/HTMLInputElementFileSystem.idl: Added.
> 
> Should we be updating the other build systems with these files? It looks like the other build files (GNUmakefile.list.am, Target.pri, etc) list filesystem files even if the feature isn't enabled.

These files are independent from others and can be added selectively when a platform wants to support entries in input, so I leave them for now.

> > Source/WebCore/ChangeLog:17
> > +        * html/FileInputType.cpp:
> > +        (WebCore::FileInputType::receiveDroppedFiles):
> > +        (WebCore::FileInputType::droppedFileSystemId): Added.
> > +        * html/FileInputType.h:
> 
> Nit: Can you add a few comments in this section about the changes you're making?

Done.

> > Source/WebCore/html/InputType.cpp:680
> > +String InputType::droppedFileSystemId()
> > +{
> > +    ASSERT_NOT_REACHED();
> > +    return String();
> 
> Nit: Should we just make this pure virtual? I guess there are already other methods that have ASSERT_NOT_REACHED(), so this is OK.

Yeah I followed other methods, assuming that having default methods in the base class and subclasses only implement what they care is the convention here. (Kent-san, let me know if this doesn't sound right)

> > LayoutTests/fast/forms/file/input-file-entries.html:37
> > +if (layoutTestController) {
> 
> window.layoutTestController or this will raise an exception in Chrome/Safari.

Fixed.

> > LayoutTests/fast/forms/file/input-file-entries.html:59
> > +    eventSender.mouseMoveTo(10, 10);
> 
> Can we compute the middle of the file control using clientWidth/clientHeight and offsetWidth/offsetHeight?

Done.
Comment 26 Kinuko Yasuda 2012-06-18 04:47:30 PDT
Created attachment 148082 [details]
for submit
Comment 27 Kinuko Yasuda 2012-06-18 07:10:57 PDT
Committed r120593: <http://trac.webkit.org/changeset/120593>
Comment 28 WebKit Review Bot 2012-06-18 17:12:32 PDT
Re-opened since this is blocked by 89404
Comment 29 Kinuko Yasuda 2012-06-20 05:02:10 PDT
Relanded as r120667: <http://trac.webkit.org/changeset/120667>
with a test fix to exclude '.svn' directory from the test results (that's why the test had failed only on bots):
http://trac.webkit.org/changeset/120674