Extend FaviconService to support fetching favicons from a Google server

chrome/browser/favicon/favicon_service_factory.cc

Index: chrome/browser/favicon/favicon_service_factory.cc
diff --git a/chrome/browser/favicon/favicon_service_factory.cc b/chrome/browser/favicon/favicon_service_factory.cc
index 81528e4c69df599acce840c7d20146b536b1bbb4..00139213bcd91a9a7378ceaba1d11cbd367b12b6 100644
--- a/chrome/browser/favicon/favicon_service_factory.cc
+++ b/chrome/browser/favicon/favicon_service_factory.cc
@@ -9,8 +9,11 @@
 #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"
 
@@ -19,10 +22,14 @@ 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