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..caf8332e86ed890945bb86a4f0216b1a04f6ef2c 100644 |
--- a/chrome/browser/favicon/content_favicon_driver_browsertest.cc |
+++ b/chrome/browser/favicon/content_favicon_driver_browsertest.cc |
@@ -10,9 +10,11 @@ |
#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" |
@@ -20,6 +22,7 @@ |
#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" |
@@ -129,7 +132,11 @@ class PendingTaskWaiter : public content::NotificationObserver, |
if (type == content::NOTIFICATION_LOAD_STOP) |
load_stopped_ = true; |
- OnNotification(); |
+ // We need to verify completion periodically because not all paths result |
+ // in a notification that can be reacted to immediately. For example, if |
+ // the history database lookup success after candidates have been fed into |
+ // FaviconHandler. |
pkotwicz
2017/05/17 05:20:34
I find this comment confusing
How about: "We need
mastiz
2017/05/17 05:53:02
Done.
|
+ CallOnNotificationPeriodically(); |
pkotwicz
2017/05/17 05:20:34
Maybe rename this function CheckStopWaitingPeriodi
mastiz
2017/05/17 05:53:02
Done.
|
} |
// favicon::Favicon |
@@ -152,6 +159,15 @@ class PendingTaskWaiter : public content::NotificationObserver, |
} |
} |
+ void CallOnNotificationPeriodically() { |
+ OnNotification(); |
pkotwicz
2017/05/17 05:20:34
Can we call EndLoopIfCanStopWaiting() directly?
mastiz
2017/05/17 05:53:03
Done.
|
+ base::ThreadTaskRunnerHandle::Get()->PostDelayedTask( |
+ FROM_HERE, |
+ base::BindOnce(&PendingTaskWaiter::CallOnNotificationPeriodically, |
+ weak_factory_.GetWeakPtr()), |
+ base::TimeDelta::FromSeconds(1)); |
+ } |
+ |
void EndLoopIfCanStopWaiting() { |
if (!quit_closure_.is_null() && |
load_stopped_ && |
@@ -188,6 +204,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 +283,70 @@ 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. |
pkotwicz
2017/05/17 05:20:34
This comment does not really match up
mastiz
2017/05/17 05:53:02
Done.
|
+ std::unique_ptr<TestResourceDispatcherHostDelegate> delegate( |
+ new TestResourceDispatcherHostDelegate(icon_url)); |
+ content::ResourceDispatcherHost::Get()->SetDelegate(delegate.get()); |
+ |
+ // 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_TRUE(delegate->was_requested()); |
+ delegate->Reset(); |
pkotwicz
2017/05/17 05:20:34
Do you need |delegate| at all for this test?
mastiz
2017/05/17 05:53:02
Done.
|
+ 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); |
+} |