Fix in-document navigations breaking icons from Web Manifests

chrome/browser/favicon/content_favicon_driver_browsertest.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/favicon/content/content_favicon_driver.h"
 
 #include "base/location.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/run_loop.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_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"
 #include "url/url_constants.h"
 
 namespace {
 
 // Tracks whether the URL passed to the constructor is requested and whether
 // the request bypasses the cache.
 class TestResourceDispatcherHostDelegate
@@ skipping 37 matching lines @@
   }
 
  private:
   GURL url_;
   bool was_requested_;
   bool bypassed_cache_;
 
   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()->GetLastCommittedURL()) {
+      return;
+    }
 
     // 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();
   }
 
   void CheckStopWaitingPeriodically() {
     EndLoopIfCanStopWaiting();
     base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
         FROM_HERE,
         base::BindOnce(&PendingTaskWaiter::CheckStopWaitingPeriodically,
                        weak_factory_.GetWeakPtr()),
         base::TimeDelta::FromSeconds(1));
   }
 
   void EndLoopIfCanStopWaiting() {
     if (!quit_closure_.is_null() &&
-        load_stopped_ &&
-        !checker_->HasPendingTasks()) {
+        !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);
 };
 
 }  // namespace
 
-class ContentFaviconDriverTest : public InProcessBrowserTest,
-                                 public FaviconDriverPendingTaskChecker {
+class ContentFaviconDriverTest : public InProcessBrowserTest {
  public:
   ContentFaviconDriverTest() {}
   ~ContentFaviconDriverTest() override {}
 
   content::WebContents* web_contents() {
     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);
 
     std::vector<favicon_base::FaviconRawBitmapResult> results;
     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,
                const std::vector<favicon_base::FaviconRawBitmapResult>&
                    results) {
               *save_results = results;
               loop->Quit();
             },
             &results, &loop),
         &tracker);
@@ skipping 16 matching lines @@
   ASSERT_TRUE(embedded_test_server()->Start());
   GURL url = embedded_test_server()->GetURL("/favicon/page_with_favicon.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());
 
   // 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);
     waiter.Wait();
   }
   ASSERT_TRUE(delegate->was_requested());
   EXPECT_FALSE(delegate->bypassed_cache());
   delegate->Reset();
 
   ui_test_utils::NavigateToURL(browser(), GURL(url::kAboutBlankURL));
 
   // 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);
     waiter.Wait();
   }
   EXPECT_FALSE(delegate->bypassed_cache());
   delegate->Reset();
 
   // 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();
   }
   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) {
   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);
+  PendingTaskWaiter waiter(web_contents());
   ui_test_utils::NavigateToURLWithDisposition(
       browser(), url, WindowOpenDisposition::CURRENT_TAB,
       ui_test_utils::BROWSER_TEST_NONE);
   waiter.Wait();
 
 #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
 }
 
 // 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 disabled, to populate the cache.
   {
     base::test::ScopedFeatureList override_features;
     override_features.InitAndDisableFeature(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();
   }
-  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));
 
   // Visit the page again now that the feature is enabled (default).
   {
-    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)
+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