Bug 62069 - Move file-choosing and icon-loading management to FileInputType
Summary: Move file-choosing and icon-loading management to FileInputType
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dimitri Glazkov (Google)
URL:
Keywords:
Depends on: 62748 62756 62931 63039 63222 63224
Blocks: 62153
  Show dependency treegraph
 
Reported: 2011-06-03 16:48 PDT by Dimitri Glazkov (Google)
Modified: 2011-06-22 23:04 PDT (History)
5 users (show)

See Also:


Attachments
WIP: First stab. (26.20 KB, patch)
2011-06-03 16:49 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Patch (28.10 KB, patch)
2011-06-06 14:50 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Patch (23.17 KB, patch)
2011-06-22 22:45 PDT, Dimitri Glazkov (Google)
tkent: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>