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 601e39174478fa44a6c5c7f99ab3e2182ddb5800..408f67e15344ab0b1ecdfb162418ecef24ea06a4 100644 |
--- a/chrome/browser/favicon/content_favicon_driver_browsertest.cc |
+++ b/chrome/browser/favicon/content_favicon_driver_browsertest.cc |
@@ -22,11 +22,9 @@ |
#include "components/favicon/core/favicon_handler.h" |
#include "components/favicon/core/favicon_service.h" |
#include "components/favicon/core/features.h" |
-#include "content/public/browser/notification_observer.h" |
-#include "content/public/browser/notification_registrar.h" |
-#include "content/public/browser/notification_types.h" |
#include "content/public/browser/resource_dispatcher_host.h" |
#include "content/public/browser/resource_dispatcher_host_delegate.h" |
+#include "content/public/browser/web_contents_observer.h" |
#include "net/base/load_flags.h" |
#include "net/test/embedded_test_server/embedded_test_server.h" |
#include "net/url_request/url_request.h" |
@@ -84,48 +82,34 @@ class TestResourceDispatcherHostDelegate |
DISALLOW_COPY_AND_ASSIGN(TestResourceDispatcherHostDelegate); |
}; |
-// Checks whether the FaviconDriver is waiting for a download to complete or |
-// for data from the FaviconService. |
-class FaviconDriverPendingTaskChecker { |
- public: |
- virtual ~FaviconDriverPendingTaskChecker() {} |
- |
- virtual bool HasPendingTasks() = 0; |
-}; |
- |
// Waits for the following the finish: |
// - The pending navigation. |
// - FaviconHandler's pending favicon database requests. |
// - FaviconHandler's pending downloads. |
-class PendingTaskWaiter : public content::NotificationObserver { |
+// - Optionally, for a specific page URL (as a mechanism to wait of Javascript |
+// completion). |
+class PendingTaskWaiter : public content::WebContentsObserver { |
public: |
PendingTaskWaiter(content::WebContents* web_contents, |
- FaviconDriverPendingTaskChecker* checker) |
- : checker_(checker), |
- load_stopped_(false), |
- weak_factory_(this) { |
- registrar_.Add(this, content::NOTIFICATION_LOAD_STOP, |
- content::Source<content::NavigationController>( |
- &web_contents->GetController())); |
- } |
+ const GURL& required_url = GURL()) |
+ : WebContentsObserver(web_contents), |
+ required_url_(required_url), |
+ weak_factory_(this) {} |
~PendingTaskWaiter() override {} |
void Wait() { |
- if (load_stopped_ && !checker_->HasPendingTasks()) |
- return; |
- |
base::RunLoop run_loop; |
quit_closure_ = run_loop.QuitClosure(); |
run_loop.Run(); |
} |
private: |
- // content::NotificationObserver: |
- void Observe(int type, |
- const content::NotificationSource& source, |
- const content::NotificationDetails& details) override { |
- if (type == content::NOTIFICATION_LOAD_STOP) |
- load_stopped_ = true; |
+ // content::WebContentsObserver: |
+ void DidStopLoading() override { |
+ if (!required_url_.is_empty() && |
+ required_url_ != web_contents()->GetVisibleURL()) { |
pkotwicz
2017/06/23 17:52:20
Should this be GetLastCommittedURL() instead?
mastiz
2017/06/27 08:57:36
I think both should be equally fine, and in seems
pkotwicz
2017/06/27 15:21:13
GetLastCommittedURL() is the typical function to u
mastiz
2017/06/28 09:02:13
Thanks for clarifying, I was not aware of that and
|
+ return; |
+ } |
// We need to poll periodically because Delegate::OnFaviconUpdated() is not |
// guaranteed to be called upon completion of the last database request / |
@@ -145,17 +129,15 @@ class PendingTaskWaiter : public content::NotificationObserver { |
} |
void EndLoopIfCanStopWaiting() { |
- if (!quit_closure_.is_null() && |
- load_stopped_ && |
- !checker_->HasPendingTasks()) { |
+ CHECK(!quit_closure_.is_null()); |
pkotwicz
2017/06/23 17:52:20
Why are you making this a CHECK()?
It is possible
mastiz
2017/06/27 08:57:36
Reverted. I however suspect this cannot happen in
pkotwicz
2017/06/27 15:21:13
Aside: ui_test_utils::NavigateToURLWithDisposition
mastiz
2017/06/28 09:02:13
I don't think it is blocking with BROWSER_TEST_NON
|
+ if (!favicon::ContentFaviconDriver::FromWebContents(web_contents()) |
+ ->HasPendingTasksForTest()) { |
quit_closure_.Run(); |
} |
} |
- FaviconDriverPendingTaskChecker* checker_; // Not owned. |
- bool load_stopped_; |
base::Closure quit_closure_; |
- content::NotificationRegistrar registrar_; |
+ const GURL required_url_; |
base::WeakPtrFactory<PendingTaskWaiter> weak_factory_; |
DISALLOW_COPY_AND_ASSIGN(PendingTaskWaiter); |
@@ -163,8 +145,7 @@ class PendingTaskWaiter : public content::NotificationObserver { |
} // namespace |
-class ContentFaviconDriverTest : public InProcessBrowserTest, |
- public FaviconDriverPendingTaskChecker { |
+class ContentFaviconDriverTest : public InProcessBrowserTest { |
public: |
ContentFaviconDriverTest() {} |
~ContentFaviconDriverTest() override {} |
@@ -173,13 +154,9 @@ class ContentFaviconDriverTest : public InProcessBrowserTest, |
return browser()->tab_strip_model()->GetActiveWebContents(); |
} |
- // FaviconDriverPendingTaskChecker: |
- bool HasPendingTasks() override { |
- return favicon::ContentFaviconDriver::FromWebContents(web_contents()) |
- ->HasPendingTasksForTest(); |
- } |
- |
- favicon_base::FaviconRawBitmapResult GetFaviconForPageURL(const GURL& url) { |
+ favicon_base::FaviconRawBitmapResult GetFaviconForPageURL( |
+ const GURL& url, |
+ favicon_base::IconType icon_type) { |
favicon::FaviconService* favicon_service = |
FaviconServiceFactory::GetForProfile( |
browser()->profile(), ServiceAccessType::EXPLICIT_ACCESS); |
@@ -188,7 +165,7 @@ class ContentFaviconDriverTest : public InProcessBrowserTest, |
base::CancelableTaskTracker tracker; |
base::RunLoop loop; |
favicon_service->GetFaviconForPageURL( |
- url, favicon_base::FAVICON, /*desired_size_in_dip=*/0, |
+ url, icon_type, /*desired_size_in_dip=*/0, |
base::Bind( |
[](std::vector<favicon_base::FaviconRawBitmapResult>* save_results, |
base::RunLoop* loop, |
@@ -225,7 +202,7 @@ IN_PROC_BROWSER_TEST_F(ContentFaviconDriverTest, ReloadBypassingCache) { |
// Initial visit in order to populate the cache. |
{ |
- PendingTaskWaiter waiter(web_contents(), this); |
+ PendingTaskWaiter waiter(web_contents()); |
ui_test_utils::NavigateToURLWithDisposition( |
browser(), url, WindowOpenDisposition::CURRENT_TAB, |
ui_test_utils::BROWSER_TEST_NONE); |
@@ -240,7 +217,7 @@ IN_PROC_BROWSER_TEST_F(ContentFaviconDriverTest, ReloadBypassingCache) { |
// A normal visit should fetch the favicon from either the favicon database or |
// the HTTP cache. |
{ |
- PendingTaskWaiter waiter(web_contents(), this); |
+ PendingTaskWaiter waiter(web_contents()); |
ui_test_utils::NavigateToURLWithDisposition( |
browser(), url, WindowOpenDisposition::CURRENT_TAB, |
ui_test_utils::BROWSER_TEST_NONE); |
@@ -251,7 +228,7 @@ IN_PROC_BROWSER_TEST_F(ContentFaviconDriverTest, ReloadBypassingCache) { |
// A reload ignoring the cache should refetch the favicon from the website. |
{ |
- PendingTaskWaiter waiter(web_contents(), this); |
+ PendingTaskWaiter waiter(web_contents()); |
chrome::ExecuteCommand(browser(), IDC_RELOAD_BYPASSING_CACHE); |
waiter.Wait(); |
} |
@@ -273,7 +250,7 @@ IN_PROC_BROWSER_TEST_F(ContentFaviconDriverTest, LoadIconFromWebManifest) { |
new TestResourceDispatcherHostDelegate(icon_url)); |
content::ResourceDispatcherHost::Get()->SetDelegate(delegate.get()); |
- PendingTaskWaiter waiter(web_contents(), this); |
+ PendingTaskWaiter waiter(web_contents()); |
ui_test_utils::NavigateToURLWithDisposition( |
browser(), url, WindowOpenDisposition::CURRENT_TAB, |
ui_test_utils::BROWSER_TEST_NONE); |
@@ -281,6 +258,9 @@ IN_PROC_BROWSER_TEST_F(ContentFaviconDriverTest, LoadIconFromWebManifest) { |
#if defined(OS_ANDROID) |
EXPECT_TRUE(delegate->was_requested()); |
+ EXPECT_NE( |
+ nullptr, |
+ GetFaviconForPageURL(url, favicon_base::WEB_MANIFEST_ICON).bitmap_data); |
#else |
EXPECT_FALSE(delegate->was_requested()); |
#endif |
@@ -298,13 +278,14 @@ IN_PROC_BROWSER_TEST_F(ContentFaviconDriverTest, |
// Initial visit with the feature still disabled, to populate the cache. |
{ |
- PendingTaskWaiter waiter(web_contents(), this); |
+ PendingTaskWaiter waiter(web_contents()); |
ui_test_utils::NavigateToURLWithDisposition( |
browser(), url, WindowOpenDisposition::CURRENT_TAB, |
ui_test_utils::BROWSER_TEST_NONE); |
waiter.Wait(); |
} |
- ASSERT_NE(nullptr, GetFaviconForPageURL(url).bitmap_data); |
+ ASSERT_NE(nullptr, |
+ GetFaviconForPageURL(url, favicon_base::FAVICON).bitmap_data); |
ui_test_utils::NavigateToURL(browser(), GURL(url::kAboutBlankURL)); |
@@ -313,12 +294,37 @@ IN_PROC_BROWSER_TEST_F(ContentFaviconDriverTest, |
override_features.InitAndEnableFeature(favicon::kFaviconsFromWebManifest); |
{ |
- PendingTaskWaiter waiter(web_contents(), this); |
+ PendingTaskWaiter waiter(web_contents()); |
ui_test_utils::NavigateToURLWithDisposition( |
browser(), url, WindowOpenDisposition::CURRENT_TAB, |
ui_test_utils::BROWSER_TEST_NONE); |
waiter.Wait(); |
} |
- EXPECT_NE(nullptr, GetFaviconForPageURL(url).bitmap_data); |
+ EXPECT_NE(nullptr, |
+ GetFaviconForPageURL(url, favicon_base::FAVICON).bitmap_data); |
} |
+ |
+#if defined(OS_ANDROID) || defined(OS_IOS) |
pkotwicz
2017/06/23 17:52:20
Shouldn't this test be Android only given that iOS
mastiz
2017/06/27 08:57:36
Done.
|
+IN_PROC_BROWSER_TEST_F(ContentFaviconDriverTest, |
+ LoadIconFromWebManifestDespitePushState) { |
+ base::test::ScopedFeatureList override_features; |
+ override_features.InitAndEnableFeature(favicon::kFaviconsFromWebManifest); |
+ |
+ ASSERT_TRUE(embedded_test_server()->Start()); |
+ GURL url = |
+ embedded_test_server()->GetURL("/favicon/pushstate_with_manifest.html"); |
+ GURL pushstate_url = embedded_test_server()->GetURL( |
+ "/favicon/pushstate_with_manifest.html#pushState"); |
+ |
+ PendingTaskWaiter waiter(web_contents(), /*required_url=*/pushstate_url); |
+ ui_test_utils::NavigateToURLWithDisposition( |
+ browser(), url, WindowOpenDisposition::CURRENT_TAB, |
+ ui_test_utils::BROWSER_TEST_NONE); |
+ waiter.Wait(); |
+ |
+ EXPECT_NE(nullptr, |
+ GetFaviconForPageURL(pushstate_url, favicon_base::WEB_MANIFEST_ICON) |
+ .bitmap_data); |
+} |
+#endif |