Index: chrome/browser/favicon/content_favicon_driver_browsertest.cc |
diff --git a/chrome/browser/favicon/content_favicon_driver_browsertest.cc b/chrome/browser/favicon/content_favicon_driver_browsertest.cc |
index d1d15fd425dfd44a0a9c1add91ad39e8ff94a197..95b62dbb506dfa163f670368f2582a03aace3714 100644 |
--- a/chrome/browser/favicon/content_favicon_driver_browsertest.cc |
+++ b/chrome/browser/favicon/content_favicon_driver_browsertest.cc |
@@ -8,18 +8,19 @@ |
#include "base/macros.h" |
#include "base/memory/weak_ptr.h" |
#include "base/run_loop.h" |
-#include "base/scoped_observer.h" |
#include "base/single_thread_task_runner.h" |
+#include "base/test/scoped_feature_list.h" |
#include "base/threading/thread_task_runner_handle.h" |
#include "chrome/app/chrome_command_ids.h" |
#include "chrome/browser/chrome_notification_types.h" |
+#include "chrome/browser/favicon/favicon_service_factory.h" |
#include "chrome/browser/ui/browser.h" |
#include "chrome/browser/ui/browser_commands.h" |
#include "chrome/browser/ui/tabs/tab_strip_model.h" |
#include "chrome/test/base/in_process_browser_test.h" |
#include "chrome/test/base/ui_test_utils.h" |
-#include "components/favicon/core/favicon_driver_observer.h" |
#include "components/favicon/core/favicon_handler.h" |
+#include "components/favicon/core/favicon_service.h" |
#include "content/public/browser/notification_observer.h" |
#include "content/public/browser/notification_registrar.h" |
#include "content/public/browser/notification_types.h" |
@@ -95,20 +96,16 @@ class FaviconDriverPendingTaskChecker { |
// - The pending navigation. |
// - FaviconHandler's pending favicon database requests. |
// - FaviconHandler's pending downloads. |
-class PendingTaskWaiter : public content::NotificationObserver, |
- public favicon::FaviconDriverObserver { |
+class PendingTaskWaiter : public content::NotificationObserver { |
public: |
PendingTaskWaiter(content::WebContents* web_contents, |
FaviconDriverPendingTaskChecker* checker) |
: checker_(checker), |
load_stopped_(false), |
- scoped_observer_(this), |
weak_factory_(this) { |
registrar_.Add(this, content::NOTIFICATION_LOAD_STOP, |
content::Source<content::NavigationController>( |
&web_contents->GetController())); |
- scoped_observer_.Add( |
- favicon::ContentFaviconDriver::FromWebContents(web_contents)); |
} |
~PendingTaskWaiter() override {} |
@@ -129,27 +126,21 @@ class PendingTaskWaiter : public content::NotificationObserver, |
if (type == content::NOTIFICATION_LOAD_STOP) |
load_stopped_ = true; |
- OnNotification(); |
+ // We need to poll periodically because Delegate::OnFaviconUpdated() is not |
+ // guaranteed to be called upon completion of the last database request / |
+ // download. In particular, OnFaviconUpdated() might not be called if a |
+ // database request confirms the data sent in the previous |
+ // OnFaviconUpdated() call. |
+ CheckStopWaitingPeriodically(); |
} |
- // favicon::Favicon |
- void OnFaviconUpdated(favicon::FaviconDriver* favicon_driver, |
- NotificationIconType notification_icon_type, |
- const GURL& icon_url, |
- bool icon_url_changed, |
- const gfx::Image& image) override { |
- OnNotification(); |
- } |
- |
- void OnNotification() { |
- if (!quit_closure_.is_null()) { |
- // We stop waiting based on changes in state to FaviconHandler which occur |
- // immediately after OnFaviconUpdated() is called. Post a task to check if |
- // we can stop waiting. |
- base::ThreadTaskRunnerHandle::Get()->PostTask( |
- FROM_HERE, base::BindOnce(&PendingTaskWaiter::EndLoopIfCanStopWaiting, |
- weak_factory_.GetWeakPtr())); |
- } |
+ void CheckStopWaitingPeriodically() { |
+ EndLoopIfCanStopWaiting(); |
+ base::ThreadTaskRunnerHandle::Get()->PostDelayedTask( |
+ FROM_HERE, |
+ base::BindOnce(&PendingTaskWaiter::CheckStopWaitingPeriodically, |
+ weak_factory_.GetWeakPtr()), |
+ base::TimeDelta::FromSeconds(1)); |
} |
void EndLoopIfCanStopWaiting() { |
@@ -164,7 +155,6 @@ class PendingTaskWaiter : public content::NotificationObserver, |
bool load_stopped_; |
base::Closure quit_closure_; |
content::NotificationRegistrar registrar_; |
- ScopedObserver<favicon::FaviconDriver, PendingTaskWaiter> scoped_observer_; |
base::WeakPtrFactory<PendingTaskWaiter> weak_factory_; |
DISALLOW_COPY_AND_ASSIGN(PendingTaskWaiter); |
@@ -188,6 +178,34 @@ class ContentFaviconDriverTest : public InProcessBrowserTest, |
->HasPendingTasksForTest(); |
} |
+ favicon_base::FaviconRawBitmapResult GetFaviconForPageURL(const GURL& url) { |
+ favicon::FaviconService* favicon_service = |
+ FaviconServiceFactory::GetForProfile( |
+ browser()->profile(), ServiceAccessType::EXPLICIT_ACCESS); |
+ |
+ std::vector<favicon_base::FaviconRawBitmapResult> results; |
+ base::CancelableTaskTracker tracker; |
+ base::RunLoop loop; |
+ favicon_service->GetFaviconForPageURL( |
+ url, favicon_base::FAVICON, /*desired_size_in_dip=*/0, |
+ base::Bind( |
+ [](std::vector<favicon_base::FaviconRawBitmapResult>* save_results, |
+ base::RunLoop* loop, |
+ const std::vector<favicon_base::FaviconRawBitmapResult>& |
+ results) { |
+ *save_results = results; |
+ loop->Quit(); |
+ }, |
+ &results, &loop), |
+ &tracker); |
+ loop.Run(); |
+ for (const favicon_base::FaviconRawBitmapResult& result : results) { |
+ if (result.is_valid()) |
+ return result; |
+ } |
+ return favicon_base::FaviconRawBitmapResult(); |
+ } |
+ |
private: |
DISALLOW_COPY_AND_ASSIGN(ContentFaviconDriverTest); |
}; |
@@ -239,3 +257,63 @@ IN_PROC_BROWSER_TEST_F(ContentFaviconDriverTest, ReloadBypassingCache) { |
ASSERT_TRUE(delegate->was_requested()); |
EXPECT_TRUE(delegate->bypassed_cache()); |
} |
+ |
+// Test that loading a page that contains icons only in the Web Manifest causes |
+// those icons to be used. |
+IN_PROC_BROWSER_TEST_F(ContentFaviconDriverTest, LoadIconFromWebManifest) { |
+ base::test::ScopedFeatureList override_features; |
+ override_features.InitAndEnableFeature(favicon::kFaviconsFromWebManifest); |
+ |
+ ASSERT_TRUE(embedded_test_server()->Start()); |
+ GURL url = embedded_test_server()->GetURL("/favicon/page_with_manifest.html"); |
+ GURL icon_url = embedded_test_server()->GetURL("/favicon/icon.png"); |
+ |
+ std::unique_ptr<TestResourceDispatcherHostDelegate> delegate( |
+ new TestResourceDispatcherHostDelegate(icon_url)); |
+ content::ResourceDispatcherHost::Get()->SetDelegate(delegate.get()); |
+ |
+ PendingTaskWaiter waiter(web_contents(), this); |
+ ui_test_utils::NavigateToURLWithDisposition( |
+ browser(), url, WindowOpenDisposition::CURRENT_TAB, |
+ ui_test_utils::BROWSER_TEST_NONE); |
+ waiter.Wait(); |
+ |
+ EXPECT_TRUE(delegate->was_requested()); |
+} |
+ |
+// Test that loading a page that contains a Web Manifest without icons and a |
+// regular favicon in the HTML reports the icon. The regular icon is initially |
+// cached in the Favicon database. |
+IN_PROC_BROWSER_TEST_F(ContentFaviconDriverTest, |
+ LoadRegularIconDespiteWebManifestWithoutIcons) { |
+ ASSERT_TRUE(embedded_test_server()->Start()); |
+ GURL url = embedded_test_server()->GetURL( |
+ "/favicon/page_with_manifest_without_icons.html"); |
+ GURL icon_url = embedded_test_server()->GetURL("/favicon/icon.png"); |
+ |
+ // Initial visit with the feature still disabled, to populate the cache. |
+ { |
+ PendingTaskWaiter waiter(web_contents(), this); |
+ ui_test_utils::NavigateToURLWithDisposition( |
+ browser(), url, WindowOpenDisposition::CURRENT_TAB, |
+ ui_test_utils::BROWSER_TEST_NONE); |
+ waiter.Wait(); |
+ } |
+ ASSERT_NE(nullptr, GetFaviconForPageURL(url).bitmap_data); |
+ |
+ ui_test_utils::NavigateToURL(browser(), GURL(url::kAboutBlankURL)); |
+ |
+ // Enable the feature and visit the page again. |
+ base::test::ScopedFeatureList override_features; |
+ override_features.InitAndEnableFeature(favicon::kFaviconsFromWebManifest); |
+ |
+ { |
+ PendingTaskWaiter waiter(web_contents(), this); |
+ ui_test_utils::NavigateToURLWithDisposition( |
+ browser(), url, WindowOpenDisposition::CURRENT_TAB, |
+ ui_test_utils::BROWSER_TEST_NONE); |
+ waiter.Wait(); |
+ } |
+ |
+ EXPECT_NE(nullptr, GetFaviconForPageURL(url).bitmap_data); |
+} |