Bug 67228 – [Qt] [WK2] implement support to upload files in Qt WebKit2

• Component: New Bugs
• Status: RESOLVED FIXED

[Igor Trindade Oliveira]

Implement support to upload files in Qt WebKit2.

[Igor Trindade Oliveira]

Created attachment 105703 [details]
Patch.

Proposed patch.

[Caio Marcelo de Oliveira Filho]

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.

[Andreas Kling]

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.

[Alexis Menard (darktears)]

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

[Igor Trindade Oliveira]

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

[Igor Trindade Oliveira]

(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

[Igor Trindade Oliveira]

Created attachment 105834 [details]
Patch.

Proposed patch.

[Caio Marcelo de Oliveira Filho]

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?

[Igor Trindade Oliveira]

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

[Igor Trindade Oliveira]

Created attachment 105858 [details]
Patch

Proposed patch. Add enum to handle file chooser types and fix leftover from previous patch.

[Andreas Kling]

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

[Andreas Kling]

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

[Caio Marcelo de Oliveira Filho]

(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/ :-)

[Kenneth Rohde Christiansen]

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

[Andreas Kling]

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

[Kenneth Rohde Christiansen]

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

[Caio Marcelo de Oliveira Filho]

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

[Igor Trindade Oliveira]

Created attachment 106914 [details]
Patch.

Proposed patch.

[Andreas Kling]

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.

[Andreas Kling]

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

[Igor Trindade Oliveira]

manually committed r94979: http://trac.webkit.org/changeset/94979