Bug 67228 - [Qt] [WK2] implement support to upload files in Qt WebKit2
Summary: [Qt] [WK2] implement support to upload files in Qt WebKit2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Normal
Assignee: Igor Trindade Oliveira
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2011-08-30 14:01 PDT by Igor Trindade Oliveira
Modified: 2011-09-13 03:43 PDT (History)
6 users (show)

See Also:


Attachments
Patch. (9.18 KB, patch)
2011-08-30 14:49 PDT, Igor Trindade Oliveira
no flags Details | Formatted Diff | Diff
Patch. (11.51 KB, patch)
2011-08-31 14:49 PDT, Igor Trindade Oliveira
no flags Details | Formatted Diff | Diff
Patch (11.33 KB, patch)
2011-08-31 16:41 PDT, Igor Trindade Oliveira
no flags Details | Formatted Diff | Diff
Patch. (11.04 KB, patch)
2011-09-09 13:45 PDT, Igor Trindade Oliveira
kling: review+
kling: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Igor Trindade Oliveira 2011-08-30 14:01:33 PDT
Implement support to upload files in Qt WebKit2.
Comment 1 Igor Trindade Oliveira 2011-08-30 14:49:56 PDT
Created attachment 105703 [details]
Patch.

Proposed patch.
Comment 2 Caio Marcelo de Oliveira Filho 2011-08-30 15:19:51 PDT
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 3 Andreas Kling 2011-08-30 15:21:09 PDT
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 4 Alexis Menard (darktears) 2011-08-30 15:33:57 PDT
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
Comment 5 Igor Trindade Oliveira 2011-08-31 08:04:27 PDT
(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.
Comment 6 Igor Trindade Oliveira 2011-08-31 08:06:46 PDT
(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
Comment 7 Igor Trindade Oliveira 2011-08-31 14:49:24 PDT
Created attachment 105834 [details]
Patch.

Proposed patch.
Comment 8 Caio Marcelo de Oliveira Filho 2011-08-31 15:19:34 PDT
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?
Comment 9 Igor Trindade Oliveira 2011-08-31 16:04:09 PDT
(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.
Comment 10 Igor Trindade Oliveira 2011-08-31 16:41:00 PDT
Created attachment 105858 [details]
Patch

Proposed patch. Add enum to handle file chooser types and fix leftover from previous patch.
Comment 11 Andreas Kling 2011-09-05 05:18:31 PDT
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.)
Comment 12 Andreas Kling 2011-09-05 05:21:21 PDT
(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.)
Comment 13 Caio Marcelo de Oliveira Filho 2011-09-05 05:39:40 PDT
(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/ :-)
Comment 14 Kenneth Rohde Christiansen 2011-09-05 05:43:00 PDT
(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.
Comment 15 Andreas Kling 2011-09-05 05:46:40 PDT
(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.
Comment 16 Kenneth Rohde Christiansen 2011-09-05 05:54:46 PDT
(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.
Comment 17 Caio Marcelo de Oliveira Filho 2011-09-05 05:56:26 PDT
(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. ;-)
Comment 18 Igor Trindade Oliveira 2011-09-09 13:45:19 PDT
Created attachment 106914 [details]
Patch.

Proposed patch.
Comment 19 Andreas Kling 2011-09-12 08:52:34 PDT
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.
Comment 20 Andreas Kling 2011-09-12 08:53:24 PDT
(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.
Comment 21 Igor Trindade Oliveira 2011-09-13 03:43:51 PDT
manually committed r94979: http://trac.webkit.org/changeset/94979