From c24cc3014d750a406523629eff94f4f5f87e92cb Mon Sep 17 00:00:00 2001 From: Kirill Burtsev Date: Thu, 28 Nov 2019 19:10:23 +0100 Subject: Fix 'setDownloadDirectory' for download item on 'SavePage' action MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Chromium's DownloadManager doesn't create its download items before path for saving page is confirmed. So assert inside updateDownloadPath was not correct. Moreover, the name is confusing because it's not really updating anything. Remove it and use ProfileAdapterClient::DownloadInfo timestamp to determine updated filename after directory change. Ammends recent new api for changing download directory 0884fab3b1. Fixes: QTBUG-80372 Change-Id: If9efb52979deb3cf21fc4e12989173c85e04e090 Reviewed-by: Jüri Valdmann --- src/core/download_manager_delegate_qt.cpp | 2 +- src/core/profile_adapter.cpp | 7 ----- src/core/profile_adapter.h | 1 - src/core/profile_adapter_client.h | 2 +- src/webengine/api/qquickwebenginedownloaditem.cpp | 6 ++--- .../api/qquickwebenginedownloaditem_p_p.h | 1 + src/webengine/api/qquickwebengineprofile.cpp | 1 + .../api/qwebenginedownloaditem.cpp | 6 ++--- .../api/qwebenginedownloaditem_p.h | 1 + src/webenginewidgets/api/qwebengineprofile.cpp | 1 + .../tst_qwebenginedownloaditem.cpp | 31 ++++++++++++++++------ 11 files changed, 35 insertions(+), 24 deletions(-) diff --git a/src/core/download_manager_delegate_qt.cpp b/src/core/download_manager_delegate_qt.cpp index 34e290317..7049b8273 100644 --- a/src/core/download_manager_delegate_qt.cpp +++ b/src/core/download_manager_delegate_qt.cpp @@ -298,7 +298,7 @@ void DownloadManagerDelegateQt::ChooseSavePath(content::WebContents *web_content ProfileAdapterClient::NoReason, adapterClient, QFileInfo(suggestedFilePath).fileName(), - time_t(QDateTime::currentMSecsSinceEpoch()) + QDateTime::currentMSecsSinceEpoch() }; for (ProfileAdapterClient *client : qAsConst(clients)) { diff --git a/src/core/profile_adapter.cpp b/src/core/profile_adapter.cpp index d421edf00..bccdf1ada 100644 --- a/src/core/profile_adapter.cpp +++ b/src/core/profile_adapter.cpp @@ -715,13 +715,6 @@ QString ProfileAdapter::determineDownloadPath(const QString &downloadDirectory, return suggestedFilePath; } -QString ProfileAdapter::updateDownloadPath(int downloadId, const QString &directory, const QString &fileName) -{ - download::DownloadItem *download = m_downloadManagerDelegate->findDownloadById(downloadId); - Q_ASSERT(download); - return determineDownloadPath(directory, fileName, download->GetStartTime().ToTimeT()); -} - #if QT_CONFIG(ssl) QWebEngineClientCertificateStore *ProfileAdapter::clientCertificateStore() { diff --git a/src/core/profile_adapter.h b/src/core/profile_adapter.h index 01477d0d9..1b89a8004 100644 --- a/src/core/profile_adapter.h +++ b/src/core/profile_adapter.h @@ -214,7 +214,6 @@ public: { return m_persistentNotifications; } QString determineDownloadPath(const QString &downloadDirectory, const QString &suggestedFilename, const time_t &startTime); - QString updateDownloadPath(int downloadId, const QString &directory, const QString &filename); private: void updateCustomUrlSchemeHandlers(); diff --git a/src/core/profile_adapter_client.h b/src/core/profile_adapter_client.h index dc0f508a1..394f92270 100644 --- a/src/core/profile_adapter_client.h +++ b/src/core/profile_adapter_client.h @@ -140,7 +140,7 @@ public: int downloadInterruptReason; WebContentsAdapterClient *page; QString suggestedFileName; - time_t startTime; + qint64 startTime; }; virtual ~ProfileAdapterClient() { } diff --git a/src/webengine/api/qquickwebenginedownloaditem.cpp b/src/webengine/api/qquickwebenginedownloaditem.cpp index 878dddadb..6abd89910 100644 --- a/src/webengine/api/qquickwebenginedownloaditem.cpp +++ b/src/webengine/api/qquickwebenginedownloaditem.cpp @@ -521,9 +521,9 @@ void QQuickWebEngineDownloadItem::setDownloadDirectory(const QString &directory) Q_EMIT downloadDirectoryChanged(); } - QString newFileName = QFileInfo(d->profile->d_ptr->profileAdapter()->updateDownloadPath(d->downloadId, - d->downloadDirectory, - d->suggestedFileName)).fileName(); + QString newFileName = QFileInfo(d->profile->d_ptr->profileAdapter()->determineDownloadPath(d->downloadDirectory, + d->suggestedFileName, + d->startTime)).fileName(); if (d->downloadFileName != newFileName) { d->downloadFileName = newFileName; Q_EMIT pathChanged(); diff --git a/src/webengine/api/qquickwebenginedownloaditem_p_p.h b/src/webengine/api/qquickwebenginedownloaditem_p_p.h index 51deee18b..1be6434ec 100644 --- a/src/webengine/api/qquickwebenginedownloaditem_p_p.h +++ b/src/webengine/api/qquickwebenginedownloaditem_p_p.h @@ -71,6 +71,7 @@ public: ~QQuickWebEngineDownloadItemPrivate(); quint32 downloadId; + qint64 startTime; QQuickWebEngineDownloadItem::DownloadState downloadState; QQuickWebEngineDownloadItem::SavePageFormat savePageFormat; QQuickWebEngineDownloadItem::DownloadType type; diff --git a/src/webengine/api/qquickwebengineprofile.cpp b/src/webengine/api/qquickwebengineprofile.cpp index edad7ec44..834bb6a05 100644 --- a/src/webengine/api/qquickwebengineprofile.cpp +++ b/src/webengine/api/qquickwebengineprofile.cpp @@ -245,6 +245,7 @@ void QQuickWebEngineProfilePrivate::downloadRequested(DownloadItemInfo &info) QQuickWebEngineDownloadItemPrivate *itemPrivate = new QQuickWebEngineDownloadItemPrivate(q, info.url); itemPrivate->downloadId = info.id; itemPrivate->downloadState = QQuickWebEngineDownloadItem::DownloadRequested; + itemPrivate->startTime = info.startTime; itemPrivate->totalBytes = info.totalBytes; itemPrivate->mimeType = info.mimeType; itemPrivate->downloadDirectory = QFileInfo(info.path).path(); diff --git a/src/webenginewidgets/api/qwebenginedownloaditem.cpp b/src/webenginewidgets/api/qwebenginedownloaditem.cpp index 724249208..fd7d90704 100644 --- a/src/webenginewidgets/api/qwebenginedownloaditem.cpp +++ b/src/webenginewidgets/api/qwebenginedownloaditem.cpp @@ -599,9 +599,9 @@ void QWebEngineDownloadItem::setDownloadDirectory(const QString &directory) if (!directory.isEmpty() && d->downloadDirectory != directory) d->downloadDirectory = directory; - d->downloadFileName = QFileInfo(d->profile->profileAdapter()->updateDownloadPath(d->downloadId, - d->downloadDirectory, - d->suggestedFileName)).fileName(); + d->downloadFileName = QFileInfo(d->profile->profileAdapter()->determineDownloadPath(d->downloadDirectory, + d->suggestedFileName, + d->startTime)).fileName(); } /*! diff --git a/src/webenginewidgets/api/qwebenginedownloaditem_p.h b/src/webenginewidgets/api/qwebenginedownloaditem_p.h index 08e478736..034684a00 100644 --- a/src/webenginewidgets/api/qwebenginedownloaditem_p.h +++ b/src/webenginewidgets/api/qwebenginedownloaditem_p.h @@ -70,6 +70,7 @@ public: bool downloadFinished; quint32 downloadId; + qint64 startTime; QWebEngineDownloadItem::DownloadState downloadState; QWebEngineDownloadItem::SavePageFormat savePageFormat; QWebEngineDownloadItem::DownloadType type; diff --git a/src/webenginewidgets/api/qwebengineprofile.cpp b/src/webenginewidgets/api/qwebengineprofile.cpp index 09f5ce2fd..470babf8f 100644 --- a/src/webenginewidgets/api/qwebengineprofile.cpp +++ b/src/webenginewidgets/api/qwebengineprofile.cpp @@ -227,6 +227,7 @@ void QWebEngineProfilePrivate::downloadRequested(DownloadItemInfo &info) itemPrivate->downloadId = info.id; itemPrivate->downloadState = info.accepted ? QWebEngineDownloadItem::DownloadInProgress : QWebEngineDownloadItem::DownloadRequested; + itemPrivate->startTime = info.startTime; itemPrivate->downloadDirectory = QFileInfo(info.path).path(); itemPrivate->downloadFileName = QFileInfo(info.path).fileName(); itemPrivate->suggestedFileName = info.suggestedFileName; diff --git a/tests/auto/widgets/qwebenginedownloaditem/tst_qwebenginedownloaditem.cpp b/tests/auto/widgets/qwebenginedownloaditem/tst_qwebenginedownloaditem.cpp index d34e3cefe..bbcef2226 100644 --- a/tests/auto/widgets/qwebenginedownloaditem/tst_qwebenginedownloaditem.cpp +++ b/tests/auto/widgets/qwebenginedownloaditem/tst_qwebenginedownloaditem.cpp @@ -622,14 +622,17 @@ void tst_QWebEngineDownloadItem::downloadTwoLinks() void tst_QWebEngineDownloadItem::downloadPage_data() { + QTest::addColumn("saveWithPageAction"); QTest::addColumn("savePageFormat"); - QTest::newRow("SingleHtmlSaveFormat") << QWebEngineDownloadItem::SingleHtmlSaveFormat; - QTest::newRow("CompleteHtmlSaveFormat") << QWebEngineDownloadItem::CompleteHtmlSaveFormat; - QTest::newRow("MimeHtmlSaveFormat") << QWebEngineDownloadItem::MimeHtmlSaveFormat; + QTest::newRow("SingleHtmlSaveFormat") << false << QWebEngineDownloadItem::SingleHtmlSaveFormat; + QTest::newRow("CompleteHtmlSaveFormat") << false << QWebEngineDownloadItem::CompleteHtmlSaveFormat; + QTest::newRow("MimeHtmlSaveFormat") << false << QWebEngineDownloadItem::MimeHtmlSaveFormat; + QTest::newRow("SavePageAction") << true << QWebEngineDownloadItem::MimeHtmlSaveFormat; } void tst_QWebEngineDownloadItem::downloadPage() { + QFETCH(bool, saveWithPageAction); QFETCH(QWebEngineDownloadItem::SavePageFormat, savePageFormat); // Set up HTTP server @@ -649,12 +652,12 @@ void tst_QWebEngineDownloadItem::downloadPage() // Set up profile and download handler QTemporaryDir tmpDir; QVERIFY(tmpDir.isValid()); - QString downloadPath = tmpDir.path() + QStringLiteral("/test.html"); + QString downloadFileName("test.html"), downloadPath = tmpDir.filePath(downloadFileName); QUrl downloadUrl = m_server->url("/"); int acceptedCount = 0; int finishedCount = 0; ScopedConnection sc2 = connect(m_profile, &QWebEngineProfile::downloadRequested, [&](QWebEngineDownloadItem *item) { - QCOMPARE(item->state(), QWebEngineDownloadItem::DownloadInProgress); + QCOMPARE(item->state(), saveWithPageAction ? QWebEngineDownloadItem::DownloadRequested : QWebEngineDownloadItem::DownloadInProgress); QCOMPARE(item->isFinished(), false); QCOMPARE(item->totalBytes(), -1); QCOMPARE(item->receivedBytes(), 0); @@ -663,11 +666,19 @@ void tst_QWebEngineDownloadItem::downloadPage() QCOMPARE(item->isSavePageDownload(), true); // FIXME(juvaldma): why is mimeType always the same? QCOMPARE(item->mimeType(), QStringLiteral("application/x-mimearchive")); - QCOMPARE(QDir(item->downloadDirectory()).filePath(item->downloadFileName()), downloadPath); QCOMPARE(item->savePageFormat(), savePageFormat); QCOMPARE(item->url(), downloadUrl); QCOMPARE(item->page(), m_page); - // no need to call item->accept() + + if (saveWithPageAction) { + QVERIFY(!item->downloadDirectory().isEmpty()); + QVERIFY(!item->downloadFileName().isEmpty()); + item->setDownloadDirectory(tmpDir.path()); + item->setDownloadFileName(downloadFileName); + item->accept(); + } // save with explicit path accepts download automatically + + QCOMPARE(QDir(item->downloadDirectory()).filePath(item->downloadFileName()), downloadPath); connect(item, &QWebEngineDownloadItem::finished, [&, item]() { QCOMPARE(item->state(), QWebEngineDownloadItem::DownloadCompleted); @@ -697,7 +708,11 @@ void tst_QWebEngineDownloadItem::downloadPage() QCOMPARE(indexRequestCount, 1); // Save some HTML - m_page->save(downloadPath, savePageFormat); + if (saveWithPageAction) + m_page->triggerAction(QWebEnginePage::SavePage); + else + m_page->save(downloadPath, savePageFormat); + QTRY_COMPARE(acceptedCount, 1); QTRY_COMPARE(finishedCount, 1); QFile file(downloadPath); -- cgit v1.2.3