Bug 88293 – Support File/DirectoryEntry access for <input type=file> if FileSystem API is enabled

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Kinuko Yasuda]

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

[Kent Tamura]

You need to announce it in webkit-dev.
http://www.webkit.org/coding/adding-features.html

[Kinuko Yasuda]

Yup, I'm going to write to the list.

[Kinuko Yasuda]

Announce mail: http://lists.webkit.org/pipermail/webkit-dev/2012-June/020952.html

[Kinuko Yasuda]

Created attachment 146255 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Kinuko Yasuda]

Created attachment 146337 [details]
Patch

[Kinuko Yasuda]

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?

[Kent Tamura]

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.

[Kinuko Yasuda]

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

[Kinuko Yasuda]

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

[Kinuko Yasuda]

Created attachment 147102 [details]
Patch

[Adam Barth]

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?

[Eric U.]

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?

[Kinuko Yasuda]

(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

[Kinuko Yasuda]

Created attachment 147524 [details]
Patch

[Kinuko Yasuda]

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.

[Build Bot]

Comment on attachment 147524 [details]
Patch

Attachment 147524 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12950647

[Gyuyoung Kim]

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.

[Kinuko Yasuda]

Created attachment 147531 [details]
Patch

[Kinuko Yasuda]

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

[Kinuko Yasuda]

Tony or David, would you be able to take a look at it?  The change left in the patch is now relatively small.

[Tony Chang]

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?

[Kinuko Yasuda]

Created attachment 148064 [details]
for submit

[Kinuko Yasuda]

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.

[Kinuko Yasuda]

Created attachment 148082 [details]
for submit

[Kinuko Yasuda]

Committed r120593: <http://trac.webkit.org/changeset/120593>

[WebKit Review Bot]

Re-opened since this is blocked by 89404

[Kinuko Yasuda]

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