Bug 68962 – [Qt][WK2] Download support and API in UIProcess

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Jesus Sanchez-Palencia]

We need to add the needed implementation to UIProcess in order to fully support Download in Qt WebKit2. We also need to expose this somehow through APIs.

[Jesus Sanchez-Palencia]

Created attachment 108944 [details]
Patch

[Jesus Sanchez-Palencia]

Created attachment 109065 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 109065 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109065&action=review

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:445
> +QString QDesktopWebViewPrivate::chooseDestinationFile(const QString& suggestedFileName)

This need to be exposed in the API, in an asynchronous manner since user interaction might be needed.
I think we should try to get the API as complete as possible on the first iteration, since adding a missing piece later might require a total redesign if it doesn't fit in the chosen model.

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:740
> +    downloadItem->deleteLater();

The user code might still have a reference to this object. API objects should not be destroyed except through an explicit destroy, a cleanup method call, or the destruction of it's owner/parent. A bit like QNetworkReply.

On the other end, we also don't want to leak those objects until the page is destroyed if the user code doesn't handle downloads, so we might have to find a compromise.

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:748
> +    downloadItem->deleteLater();

ditto

[Andreas Kling]

(In reply to comment #3)
> (From update of attachment 109065 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109065&action=review
> > Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:740
> > +    downloadItem->deleteLater();
> 
> The user code might still have a reference to this object. API objects should not be destroyed except through an explicit destroy, a cleanup method call, or the destruction of it's owner/parent. A bit like QNetworkReply.
> 
> On the other end, we also don't want to leak those objects until the page is destroyed if the user code doesn't handle downloads, so we might have to find a compromise.

Perhaps it could work like QWebPage::unsupportedContent(), i.e if you do the equivalent of QWebPage::setForwardUnsupportedContent(true), you get the signal, and your slot takes ownership of the item.

[Jesus Sanchez-Palencia]

Thanks for the feedback!

(In reply to comment #3)
> > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:445
> > +QString QDesktopWebViewPrivate::chooseDestinationFile(const QString& suggestedFileName)
> 
> This need to be exposed in the API, in an asynchronous manner since user interaction might be needed.
> I think we should try to get the API as complete as possible on the first iteration, since adding a missing piece later might require a total redesign if it doesn't fit in the chosen model.

This will require new API to be added to Download.h, DownloadProxy.h and DownloadProxy.messages as well, since currently there is no async way to decide the destination path. Should I fix this on a separate patch?

After having this async API, what about having something like a virtual QDesktopWebView::chooseDestinationPath(const QString& suggestedFilename) and a slot QDesktopWebView::destinationChosen(const QString& chosenPath) .

The default implementation would just call a private function that creates a QFileDialog and connects its finished() signal to QDesktopWebView::destinationChosen. If this is not the desired behavior, then the user can override QDesktopWebView::chooseDestinationPath doing whatever he wants to and then calling or connecting something to QDesktopWebView::destinationChosen.

This slot would call a private function that would call QtWebPageProxy::didDecidedDestination, that will also be added, responsible for using DownloadProxy to send a message back to the WebProcess with the chosen file path.

Does it sound reasonable?


(In reply to comment #4)
> Perhaps it could work like QWebPage::unsupportedContent(), i.e if you do the equivalent of QWebPage::setForwardUnsupportedContent(true), you get the signal, and your slot takes ownership of the item.

So then we could have something like QDesktopWebView::setHandleDownloadItem(bool), set as false by default, and a signal QDesktopWebView::handleDownloadItem(QWebDownloadItem*). QtWebPageProxy would then check if it should emit this signal or if it should deleteLater the download item.

[Jesus Sanchez-Palencia]

(In reply to comment #5)
> (In reply to comment #4)
> > Perhaps it could work like QWebPage::unsupportedContent(), i.e if you do the equivalent of QWebPage::setForwardUnsupportedContent(true), you get the signal, and your slot takes ownership of the item.
> 
> So then we could have something like QDesktopWebView::setHandleDownloadItem(bool), set as false by default, and a signal QDesktopWebView::handleDownloadItem(QWebDownloadItem*). QtWebPageProxy would then check if it should emit this signal or if it should deleteLater the download item.


Me, Caio (cmarcelo) and Kling just had a fruitful discussion on irc and we realized that an option here would be just giving the ownership of a QWebDownloadItem to the WebViews. This would be done, i.e., in QDesktopWebViewPrivate::downloadStarted (by downloadItem->setParent(q)). The deleteLaters would be removed from QtWebPageProxy and I could modify finished() and failed() to finished(QWebDownloadItem*) and failed(QWebDownloadItem*) so the API user could be able to deleteLater a QWebDownloadItem if he wanted to.

What do you think?

[Caio Marcelo de Oliveira Filho]

Just adding my 2 cents here.

One idea that popped up was to expose this API in QML this way:

WebView {
    // ### We can also use an explicit "downloadDelegate:" property here too instead.
    DownloadDelegate {
        onDownloadStarted: { console.log(download.someInformation); }
        onDownloadProgressChanged: {
            console.log(download.someInformation + " " + progress;
            if (download.anotherInformation === "bla")
                download.cancel();
        }
        onDownloadFinished { download.destroy(); }
    }
}

The idea here is to avoid us making explicity connections in the javascript side as well as not exposing those as signals of WebView to not bloat it (and also to avoid double work because Touch/Desktop). QWebDownloadItem* is passed as 'download' parameter in those signals.

As a bonus, if there's no DownloadDelegate, we can have a default behavior in Qt if we want. Either JustWorksTM or ignoring all downloads.

Ownership rules would follow the same approach of QNAM/QNetworkReply Jesus described.

[Caio Marcelo de Oliveira Filho]

(In reply to comment #7)
>     DownloadDelegate {
>         onDownloadStarted: { console.log(download.someInformation); }
>         onDownloadProgressChanged: {

Well, this convenience might not be that useful. After talking here with Adriano and Jesus, for progress changed we most of the time want to use property bindings to update things. In many cases we'll push this download object to the hands of some sort of model.

[Jocelyn Turcotte]

(In reply to comment #5)
> This will require new API to be added to Download.h, DownloadProxy.h and DownloadProxy.messages as well, since currently there is no async way to decide the destination path. Should I fix this on a separate patch?

As far as I know, Apple needed a sync message since their network stack required it, but ours doesn't, and if we can avoid a sync message we should. This can be done in the same patch I think.

> After having this async API, what about having something like a virtual QDesktopWebView::chooseDestinationPath(const QString& suggestedFilename) and a slot QDesktopWebView::destinationChosen(const QString& chosenPath) .
> 
> The default implementation would just call a private function that creates a QFileDialog and connects its finished() signal to QDesktopWebView::destinationChosen. If this is not the desired behavior, then the user can override QDesktopWebView::chooseDestinationPath doing whatever he wants to and then calling or connecting something to QDesktopWebView::destinationChosen.
> 
> This slot would call a private function that would call QtWebPageProxy::didDecidedDestination, that will also be added, responsible for using DownloadProxy to send a message back to the WebProcess with the chosen file path.
> 
> Does it sound reasonable?

It would be nice if common web use cases of the views could be implemented in pure QML/JavaScript, no C++. Also we are trying to remove our dependencies to QtGui to depend only on QtBase, so QFileDialog should be avoided.

We don't have to expose all the web process calls in the API, so we could actually simplify the workflow like this:
- The user clicks on a link, the HTTP response header are received, WebCore determines that it can't support the mime type, the download policy is chosen.
- The web process creates a Download using the response, QtFileDownloader determines the suggested filename right away.
- QDesktopWebView::downloadRequested(QWebDownloadItem) is emitted, QWebDownloadItem contains the guessed file name in suggestedFileName() or destinationPath().
- If the user code wants to start the download, it calls setDestinationPath() and the start() slot. Else it call ignore()/cancel().

This is just an idea, but what I mean is that there is no benefit of having API callbacks for both started and decideDestination.
It would be nice to allow as many actions as possible to be performed on QWebDownloadItem instead of the web view.

> So then we could have something like QDesktopWebView::setHandleDownloadItem(bool), set as false by default, and a signal QDesktopWebView::handleDownloadItem(QWebDownloadItem*). QtWebPageProxy would then check if it should emit this signal or if it should deleteLater the download item.

If we want to disable downloads unless setHandleDownloadItem is true, then I think we could avoid the Download being created completely by avoiding calling WKFramePolicyListenerDownload in qt_wk_decidePolicyForNavigationAction.

[Jesus Sanchez-Palencia]

Created attachment 109789 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 109789 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109789&action=review

> Source/WebKit2/UIProcess/API/C/WKContext.h:104
> +    WKContextDownloadDecideDestinationWithSuggestedFilenameAsyncCallback decideDestinationWithSuggestedFilenameAsync;

Humm... I didn't realize that this would lead to have to add this function to the C API.
We can't really have this there along with the sync API, and there is not really a mechanism in place to allow us to get this message without going through the C API.
Luckily though, since what we need is only the suggested filename, the ResourceResponse has a suggestedFileName field that we can use in the didReceiveResponse message.

So I would suggest to not use the "decideDestination" message(s) at all, pass the suggested filename in the didReceiveResponse message, and rename the DidDecideDownloadDestinationAsync message back to the WebProcess you added to something like "StartTransfer(uint64_t downloadID, WTF::String destinationFileName)".
The downloadRequested signal would be emitted following didReceiveResponse instead of decideDestinationWithSuggestedFilename, and QtFileDownloader::decidedDestination could be merged with QtFileDownloader::start.

Does that sound right?

> Source/WebKit2/UIProcess/API/qt/qwebdownloaditem.h:66
> +    void setDestinationPath(const QString& destination);

You shouldn't need this as a slot since you already have it as a property writer.

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:738
> +    QWebDownloadItem* downloadItem = m_downloads.take(download->downloadID());

We still need a default behavior that doesn't leak the items if the user code doesn't handle them.
I think it would be better to add something directly in QDesktopWebView to set downloads enabled now, but disabled by default.
We can move it to a future QWebSettings equivalent when we have it.

[Kenneth Rohde Christiansen]

Comment on attachment 109789 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109789&action=review

> Source/WebKit2/Shared/qt/WebCoreArgumentCodersQt.cpp:63
> +    encoder->encode(resourceResponse.url().string());
> +    encoder->encode(resourceResponse.mimeType());
> +    encoder->encode(static_cast<int64_t>(resourceResponse.expectedContentLength()));
> +    encoder->encode(resourceResponse.textEncodingName());

Isn't it possible to bulk decode these? We are doing that for EditorState I believe

> Source/WebKit2/UIProcess/API/qt/qwebdownloaditem.cpp:93
> +    bool allowOverwrite = true; // Currently, we don't use this.

Override ? Sorry im not sure about the actual english difference between override and overwrite. Would be good to check :-)

> Source/WebKit2/UIProcess/API/qt/qwebdownloaditem.h:50
> +        FileAlreadyExists,

DestinationAlreadyExists? It could be a dir right?

> Source/WebKit2/UIProcess/API/qt/qwebdownloaditem.h:51
> +        CancelledByCaller,

Could it be cancelled by others?

> Source/WebKit2/UIProcess/Downloads/DownloadProxy.cpp:190
> +void DownloadProxy::decideDestinationWithSuggestedFilenameAsync(const String& filename)

Is it common to append Async in webkit2? I don't believe so, but things can have diverted after our branch

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:163
> +    memset(&downloadClient, 0, sizeof(WKContextDownloadClient));

Are we not setting everything?

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:718
> +    QWebDownloadItem* downloadItem = new QWebDownloadItem(0);

The 0 is confusing... what does it mean? Could it be a default value?

> Source/WebKit2/WebProcess/Downloads/DownloadManager.h:61
> +    void didDecideDownloadDestinationAsync(uint64_t downloadID, const WTF::String& destination, bool allowOverwrite);

Maybe "replaceIfExists" is better than allowOverwrite.

[Jocelyn Turcotte]

Comment on attachment 109789 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109789&action=review

Other details that could be worth looking at:

> Source/WebKit2/UIProcess/API/qt/qwebdownloaditem.h:41
> +    Q_PROPERTY(quint64 contentLength READ expectedContentLength CONSTANT FINAL)
> +    Q_PROPERTY(QString destination READ destinationPath WRITE setDestinationPath)
> +    Q_PROPERTY(QString suggestedFilename READ suggestedFilename CONSTANT FINAL)
> +    Q_PROPERTY(quint64 totalBytesReceived READ totalBytesReceived NOTIFY dataReceived FINAL)
> +    Q_PROPERTY(QUrl url READ sourceUrl CONSTANT FINAL)

Also, please name the properties the same as their accessors.

> Source/WebKit2/UIProcess/API/qt/qwebdownloaditem.h:65
> +    void cancel();

This should lead to a CancelledByCaller error, and I might be missing something but I think that in the current implementation you will get a NetworkFailure error instead.

> Source/WebKit2/UIProcess/API/qt/qwebdownloaditem.h:71
> +    void finished();

This could be named "succeeded" to make it consistent with the web view signals and clear that this won't be emitted when the download fails.

[Jesus Sanchez-Palencia]

Thanks for the reviews, guys!
I will reply to a few comments before uploading the new patch. Everything else was, somehow, addressed on it.

(In reply to comment #11)
> The downloadRequested signal would be emitted following didReceiveResponse instead of decideDestinationWithSuggestedFilename, and QtFileDownloader::decidedDestination could be merged with QtFileDownloader::start.
> 
> Does that sound right?

Yes, everything else but merging QtFileDownloader::start with QtFileDownloader::decidedDestination. In fact, QtFileDownloader::start could be called init() since all we need is to connect the signals outside its ctor and decidedDestination() can become something like startTransfer().

> We still need a default behavior that doesn't leak the items if the user code doesn't handle them.
> I think it would be better to add something directly in QDesktopWebView to set downloads enabled now, but disabled by default.
> We can move it to a future QWebSettings equivalent when we have it.

On the current patch every time a download is requested, a QWebDownloadItem is created and has its ownership given to the WebView (we call downloadItem->setParent(q) on QDesktopWebViewPrivate::downloadRequested).


(In reply to comment #12)
> > Source/WebKit2/UIProcess/API/qt/qwebdownloaditem.cpp:93
> > +    bool allowOverwrite = true; // Currently, we don't use this.
> 
> Override ? Sorry im not sure about the actual english difference between override and overwrite. Would be good to check :-)

It is like this because it came from Apple's implementation of Download.h, so I guess the english is fine. On any case, this will be removed on the upcoming patch... :)

> > Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:718
> > +    QWebDownloadItem* downloadItem = new QWebDownloadItem(0);
> 
> The 0 is confusing... what does it mean? Could it be a default value?

It means a null parent for now, since the parent will only be set by viewInterface->downloadRequested(). I will remove it on the next patch.

[Jesus Sanchez-Palencia]

Created attachment 111369 [details]
Patch

[Simon Hausmann]

Comment on attachment 111369 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111369&action=review

> Source/WebKit2/ChangeLog:10
> +        We also add APIs by exposing QWebDownloadItem. Objects from this type
> +        have their ownership handled by QDesktopWebView and will be created or

I'm wondering about this ownership model. Imagine a web browser with tabs, where each tab is one DesktopWebView and you're downloading a file.
Wouldn't you imagine the download to continue after closing the tab that started the download?

[Jesus Sanchez-Palencia]

Comment on attachment 111369 [details]
Patch

I've found an issue with the current design when dealing with multiple WebViews so I'm clearing the review flag while the new patch is being cooked.

[Jesus Sanchez-Palencia]

Created attachment 111709 [details]
Patch

[Jesus Sanchez-Palencia]

(In reply to comment #18)
> Created an attachment (id=111709) [details]
> Patch

The previous API design was kept but the internals have changed. It was needed to keep downloads related to the WebContext instead of QtWebPageProxy, but still keeping it related to the originating page. This solves the multiple pages/webviews use case and keep the download items alive while the WebContext is alive. Now, for instance, a browser can start downloads in several tabs/webviews, close the originating ones and keep the ongoing downloads and keep track of their progress. This wasn't possible with the previous approach.

The ownership issue was solved by giving the download items to the javascript engine as soon as the webview receives downloadRequested through its ViewInterface.

I will work on an API test (qmltest) but having a feedback on this patch would be great!

[Simon Hausmann]

Comment on attachment 111709 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111709&action=review

> Source/WebKit2/UIProcess/WebPageProxy.cpp:972
> +    DownloadProxy* download;
>      uint64_t downloadID = 0;
>      if (action == PolicyDownload) {
>          // Create a download proxy.
> -        downloadID = m_process->context()->createDownloadProxy()->downloadID();
> +        download = m_process->context()->createDownloadProxy();
> +        downloadID = download->downloadID();
> +#if PLATFORM(QT)
> +        download->setInitiatingPage(this);
> +#endif

Why is the download variable declared outside of the if {} scope? Wouldn't it be easier to replace

    downloadID = ...

with

    DownloadProxy* download = ...
    downloadID = download->downloadID();

?

> Source/WebKit2/UIProcess/qt/QtDownloadManager.cpp:75
> +    download->initiatingPage()->pageClient()->qtWebPageProxy()->downloadRequested(downloadItem);

This is the only part that bugs me, because it required adding all those accessors to seemingly unrelated code.

I see however that it's the entry-point in the API, so either we stick to it or we find a different way of signalling the downloads to the application.

> Source/WebKit2/WebProcess/WebProcess.messages.in:67
> +#if PLATFORM(QT)

Oh, you changed your mind in favour of an #ifdef instead of a comment? :) (no problem with that though, I see that the #ifdef makes the patch minimal)

[Jesus Sanchez-Palencia]

Created attachment 111775 [details]
Patch

[Jesus Sanchez-Palencia]

(In reply to comment #21)
> Created an attachment (id=111775) [details]
> Patch

Patch addressing the previous comments.


(In reply to comment #20)
> Why is the download variable declared outside of the if {} scope? Wouldn't it be easier to replace
> 

Sure! Fixed.

> 
> > Source/WebKit2/UIProcess/qt/QtDownloadManager.cpp:75
> > +    download->initiatingPage()->pageClient()->qtWebPageProxy()->downloadRequested(downloadItem);
> 
> This is the only part that bugs me, because it required adding all those accessors to seemingly unrelated code.

Yeah, me too... On this new patch I have removed the need of DownloadProxy to know the WebPage and, therefore, QtDownloadManager does not deal with it anymore. Instead, the PageClient (QtWebPageProxy) is used to bootstrap a download item and keep it with the download manager but then it's only when we receive the downloadResponse that we will actually fill the item with the needed information and emit WebView::downloadRequested, as before. IMHO, this seems to be a cleaner approach, keeping the internal design aligned and the previous proposed API.

> 
> > Source/WebKit2/WebProcess/WebProcess.messages.in:67
> > +#if PLATFORM(QT)
> 
> Oh, you changed your mind in favour of an #ifdef instead of a comment? :) (no problem with that though, I see that the #ifdef makes the patch minimal)

Yes =/. I decided to go for the ifdefs after I realized the amount of Qt-specific code I was adding anyway. I recognize it as a failure of my personal quest. =)


Thanks for the review!

[Jesus Sanchez-Palencia]

Created attachment 113331 [details]
Patch

[Jesus Sanchez-Palencia]

Created attachment 113350 [details]
Patch

[Jesus Sanchez-Palencia]

Created attachment 113352 [details]
Patch

[Simon Hausmann]

Comment on attachment 113352 [details]
Patch

r=me

Todo for the future: Move this into the "experimental" section of the new WebView API - as discussed face-to-face.

[WebKit Review Bot]

Comment on attachment 113352 [details]
Patch

Rejecting attachment 113352 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
urce/WebKit2/WebProcess/Downloads/qt/DownloadQt.cpp
patching file Source/WebKit2/WebProcess/Downloads/qt/QtFileDownloader.cpp
patching file Source/WebKit2/WebProcess/Downloads/qt/QtFileDownloader.h
patching file Source/WebKit2/WebProcess/WebProcess.cpp
patching file Source/WebKit2/WebProcess/WebProcess.h
patching file Source/WebKit2/WebProcess/WebProcess.messages.in

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Simon Hausmann', u'--f..." exit_code: 1

Full output: http://queues.webkit.org/results/10190369

[Jesus Sanchez-Palencia]

Created attachment 113460 [details]
Patch for landing

[Jesus Sanchez-Palencia]

Comment on attachment 113460 [details]
Patch for landing

webkit-patch land-safely fooled me and didn't update the reviewer name =/. Uploading a new one soon.

[Jesus Sanchez-Palencia]

Created attachment 113462 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 113462 [details]
Patch for landing

Rejecting attachment 113462 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

NOBODY (OOPS!) found in /mnt/git/webkit-commit-queue/Source/WebKit/qt/ChangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/qt/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/10257549

[Jesus Sanchez-Palencia]

Created attachment 113463 [details]
Patch for landing

[Jesus Sanchez-Palencia]

Created attachment 113464 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 113464 [details]
Patch for landing

Clearing flags on attachment: 113464

Committed r99178: <http://trac.webkit.org/changeset/99178>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.