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 <allan.jensen@qt.io>

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<FaviconInfo> &candidates)
 
     const QList<FaviconInfo> &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<FaviconInfo> &candidates)
 
 void FaviconManager::updateCandidates(const QList<FaviconInfo> &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<FaviconInfo> &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<FaviconInfo> &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(
+                        "<html>" +
+                        "<link rel='icon' type='image/png' href='" + icon + "'/>" +
+                        "<link rel='apple-touch-icon' type='image/png' href='" + icon + "'/>" +
+                        "</html>"
+            );
+            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(
+                        "<html>" +
+                        "<link rel='apple-touch-icon' type='image/png' href='" + icon + "'/>" +
+                        "</html>"
+            );
+            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("<html>"
+                    "<link rel='icon' type='image/png' href='" + icon + "'/>"
+                    "<link rel='apple-touch-icon' type='image/png' href='" + icon + "'/>"
+                    "</html>");
+    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("<html>"
+                    "<link rel='apple-touch-icon' type='image/png' href='" + icon + "'/>"
+                    "</html>");
+    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"