Omnibox Material Refresh: Search Provider Favicons from History DB

This CL hooks up the OmniboxView to the OmniboxClient, which retrieves
the favicon for the default search provider from the FaviconService.

The implementation below is a good start but there needs to be further
work in these two areas:

 a) Our coverage is currently limited to what FaviconService can
    provide us, but that is not adequate. Most search engine favicons
    are never populated into the FaviconService database. See bug
    88243.

 b) We need an in-memory synchronous cache to avoid hits to the sqlite
    Favicons database on every keystroke.

This CL works for search engines with a populated favicon. For example,
visiting search.yahoo.com, and then switching the default search
provider to Yahoo will work.

Bug: 823535
Change-Id: Ie4e4425d9cfa59f971bc3264c1b79104d1e5d958
Reviewed-on: https://chromium-review.googlesource.com/1026695Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553590}

chrome/browser/ui/omnibox/chrome_omnibox_client.cc

@@ -45,6 +45,7 @@
 #include "chrome/common/search/instant_types.h"
 #include "chrome/common/url_constants.h"
 #include "components/favicon/content/content_favicon_driver.h"
+#include "components/favicon/core/favicon_service.h"
 #include "components/feature_engagement/buildflags.h"
 #include "components/omnibox/browser/autocomplete_match.h"
 #include "components/omnibox/browser/autocomplete_result.h"
@@ -337,6 +338,28 @@ gfx::Image ChromeOmniboxClient::GetFaviconForPageUrl(
                                              std::move(on_favicon_fetched));
 }
 
+gfx::Image ChromeOmniboxClient::GetFaviconForDefaultSearchProvider(
+    FaviconFetchedCallback on_favicon_fetched) {
+  const GURL& favicon_url =
+      GetTemplateURLService()->GetDefaultSearchProvider()->favicon_url();
+  if (!favicon_url.is_valid())
+    return gfx::Image();
+
+  auto* favicon_service = FaviconServiceFactory::GetForProfile(
+      profile_, ServiceAccessType::EXPLICIT_ACCESS);
+  pending_default_search_provider_favicon_callback_ =
+      std::move(on_favicon_fetched);
+  favicon_service->GetFaviconImage(
+      favicon_url,
+      base::BindRepeating(
+          &ChromeOmniboxClient::OnDefaultSearchProviderFaviconFetched,
+          weak_factory_.GetWeakPtr()),
+      &default_search_provider_favicon_task_tracker_);
+
+  // TODO(tommycli): Implement a synchronous caching layer to prevent flicker.
+  return gfx::Image();
+}
+
 void ChromeOmniboxClient::OnCurrentMatchChanged(
     const AutocompleteMatch& match) {
   if (!prerender::IsOmniboxEnabled(profile_))
@@ -463,3 +486,13 @@ void ChromeOmniboxClient::OnBitmapFetched(const BitmapFetchedCallback& callback,
   request_id_ = BitmapFetcherService::REQUEST_ID_INVALID;
   callback.Run(bitmap);
 }
+
+void ChromeOmniboxClient::OnDefaultSearchProviderFaviconFetched(
+    const favicon_base::FaviconImageResult& result) {
+  if (pending_default_search_provider_favicon_callback_.is_null() ||
+      result.image.IsEmpty()) {
+    return;
+  }
+  std::move(pending_default_search_provider_favicon_callback_)
+      .Run(result.image);
+}

chrome/browser/ui/omnibox/chrome_omnibox_client.h

@@ -7,10 +7,13 @@
 
 #include "base/compiler_specific.h"
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "base/task/cancelable_task_tracker.h"
 #include "chrome/browser/autocomplete/chrome_autocomplete_scheme_classifier.h"
 #include "chrome/browser/bitmap_fetcher/bitmap_fetcher_service.h"
 #include "chrome/browser/ui/omnibox/favicon_cache.h"
 #include "chrome/common/search/instant_types.h"
+#include "components/favicon_base/favicon_types.h"
 #include "components/omnibox/browser/omnibox_client.h"
 
 class ChromeOmniboxEditController;
@@ -62,6 +65,8 @@ class ChromeOmniboxClient : public OmniboxClient {
   gfx::Image GetFaviconForPageUrl(
       const GURL& page_url,
       FaviconFetchedCallback on_favicon_fetched) override;
+  gfx::Image GetFaviconForDefaultSearchProvider(
+      FaviconFetchedCallback on_favicon_fetched) override;
   void OnCurrentMatchChanged(const AutocompleteMatch& match) override;
   void OnTextChanged(const AutocompleteMatch& current_match,
                      bool user_input_in_progress,
@@ -84,12 +89,20 @@ class ChromeOmniboxClient : public OmniboxClient {
   void OnBitmapFetched(const BitmapFetchedCallback& callback,
                        const SkBitmap& bitmap);
 
+  void OnDefaultSearchProviderFaviconFetched(
+      const favicon_base::FaviconImageResult& result);
+
   ChromeOmniboxEditController* controller_;
   Profile* profile_;
   ChromeAutocompleteSchemeClassifier scheme_classifier_;
   BitmapFetcherService::RequestId request_id_;
   FaviconCache favicon_cache_;
 
+  base::CancelableTaskTracker default_search_provider_favicon_task_tracker_;
+  FaviconFetchedCallback pending_default_search_provider_favicon_callback_;
+
+  base::WeakPtrFactory<ChromeOmniboxClient> weak_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(ChromeOmniboxClient);
 };
 

components/omnibox/browser/omnibox_client.cc

@@ -92,3 +92,8 @@ gfx::Image OmniboxClient::GetFaviconForPageUrl(
     FaviconFetchedCallback on_favicon_fetched) {
   return gfx::Image();
 }
+
+gfx::Image OmniboxClient::GetFaviconForDefaultSearchProvider(
+    FaviconFetchedCallback on_favicon_fetched) {
+  return gfx::Image();
+}

components/omnibox/browser/omnibox_client.h

@@ -28,9 +28,10 @@ namespace gfx {
 class Image;
 }
 
-typedef base::Callback<void(const SkBitmap& bitmap)> BitmapFetchedCallback;
-typedef base::OnceCallback<void(const gfx::Image& favicon)>
-    FaviconFetchedCallback;
+using BitmapFetchedCallback =
+    base::RepeatingCallback<void(const SkBitmap& bitmap)>;
+using FaviconFetchedCallback =
+    base::OnceCallback<void(const gfx::Image& favicon)>;
 
 // Interface that allows the omnibox component to interact with its embedder
 // (e.g., getting information about the current page, retrieving objects
@@ -125,14 +126,16 @@ class OmniboxClient {
                                const BitmapFetchedCallback& on_bitmap_fetched) {
   }
 
-  // Fetches the favicon for |page_url| if the embedder supports fetching
-  // favicons (not all embedders do). Returns the favicon if it is synchronously
-  // available. Otherwise, this method returns an empty gfx::Image and
-  // |on_favicon_fetched| may or may not be called asynchronously later.
-  // |on_favicon_fetched| will never be run synchronously.
+  // These two methods fetch favicons if the embedder supports it. Not all
+  // embedders do. These methods return the favicon synchronously if possible.
+  // Otherwise, they return an empty gfx::Image and |on_favicon_fetched| may or
+  // may not be called asynchronously later. |on_favicon_fetched| will never be
+  // run synchronously.
   virtual gfx::Image GetFaviconForPageUrl(
       const GURL& page_url,
       FaviconFetchedCallback on_favicon_fetched);
+  virtual gfx::Image GetFaviconForDefaultSearchProvider(
+      FaviconFetchedCallback on_favicon_fetched);
 
   // Called when the current autocomplete match has changed.
   virtual void OnCurrentMatchChanged(const AutocompleteMatch& match) {}

components/omnibox/browser/omnibox_view.cc

@@ -119,13 +119,14 @@ gfx::ImageSkia OmniboxView::GetIcon(int dip_size,
                            : AutocompleteMatchType::URL_WHAT_YOU_TYPED;
   if (ui::MaterialDesignController::IsNewerMaterialUi() &&
       AutocompleteMatch::IsSearchType(type)) {
-    // TODO(tommycli): Implement fetching the default search provider and
-    // starting the async request for its favicon here. This will also need
-    // caching to provide good performance.
-
-    // Fall through to provide the vector icon until we receive the favicon.
-    // Note that the FaviconService can fail to fetch the favicon, in which
-    // the default vector icon we provide below should remain.
+    gfx::Image favicon = model_->client()->GetFaviconForDefaultSearchProvider(
+        std::move(on_icon_fetched));
+    if (!favicon.IsEmpty())
+      return favicon.AsImageSkia();
+
+    // If the client returns an empty favicon, fall through to provide the
+    // generic vector icon. |on_icon_fetched| may or may not be called later.
+    // If it's never called, the vector icon we provide below should remain.
   }
 
   const gfx::VectorIcon& vector_icon =