Bug 62069

Created attachment 95988 [details] WIP: First stab.

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 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 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 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 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?

Created attachment 96128 [details] Patch

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

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

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

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

Created attachment 98314 [details] Patch

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.

Thanks for review! Will fix nits upon landing.

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