Bug 112155 – [Qt][WK2] Don't require a callback from the application to start a download

• Status: RESOLVED
• Resolution: INVALID
• Component: New Bugs

[Jocelyn Turcotte]

[Qt][WK2] Don't require a callback from the application to start a download

[Jocelyn Turcotte]

Created attachment 192748 [details]
Patch

[Simon Hausmann]

Comment on attachment 192748 [details]
Patch

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

Looks pretty sensible! Less ifdefs again :)

> Source/WebKit2/Shared/Downloads/qt/QtFileDownloader.cpp:175
> +    bool dummy;
> +    String destination = m_download->decideDestinationWithSuggestedFilename(filename, dummy);

What was the dummy here again? bool overwrite?

> Source/WebKit2/UIProcess/API/qt/qwebdownloaditem_p_p.h:36
> +    WKDownloadRef download;

Shouldn't this be a WKRetainPtr<WKDownloadRef> ?

> Source/WebKit2/UIProcess/qt/QtWebPagePolicyClient.cpp:149
> +            QString requestURL = WKURLCopyQString(adoptWK(WKURLRequestCopyURL(request)).get());

adoptToQString?

[Jesus Sanchez-Palencia]

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

LGTM as well.

[Jocelyn Turcotte]

Created attachment 193890 [details]
Patch

Address Simon's comment.

[Benjamin Poulain]

Comment on attachment 193890 [details]
Patch

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

Looks like plenty of good stuff.
Signed off for WebKit2.

> Source/WebKit2/Shared/Downloads/qt/QtFileDownloader.cpp:107
> +    // Queue the call since we might still be in our Download's constructor.

It is unclear from the comment why being in Download's constructor would be a problem when emitting "onFinished".

> Source/WebKit2/Shared/Downloads/qt/QtFileDownloader.cpp:174
> +    bool allowOverwrite; // dummy

I am going to hell for this: WebKit comment style :-D

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1062
> +bool QQuickWebViewExperimental::downloadUnsupportedContent() const
> +{
> +    Q_D(const QQuickWebView);
> +    return d->m_downloadUnsupportedContent;
> +}
> +
> +void QQuickWebViewExperimental::setDownloadUnsupportedContent(bool enable)
> +{
> +    Q_D(QQuickWebView);
> +    if (d->m_downloadUnsupportedContent != enable) {
> +        d->m_downloadUnsupportedContent = enable;
> +        emit downloadUnsupportedContentChanged();
> +    }
> +}
> +

Just a generic idea: maybe providing a list of mimetypes of interest would be more interesting than a boolean?

It depends on what people use this code for obviously :)

> Source/WebKit2/UIProcess/qt/QtDownloadManager.cpp:125
> +    for (int i = 1; i < 100; ++i) {

Could the limit of 100 be a problem?
Is it covered by a test?

> Source/WebKit2/UIProcess/qt/QtWebPagePolicyClient.cpp:147
> +        // If we can't use (show) it then we might have to download it.

I don't think the comment add anything anymore.

[Jocelyn Turcotte]

Created attachment 194233 [details]
Patch

(In reply to comment #5)
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1062
> > +bool QQuickWebViewExperimental::downloadUnsupportedContent() const
> 
> Just a generic idea: maybe providing a list of mimetypes of interest would be more interesting than a boolean?
> 
> It depends on what people use this code for obviously :)

The current call is to remain similar to QWebPage::forwardUnsupportedContent, but I can carry this idea to the API review.

> > Source/WebKit2/UIProcess/qt/QtDownloadManager.cpp:125
> > +    for (int i = 1; i < 100; ++i) {
> 
> Could the limit of 100 be a problem?
> Is it covered by a test?

Right now it is going to fail the download if all of them are taken.
I've increased it to 10000 to make it very unlikely that a user will face this limitation while keeping the number readable in the file name. The goal is to let the application handle extreme use cases by moving/renaming the file once the download is finished.

There is no test but I'm adding a download UI to MiniBrowser to be able to test the general download code manually at least. Having an automated test to cover the 10000 limit wouldn't be profitable I think (long execution time and potential flackiness).

[Anders Carlsson]

Comment on attachment 194233 [details]
Patch

Qt has been removed, clearing review flags.

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.