Bug 62069

Summary: Move file-choosing and icon-loading management to FileInputType
Product: WebKit Reporter: Dimitri Glazkov (Google) <dglazkov>
Component: FormsAssignee: Dimitri Glazkov (Google) <dglazkov>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, jianli, keishi, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 62748, 62756, 62931, 63039, 63222, 63224    
Bug Blocks: 62153    
Attachments:
Description Flags
WIP: First stab.
none
Patch
none
Patch tkent: review+

Description Dimitri Glazkov (Google) 2011-06-03 16:48:43 PDT
Make FileInputType a FileChooserClient.
Comment 1 Dimitri Glazkov (Google) 2011-06-03 16:49:46 PDT
Created attachment 95988 [details]
WIP: First stab.
Comment 2 Dimitri Glazkov (Google) 2011-06-03 16:51:29 PDT
RenderObject seems like a bad candidate to be a FileChooserClient -- its lifetime is just too volatile. What if we just made FileInputType the FileChooserClient?
Comment 3 WebKit Review Bot 2011-06-03 16:56:04 PDT
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
Comment 4 WebKit Review Bot 2011-06-03 17:52:09 PDT
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
Comment 5 Kent Tamura 2011-06-06 03:48:38 PDT
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.
Comment 6 Dimitri Glazkov (Google) 2011-06-06 10:42:24 PDT
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?
Comment 7 Dimitri Glazkov (Google) 2011-06-06 14:50:20 PDT
Created attachment 96128 [details]
Patch
Comment 8 Kent Tamura 2011-06-06 16:33:46 PDT
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.
Comment 9 Kent Tamura 2011-06-06 16:37:51 PDT
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().
Comment 10 Dimitri Glazkov (Google) 2011-06-07 09:50:27 PDT
(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.
Comment 11 Dimitri Glazkov (Google) 2011-06-07 10:57:55 PDT
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.
Comment 12 Dimitri Glazkov (Google) 2011-06-07 11:49:04 PDT
(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.
Comment 13 Dimitri Glazkov (Google) 2011-06-07 15:52:18 PDT
Perhaps what I need to do first is decouple concerns of rendering from FileChooser. It's wrong for it to be providing rendering feedback.
Comment 14 Dimitri Glazkov (Google) 2011-06-22 22:45:09 PDT
Created attachment 98314 [details]
Patch
Comment 15 Kent Tamura 2011-06-22 22:59:00 PDT
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.
Comment 16 Dimitri Glazkov (Google) 2011-06-22 23:04:14 PDT
Thanks for review! Will fix nits upon landing.
Comment 17 Dimitri Glazkov (Google) 2011-06-22 23:04:59 PDT
Committed r89535: <http://trac.webkit.org/changeset/89535>