From 837347e1901086a9757b5aca7e5bbee7b0d8e20e Mon Sep 17 00:00:00 2001 From: Peter Varga Date: Mon, 11 Feb 2019 16:38:47 +0100 Subject: Fix handling of touch icons when it is disabled Store the icon type in a bitfield because the same icon URL might be used for various types on same page. This way webengine won't ignore to download a default icon what is also set as a touch icon when touch icons are disabled. Moreover, do not store the icon types from the previous page because a subsequent page might use the same icon URL but with different type. With this change the type of the cached icons are updated after each page load. Fixes: QTBUG-70081 Change-Id: I8031a740b07b0c6a8e5759a994f386b13ce87be2 Reviewed-by: Allan Sandfeld Jensen --- src/core/favicon_manager.cpp | 20 ++++++++---- src/core/favicon_manager.h | 15 +++++---- src/core/type_conversion.cpp | 2 +- tests/auto/quick/qmltests/data/tst_favicon.qml | 33 +++++++++++++++++++ .../widgets/faviconmanager/tst_faviconmanager.cpp | 38 ++++++++++++++++++++++ 5 files changed, 94 insertions(+), 14 deletions(-) diff --git a/src/core/favicon_manager.cpp b/src/core/favicon_manager.cpp index de6a0f183..f7ba858c1 100644 --- a/src/core/favicon_manager.cpp +++ b/src/core/favicon_manager.cpp @@ -253,7 +253,7 @@ void FaviconManager::update(const QList &candidates) const QList &faviconInfoList = getFaviconInfoList(true /* candidates only */); for (auto it = faviconInfoList.cbegin(), end = faviconInfoList.cend(); it != end; ++it) { - if (!touchIconsEnabled && it->type != FaviconInfo::Favicon) + if (!touchIconsEnabled && !(it->type & FaviconInfo::Favicon)) continue; if (it->isValid()) @@ -272,6 +272,14 @@ void FaviconManager::update(const QList &candidates) void FaviconManager::updateCandidates(const QList &candidates) { + // Invalidate types of the already stored candidate icons because it might differ + // among pages. + for (FaviconInfo candidateFaviconInfo : candidates) { + const QUrl &candidateUrl = candidateFaviconInfo.url; + if (m_faviconInfoMap.contains(candidateUrl)) + m_faviconInfoMap[candidateUrl].type = FaviconInfo::InvalidIcon; + } + m_candidateCount = candidates.count(); for (FaviconInfo candidateFaviconInfo : candidates) { const QUrl &candidateUrl = candidateFaviconInfo.url; @@ -279,8 +287,8 @@ void FaviconManager::updateCandidates(const QList &candidates) if (!m_faviconInfoMap.contains(candidateUrl)) m_faviconInfoMap.insert(candidateUrl, candidateFaviconInfo); else { - // The same icon can be used for more than one page with different types. - m_faviconInfoMap[candidateUrl].type = candidateFaviconInfo.type; + // The same icon URL can be used for different types. + m_faviconInfoMap[candidateUrl].type |= candidateFaviconInfo.type; } m_faviconInfoMap[candidateUrl].candidate = true; @@ -311,7 +319,7 @@ QUrl FaviconManager::candidateIconUrl(bool touchIconsEnabled) const unsigned bestArea = 0; for (auto it = faviconInfoList.cbegin(), end = faviconInfoList.cend(); it != end; ++it) { - if (!touchIconsEnabled && it->type != FaviconInfo::Favicon) + if (!touchIconsEnabled && !(it->type & FaviconInfo::Favicon)) continue; if (it->isValid() && bestArea < area(it->size)) { @@ -331,7 +339,7 @@ void FaviconManager::generateCandidateIcon(bool touchIconsEnabled) const QList &faviconInfoList = getFaviconInfoList(true /* candidates only */); for (auto it = faviconInfoList.cbegin(), end = faviconInfoList.cend(); it != end; ++it) { - if (!touchIconsEnabled && it->type != FaviconInfo::Favicon) + if (!touchIconsEnabled && !(it->type & FaviconInfo::Favicon)) continue; if (!it->isValid() || !it->isDownloaded()) @@ -373,7 +381,7 @@ FaviconInfo::FaviconInfo(const FaviconInfo &other) { } -FaviconInfo::FaviconInfo(const QUrl &url, FaviconInfo::FaviconType type) +FaviconInfo::FaviconInfo(const QUrl &url, FaviconInfo::FaviconTypeFlags type) : url(url) , type(type) , size(QSize(0, 0)) diff --git a/src/core/favicon_manager.h b/src/core/favicon_manager.h index f9758d0f0..60d194c4b 100644 --- a/src/core/favicon_manager.h +++ b/src/core/favicon_manager.h @@ -84,23 +84,24 @@ class WebContentsAdapterClient; // Based on src/3rdparty/chromium/content/public/common/favicon_url.h class QWEBENGINECORE_PRIVATE_EXPORT FaviconInfo { public: - enum FaviconType { - InvalidIcon, - Favicon, - TouchIcon, - TouchPrecomposedIcon + enum FaviconTypeFlag { + InvalidIcon = 0, + Favicon = 1 << 0, + TouchIcon = 1 << 1, + TouchPrecomposedIcon = 1 << 2 }; + Q_DECLARE_FLAGS(FaviconTypeFlags, FaviconTypeFlag); FaviconInfo(); FaviconInfo(const FaviconInfo &); - FaviconInfo(const QUrl &, FaviconInfo::FaviconType); + FaviconInfo(const QUrl &, FaviconInfo::FaviconTypeFlags); ~FaviconInfo(); bool isValid() const; bool isDownloaded() const; QUrl url; - FaviconType type; + FaviconTypeFlags type; // Stores the largest size in case of multi-size icon QSize size; bool candidate; diff --git a/src/core/type_conversion.cpp b/src/core/type_conversion.cpp index d35426ac3..f69b2a297 100644 --- a/src/core/type_conversion.cpp +++ b/src/core/type_conversion.cpp @@ -216,7 +216,7 @@ int flagsFromModifiers(Qt::KeyboardModifiers modifiers) return modifierFlags; } -FaviconInfo::FaviconType toQt(content::FaviconURL::IconType type) +FaviconInfo::FaviconTypeFlags toQt(content::FaviconURL::IconType type) { switch (type) { case content::FaviconURL::IconType::kFavicon: diff --git a/tests/auto/quick/qmltests/data/tst_favicon.qml b/tests/auto/quick/qmltests/data/tst_favicon.qml index 563a87c83..50a412384 100644 --- a/tests/auto/quick/qmltests/data/tst_favicon.qml +++ b/tests/auto/quick/qmltests/data/tst_favicon.qml @@ -375,5 +375,38 @@ TestWebEngineView { faviconImage.destroy() } + + function test_touchIconWithSameURL() + { + WebEngine.settings.touchIconsEnabled = false; + + var icon = "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII="; + + webEngineView.loadHtml( + "" + + "" + + "" + + "" + ); + verify(webEngineView.waitForLoadSucceeded()); + + // The default favicon has to be loaded even if its URL is also set as a touch icon while touch icons are disabled. + tryCompare(iconChangedSpy, "count", 1); + compare(webEngineView.icon.toString().replace(/^image:\/\/favicon\//, ''), icon); + + iconChangedSpy.clear(); + + webEngineView.loadHtml( + "" + + "" + + "" + ); + verify(webEngineView.waitForLoadSucceeded()); + + // This page only has a touch icon. With disabled touch icons we don't expect any icon to be shown even if the same icon + // was loaded previously. + tryCompare(iconChangedSpy, "count", 1); + verify(!webEngineView.icon.toString().replace(/^image:\/\/favicon\//, '')); + } } } diff --git a/tests/auto/widgets/faviconmanager/tst_faviconmanager.cpp b/tests/auto/widgets/faviconmanager/tst_faviconmanager.cpp index 606d05d9e..540c8d505 100644 --- a/tests/auto/widgets/faviconmanager/tst_faviconmanager.cpp +++ b/tests/auto/widgets/faviconmanager/tst_faviconmanager.cpp @@ -62,6 +62,7 @@ private Q_SLOTS: void downloadTouchIconsEnabled_data(); void downloadTouchIconsEnabled(); void dynamicFavicon(); + void touchIconWithSameURL(); private: QWebEngineView *m_view; @@ -508,6 +509,43 @@ void tst_FaviconManager::dynamicFavicon() } } +void tst_FaviconManager::touchIconWithSameURL() +{ + m_page->settings()->setAttribute(QWebEngineSettings::TouchIconsEnabled, false); + + const QString icon("data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII="); + QSignalSpy loadFinishedSpy(m_page, SIGNAL(loadFinished(bool))); + QSignalSpy iconUrlChangedSpy(m_page, SIGNAL(iconUrlChanged(QUrl))); + QSignalSpy iconChangedSpy(m_page, SIGNAL(iconChanged(QIcon))); + + m_page->setHtml("" + "" + "" + ""); + QTRY_COMPARE(loadFinishedSpy.count(), 1); + + // The default favicon has to be loaded even if its URL is also set as a touch icon while touch icons are disabled. + QTRY_COMPARE(iconUrlChangedSpy.count(), 1); + QCOMPARE(m_page->iconUrl().toString(), icon); + QTRY_COMPARE(iconChangedSpy.count(), 1); + + loadFinishedSpy.clear(); + iconUrlChangedSpy.clear(); + iconChangedSpy.clear(); + + m_page->setHtml("" + "" + ""); + QTRY_COMPARE(loadFinishedSpy.count(), 1); + + // This page only has a touch icon. With disabled touch icons we don't expect any icon to be shown even if the same icon + // was loaded previously. + QTRY_COMPARE(iconUrlChangedSpy.count(), 1); + QVERIFY(m_page->iconUrl().toString().isEmpty()); + QTRY_COMPARE(iconChangedSpy.count(), 1); + +} + QTEST_MAIN(tst_FaviconManager) #include "tst_faviconmanager.moc" -- cgit v1.2.3