Extend FaviconService to support fetching favicons from a Google server

chrome/browser/favicon/favicon_service_factory.cc

 // Copyright (c) 2012 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 "chrome/browser/favicon/favicon_service_factory.h"
 
 #include "base/memory/ptr_util.h"
 #include "base/memory/singleton.h"
 #include "chrome/browser/favicon/chrome_favicon_client.h"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/search/suggestions/image_decoder_impl.h"
 #include "components/favicon/core/favicon_service.h"
 #include "components/history/core/browser/history_service.h"
+#include "components/image_fetcher/image_decoder.h"
+#include "components/image_fetcher/image_fetcher_impl.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "components/prefs/pref_service.h"
 
 namespace {
 
 std::unique_ptr<KeyedService> BuildFaviconService(
     content::BrowserContext* context) {
   Profile* profile = Profile::FromBrowserContext(context);
+
   return base::MakeUnique<favicon::FaviconService>(
       base::WrapUnique(new ChromeFaviconClient(profile)),
       HistoryServiceFactory::GetForProfile(profile,
-                                           ServiceAccessType::EXPLICIT_ACCESS));
+                                           ServiceAccessType::EXPLICIT_ACCESS),
+      base::MakeUnique<image_fetcher::ImageFetcherImpl>(
+          base::MakeUnique<suggestions::ImageDecoderImpl>(),
+          profile->GetRequestContext()));

[pkotwicz, 2016/12/19 00:03:28]

Is ImageFetcherImpl the new hot thing? ContentFaviconDriver uses
WebContents::DownloadImage(). It is kind of confusing for the FaviconService to
use both.

It looks like ImageFetcherImpl is not ready to be used in
ContentFaviconDriver::StartDownload() because it does not decode all of the
frames from a .ico file

[jkrcal, 2016/12/19 17:07:19]

I need to ask colleagues whether image fetcher has any advantage over
WebContents::DownloadImage(). 

Would you like me to convert my new code to WebContents::DownloadImage()?

[pkotwicz, 2016/12/20 02:33:01]

If I were you I would wait to get feedback from your colleagues first.

If ImageFetcherImpl is the new hotness, I am all for using it. If there is no
preference between the two, I suggest using WebContents::DownloadImage() for the
sake of consistency

[jkrcal, 2017/02/06 19:01:22]

Indeed, the ImageFetcherImpl is not ready to decode all frames from .ico files.
So at the moment, we cannot implement fetching images in FaviconDriverImpl
generally using ImageFetcherImpl.

Anyway, after taking another look, I am not convinced about using
WebContents::DownloadImage() here. Before my CL, FaviconService never initiated
downloading images on its own. 

WebContents::DownloadImage() is currently called from FaviconDriver
(ContentFaviconDriver, in particular). It does not feel correct for the
FaviconService to ask the FaviconDriver to do the job because:
 - FaviconDriver is conceptually bound to a tab (being a TabHelper) whereas
FaviconService::Get1xFaviconForPageURLDownloadFromGoogleServerIfMissing() is
conceptually tab-independent (as we can ask for icons from pages that are not
loaded in any open tab).
 - FaviconService has no reference to any FaviconDriver instance and it does not
feel correct to add this dependency.

WDYT, Peter?

[pkotwicz, 2017/02/07 04:55:54]

I am ok with the ImageFetcherImpl

 }
 
 }  // namespace
 
 // static
 favicon::FaviconService* FaviconServiceFactory::GetForProfile(
     Profile* profile,
     ServiceAccessType sat) {
   if (!profile->IsOffTheRecord()) {
     return static_cast<favicon::FaviconService*>(
@@ skipping 32 matching lines @@
 }
 
 KeyedService* FaviconServiceFactory::BuildServiceInstanceFor(
     content::BrowserContext* context) const {
   return BuildFaviconService(context).release();
 }
 
 bool FaviconServiceFactory::ServiceIsNULLWhileTesting() const {
   return true;
 }