Bug 62069 – Move file-choosing and icon-loading management to FileInputType

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Dimitri Glazkov (Google)]

Make FileInputType a FileChooserClient.

[Dimitri Glazkov (Google)]

Created attachment 95988 [details]
WIP: First stab.

[Dimitri Glazkov (Google)]

RenderObject seems like a bad candidate to be a FileChooserClient -- its lifetime is just too volatile. What if we just made FileInputType the FileChooserClient?

[WebKit Review Bot]

Comment on attachment 95988 [details]
WIP: First stab.

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

[WebKit Review Bot]

Comment on attachment 95988 [details]
WIP: First stab.

Attachment 95988 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8763272

[Kent Tamura]

Comment on attachment 95988 [details]
WIP: First stab.

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

> Source/WebCore/html/FileInputType.cpp:48
> +// FIXME: Doesn't look like I need this code?!

The purpose of this is to pass already-selected files to a FileChooser.
The FileChooser can open a directory in which the selected files is, or provide a UI to remove a part of the selected files.

[Dimitri Glazkov (Google)]

Comment on attachment 95988 [details]
WIP: First stab.

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

>> Source/WebCore/html/FileInputType.cpp:48
>> +// FIXME: Doesn't look like I need this code?!
> 
> The purpose of this is to pass already-selected files to a FileChooser.
> The FileChooser can open a directory in which the selected files is, or provide a UI to remove a part of the selected files.

But if the lifetime of FileChooser is now tied to FileInputType. How can there be already-selected files in a newly-constructed FileInputType?

[Dimitri Glazkov (Google)]

Created attachment 96128 [details]
Patch

[Kent Tamura]

Comment on attachment 95988 [details]
WIP: First stab.

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

>>> Source/WebCore/html/FileInputType.cpp:48
>>> +// FIXME: Doesn't look like I need this code?!
>> 
>> The purpose of this is to pass already-selected files to a FileChooser.
>> The FileChooser can open a directory in which the selected files is, or provide a UI to remove a part of the selected files.
> 
> But if the lifetime of FileChooser is now tied to FileInputType. How can there be already-selected files in a newly-constructed FileInputType?

Yes.  IMO, we should
 * Follow the assumption that a FileChooser instance is created when we need to open a file chooser dialog, not in FileInputType constructor, or
 * Remove FileChooser::m_filenames.  It's redundant because we have FileInputType::m_fileList.

[Kent Tamura]

Comment on attachment 96128 [details]
Patch

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

> Source/WebCore/html/FileInputType.cpp:309
> +    element()->dispatchFormControlChangeEvent();
> + 
> +    repaint();

dispatchFormControlChangeEvent() dispatches a synchronous event, a JavaScript code might run in an event handler, and it might change the type of the <input>.
So this FileInputType might be deleted after dispatchFromControlChangeEvent().  We should not call repaint().

[Dimitri Glazkov (Google)]

(In reply to comment #9)
> (From update of attachment 96128 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96128&action=review
> 
> > Source/WebCore/html/FileInputType.cpp:309
> > +    element()->dispatchFormControlChangeEvent();
> > + 
> > +    repaint();
> 
> dispatchFormControlChangeEvent() dispatches a synchronous event, a JavaScript code might run in an event handler, and it might change the type of the <input>.
> So this FileInputType might be deleted after dispatchFromControlChangeEvent().  We should not call repaint().

Oooh. good point. Let me think on this a bit.

[Dimitri Glazkov (Google)]

Comment on attachment 96128 [details]
Patch

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

>>> Source/WebCore/html/FileInputType.cpp:309

>> 
>> dispatchFormControlChangeEvent() dispatches a synchronous event, a JavaScript code might run in an event handler, and it might change the type of the <input>.
>> So this FileInputType might be deleted after dispatchFromControlChangeEvent().  We should not call repaint().
> 
> Oooh. good point. Let me think on this a bit.

This one is easy. I can just move dispatchFormControlChangeEvent to the end of this function. However, we should probably consider switching change event to become a scoped event, to avoid this in the future.

[Dimitri Glazkov (Google)]

(In reply to comment #8)
> (From update of attachment 95988 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95988&action=review
> 
> >>> Source/WebCore/html/FileInputType.cpp:48
> >>> +// FIXME: Doesn't look like I need this code?!
> >> 
> >> The purpose of this is to pass already-selected files to a FileChooser.
> >> The FileChooser can open a directory in which the selected files is, or provide a UI to remove a part of the selected files.
> > 
> > But if the lifetime of FileChooser is now tied to FileInputType. How can there be already-selected files in a newly-constructed FileInputType?
> 
> Yes.  IMO, we should
>  * Follow the assumption that a FileChooser instance is created when we need to open a file chooser dialog, not in FileInputType constructor, or

I tried this first. You can't really do this elegantly, since fileChooser() is queried by RenderFileUploadControl (to draw icon and text), so it has to exist longer than that. Perhaps once we switch to purely CSS rendering, we could take this step, but at the moment it's not really possible.

>  * Remove FileChooser::m_filenames.  It's redundant because we have FileInputType::m_fileList.

I tried this next. Once again, the entanglement with rendering spoils everything. In this case, loadIcon and basenameForWidth need to know about the list of files, and adding filenames as parameter seems inelegant.

[Dimitri Glazkov (Google)]

Perhaps what I need to do first is decouple concerns of rendering from FileChooser. It's wrong for it to be providing rendering feedback.

[Dimitri Glazkov (Google)]

Created attachment 98314 [details]
Patch

[Kent Tamura]

Comment on attachment 98314 [details]
Patch

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

> Source/WebCore/html/HTMLInputElement.cpp:1273
> +    ASSERT(m_inputType->isFileUpload());
> +    m_inputType->receiveDroppedFiles(filenames);

InputType::receiveDroppedFiles() has ASSERT_NOT_REACHED().  So the assertion here is redundant.

> Source/WebCore/html/HTMLInputElement.cpp:1279
> +    ASSERT(m_inputType->isFileUpload());
> +    return m_inputType->icon();

ditto.

[Dimitri Glazkov (Google)]

Thanks for review! Will fix nits upon landing.

[Dimitri Glazkov (Google)]

Committed r89535: <http://trac.webkit.org/changeset/89535>