Summary: | [Qt] [WK2] implement support to upload files in Qt WebKit2 | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Igor Trindade Oliveira <igor.oliveira> | ||||||||||
Component: | New Bugs | Assignee: | Igor Trindade Oliveira <igor.oliveira> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | benjamin, cmarcelo, igor.oliveira, jesus, kenneth, kling | ||||||||||
Priority: | P3 | Keywords: | Qt | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Igor Trindade Oliveira
2011-08-30 14:01:33 PDT
Created attachment 105703 [details]
Patch.
Proposed patch.
Comment on attachment 105703 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=105703&action=review I have a few comments and questions. > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:387 > + QWidget* widget = q->canvas(); I think there's an extra space after "=". > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:393 > + fileNames = QFileDialog::getOpenFileNames(widget, QString::null, suggestedFiles); > + else > + fileNames += QFileDialog::getOpenFileName(widget, QString::null, suggestedFiles); Why QString::null instead of QString()? Isn't the third argument of getOpenFileNames/getOpenFileNames the directory? > Source/WebKit2/UIProcess/qt/ClientImpl.cpp:163 > + const QStringList fileList = toViewInterface(clientInfo)->chooseFiles(suggestedFileNames.join(QLatin1String(",")), allowsMultipleFiles); Why are suggested file names joined? > Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:157 > + uiClient.runOpenPanel = qt_wk_runOpenPanel; Extra space after "=". > Source/WebKit2/UIProcess/qt/ViewInterface.h:81 > + virtual QStringList chooseFiles(const QString& suggestedFiles, bool AllowsMultipleFiles) = 0; "AllowsMultipleFiles" starting with uppercase is wrong. By the way, could we get rid of the boolean and use an enum or a different design instead? I also don't think ViewInterface should get the suggestedFiles joined. If Qt QFileDialog API demands this join, we do in our implementations, not on ViewInterface. Comment on attachment 105703 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=105703&action=review > Source/WebKit2/UIProcess/API/C/WKOpenPanelParameters.cpp:43 > + Unnecessary whitespace change. > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:384 > +QStringList QDesktopWebViewPrivate::chooseFiles(const QString& suggestedFiles, bool allowsMultipleFiles) Should be "allowMultipleFiles" > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:387 > + QWidget* widget = q->canvas(); Coding style, double spaces after = > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:391 > + fileNames = QFileDialog::getOpenFileNames(widget, QString::null, suggestedFiles); QString::null -> QString() > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:393 > + fileNames += QFileDialog::getOpenFileName(widget, QString::null, suggestedFiles); Ditto. > Source/WebKit2/UIProcess/API/qt/qdesktopwebview_p.h:75 > + virtual QStringList chooseFiles(const QString& suggestedFiles, bool allowsMultipleFiles); This belongs in QDesktopWebPageProxy IMO. > Source/WebKit2/UIProcess/qt/ClientImpl.cpp:153 > +void qt_wk_runOpenPanel(WKPageRef, WKFrameRef, WKOpenPanelParametersRef parameters, WKOpenPanelResultListenerRef listener, const void *clientInfo) Coding style, * placement. > Source/WebKit2/UIProcess/qt/ClientImpl.cpp:162 > + const bool allowsMultipleFiles = WKOpenPanelParametersGetAllowsMultipleFiles(parameters) ? true : false; No need for the "? true : false" part. > Source/WebKit2/UIProcess/qt/ClientImpl.cpp:165 > + if (!fileList.size()) { Would read better as "if (fileList.isEmpty()) {" > Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:157 > + uiClient.runOpenPanel = qt_wk_runOpenPanel; Style, two spaces after = > Source/WebKit2/UIProcess/qt/TouchViewInterface.h:78 > + virtual QStringList chooseFiles(const QString& suggestedFile, bool allowsMultipleFiles) { return QStringList(); } Inconsistent with base class, suggestedFile -> suggestedFiles. Comment on attachment 105703 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=105703&action=review >> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:391 >> + fileNames = QFileDialog::getOpenFileNames(widget, QString::null, suggestedFiles); > > QString::null -> QString() Perhaps a nice title here wouldn't hurt. I think the call is broken here the working directory is the third parameters. > Source/WebKit2/UIProcess/qt/ClientImpl.cpp:155 > + Vector<String> wkSuggestedFileNames = toImpl(parameters)->suggestedFileNames(); So we use the C API on one side but then the C++ also. mmmm (In reply to comment #2) > (From update of attachment 105703 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=105703&action=review > > I have a few comments and questions. > > > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:387 > > + QWidget* widget = q->canvas(); > > I think there's an extra space after "=". Ok > > > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:393 > > + fileNames = QFileDialog::getOpenFileNames(widget, QString::null, suggestedFiles); > > + else > > + fileNames += QFileDialog::getOpenFileName(widget, QString::null, suggestedFiles); > > Why QString::null instead of QString()? A leftover in the patch iterations. > > Isn't the third argument of getOpenFileNames/getOpenFileNames the directory? > In fact i need rename the method and argument name. The right name should be "selectedFiles". > > Source/WebKit2/UIProcess/qt/ClientImpl.cpp:163 > > + const QStringList fileList = toViewInterface(clientInfo)->chooseFiles(suggestedFileNames.join(QLatin1String(",")), allowsMultipleFiles); > > Why are suggested file names joined? It need to be a QString separated by ",". > > > Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:157 > > + uiClient.runOpenPanel = qt_wk_runOpenPanel; > > Extra space after "=". Ok > > > Source/WebKit2/UIProcess/qt/ViewInterface.h:81 > > + virtual QStringList chooseFiles(const QString& suggestedFiles, bool AllowsMultipleFiles) = 0; > > "AllowsMultipleFiles" starting with uppercase is wrong. By the way, could we get rid of the boolean and use an enum or a different design instead? > Yeah, IIRC WK1 uses a enum. > I also don't think ViewInterface should get the suggestedFiles joined. If Qt QFileDialog API demands this join, we do in our implementations, not on ViewInterface. Yeah. Already discussed in irc. (In reply to comment #3) > (From update of attachment 105703 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=105703&action=review > > > Source/WebKit2/UIProcess/API/C/WKOpenPanelParameters.cpp:43 > > + > > Unnecessary whitespace change. > Ok > > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:384 > > +QStringList QDesktopWebViewPrivate::chooseFiles(const QString& suggestedFiles, bool allowsMultipleFiles) > > Should be "allowMultipleFiles" > Ok > > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:387 > > + QWidget* widget = q->canvas(); > > Coding style, double spaces after = Ok > > > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:391 > > + fileNames = QFileDialog::getOpenFileNames(widget, QString::null, suggestedFiles); > > QString::null -> QString() > > > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:393 > > + fileNames += QFileDialog::getOpenFileName(widget, QString::null, suggestedFiles); > > Ditto. > > > Source/WebKit2/UIProcess/API/qt/qdesktopwebview_p.h:75 > > + virtual QStringList chooseFiles(const QString& suggestedFiles, bool allowsMultipleFiles); > > This belongs in QDesktopWebPageProxy IMO. > Already discussed in irc. > > Source/WebKit2/UIProcess/qt/ClientImpl.cpp:153 > > +void qt_wk_runOpenPanel(WKPageRef, WKFrameRef, WKOpenPanelParametersRef parameters, WKOpenPanelResultListenerRef listener, const void *clientInfo) > > Coding style, * placement. > > > Source/WebKit2/UIProcess/qt/ClientImpl.cpp:162 > > + const bool allowsMultipleFiles = WKOpenPanelParametersGetAllowsMultipleFiles(parameters) ? true : false; > > No need for the "? true : false" part. > puff. A leftover in the patch iterations. It is a shame. > > Source/WebKit2/UIProcess/qt/ClientImpl.cpp:165 > > + if (!fileList.size()) { > > Would read better as "if (fileList.isEmpty()) {" Agree. > > > Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:157 > > + uiClient.runOpenPanel = qt_wk_runOpenPanel; > > Style, two spaces after = > Ok > > Source/WebKit2/UIProcess/qt/TouchViewInterface.h:78 > > + virtual QStringList chooseFiles(const QString& suggestedFile, bool allowsMultipleFiles) { return QStringList(); } > > Inconsistent with base class, suggestedFile -> suggestedFiles. Ok Created attachment 105834 [details]
Patch.
Proposed patch.
Comment on attachment 105834 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=105834&action=review LGTM except for the boolean comment below. > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:31 > +#include <QtGui/QFileDialog> Left over from previous version? > Source/WebKit2/UIProcess/qt/qdesktopwebpageproxy.h:63 > + QStringList chooseFiles(const QStringList& selectedFileNames, bool allowMultipleFiles); What about an enumeration instead of boolean? (In reply to comment #8) > (From update of attachment 105834 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=105834&action=review > > LGTM except for the boolean comment below. > > > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:31 > > +#include <QtGui/QFileDialog> > > Left over from previous version? > ouch! > > Source/WebKit2/UIProcess/qt/qdesktopwebpageproxy.h:63 > > + QStringList chooseFiles(const QStringList& selectedFileNames, bool allowMultipleFiles); > > What about an enumeration instead of boolean? Ok. You convinced me. Created attachment 105858 [details]
Patch
Proposed patch. Add enum to handle file chooser types and fix leftover from previous patch.
Comment on attachment 105858 [details]
Patch
There are two things I don't like with this patch.
Number one: After (way too much) arguing with Benjamin I have to admit I was wrong about QtWebPageProxy/ViewInterface. This does indeed belong on the ViewInterface, as that's kind of a "PageClient jr". We may need to bundle the pointers for ClientInfo* purposes later, but this is not the time and place. Sorry about that.
The second problem is with the way file choosing is implemented in a nested event loop (happens by calling QFileDialog::getOpenFileName{,s}). This has caused a LOT of trouble (reentrancy issues with QNAM for example) for QtWebKit in the past, and many of these problems are not fixable with the Qt4 API. So I'm hesitant to introduce more nesting.
My question then becomes: would it be possible to change WebKit to make this a two-step process? (In a way that we can move the dialog to the main event loop instead of nesting a new one.)
(In reply to comment #11) > My question then becomes: would it be possible to change WebKit to make this a two-step process? (In a way that we can move the dialog to the main event loop instead of nesting a new one.) By two-step process I mean that the chooseFiles() implementation spawns a file selection dialog, and we then have an asynchronous callback that runs when the dialog is finished. I don't know if this would introduce problems on ports that have (sane) synchronous dialog boxes that don't nest (and what ports that might be.) (In reply to comment #11) > My question then becomes: would it be possible to change WebKit to make this a two-step process? (In a way that we can move the dialog to the main event loop instead of nesting a new one.) I don't think WebKit need to be changed. Currently, to answer the sync query made from the WebProcess we have to call the appropriate "listener" function, like "WKOpenPanelResultListenerChooseFiles();". WebProcess will be blocked until some listener is called (in the sync case). So what we can do: delay the listener call until some signal is emitted by the dialog might be the solution for this. Igor, this might be useful reading about dialogs/nested loops http://labs.qt.nokia.com/2010/02/23/unpredictable-exec/ :-) (In reply to comment #13) > (In reply to comment #11) > > > My question then becomes: would it be possible to change WebKit to make this a two-step process? (In a way that we can move the dialog to the main event loop instead of nesting a new one.) > > I don't think WebKit need to be changed. Currently, to answer the sync query made from the WebProcess we have to call the appropriate "listener" function, like "WKOpenPanelResultListenerChooseFiles();". WebProcess will be blocked until some listener is called (in the sync case). > > So what we can do: delay the listener call until some signal is emitted by the dialog might be the solution for this. > > Igor, this might be useful reading about dialogs/nested loops > http://labs.qt.nokia.com/2010/02/23/unpredictable-exec/ :-) To build a bit on our experience from the N9 browser; we want to avoid sync calls by all cost. Think about it, on a mobile device we can have interruptions at all times like receiving a phone call. (In reply to comment #14) > (In reply to comment #13) > > (In reply to comment #11) > > > > > My question then becomes: would it be possible to change WebKit to make this a two-step process? (In a way that we can move the dialog to the main event loop instead of nesting a new one.) > > > > I don't think WebKit need to be changed. Currently, to answer the sync query made from the WebProcess we have to call the appropriate "listener" function, like "WKOpenPanelResultListenerChooseFiles();". WebProcess will be blocked until some listener is called (in the sync case). > > > > So what we can do: delay the listener call until some signal is emitted by the dialog might be the solution for this. > > > > Igor, this might be useful reading about dialogs/nested loops > > http://labs.qt.nokia.com/2010/02/23/unpredictable-exec/ :-) > > To build a bit on our experience from the N9 browser; we want to avoid sync calls by all cost. Think about it, on a mobile device we can have interruptions at all times like receiving a phone call. I don't see the harm in blocking the WebProcess here, surely the phone stack will still bring your call through? Blocking the UIProcess OTOH is a no-no, of course. (In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #13) > > > (In reply to comment #11) > > > > > > > My question then becomes: would it be possible to change WebKit to make this a two-step process? (In a way that we can move the dialog to the main event loop instead of nesting a new one.) > > > > > > I don't think WebKit need to be changed. Currently, to answer the sync query made from the WebProcess we have to call the appropriate "listener" function, like "WKOpenPanelResultListenerChooseFiles();". WebProcess will be blocked until some listener is called (in the sync case). > > > > > > So what we can do: delay the listener call until some signal is emitted by the dialog might be the solution for this. > > > > > > Igor, this might be useful reading about dialogs/nested loops > > > http://labs.qt.nokia.com/2010/02/23/unpredictable-exec/ :-) > > > > To build a bit on our experience from the N9 browser; we want to avoid sync calls by all cost. Think about it, on a mobile device we can have interruptions at all times like receiving a phone call. > > I don't see the harm in blocking the WebProcess here, surely the phone stack will still bring your call through? Blocking the UIProcess OTOH is a no-no, of course. Well, it is complicated... on the N9 for instance this might close the vkb (there is only one vkb for the whole platform) and close the dialog; getting back to the right state after ending the phone call etc is the problem. Yes, you can work around some of these issues, but sometimes it becomes quite complicated. (In reply to comment #13) > I don't think WebKit need to be changed. Currently, to answer the sync query made from the WebProcess we have to call the appropriate "listener" function, like "WKOpenPanelResultListenerChooseFiles();". WebProcess will be blocked until some listener is called (in the sync case). Guys, Igor just corrected me. The WebProcess query isn't Sync, it's Async already. :-) Note that solution is the same, use open() for dialog, watch a dialog signal and only then call the listener function. ;-) Created attachment 106914 [details]
Patch.
Proposed patch.
Comment on attachment 106914 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=106914&action=review r=me > Source/WebKit2/UIProcess/qt/TouchViewInterface.h:78 > + virtual void chooseFiles(WKOpenPanelResultListenerRef listenerRef, const QStringList& selectedFileNames, FileChooserType type) { } Unnecessary parameter names: listenerRef, type. (In reply to comment #19) > > Source/WebKit2/UIProcess/qt/TouchViewInterface.h:78 > > + virtual void chooseFiles(WKOpenPanelResultListenerRef listenerRef, const QStringList& selectedFileNames, FileChooserType type) { } > > Unnecessary parameter names: listenerRef, type. Actually, all of them are unnecessary, since this is inline. D'oh. manually committed r94979: http://trac.webkit.org/changeset/94979 |