Add baked-in favicons for default popular sites on NTP

components/ntp_tiles/popular_sites_impl.cc

Index: components/ntp_tiles/popular_sites_impl.cc
diff --git a/components/ntp_tiles/popular_sites_impl.cc b/components/ntp_tiles/popular_sites_impl.cc
index 34fa0274a06fdae706fd592b9930bb656fe5d22b..eff5c9c54090a7e2628b304887220c15fed6c4e5 100644
--- a/components/ntp_tiles/popular_sites_impl.cc
+++ b/components/ntp_tiles/popular_sites_impl.cc
@@ -17,6 +17,8 @@
 #include "base/time/time.h"
 #include "base/values.h"
 #include "components/data_use_measurement/core/data_use_user_data.h"
+#include "components/favicon/core/favicon_service.h"
+#include "components/favicon_base/favicon_types.h"
 #include "components/google/core/browser/google_util.h"
 #include "components/ntp_tiles/constants.h"
 #include "components/ntp_tiles/field_trial.h"
@@ -134,11 +136,52 @@ PopularSites::SitesVector ParseSiteList(const base::ListValue& list) {
 
 // This allows instantiation of PopularSites without registered prefs.
 // That case can occur during tests.
-PopularSites::SitesVector GetDefaultFromPrefs(const PrefService& prefs) {
+PopularSites::SitesVector GetDefaultFromPrefs(

[jkrcal, 2017/02/14 08:23:06]

drive-by comment: When is this function called?

You should make sure you never overwrite anything in the favicon service
(because possibly messing up with sync). In other words, you should not
SetFavicons(site_url, ...) if there are already same favicons for the given
site_url.

[sfiera, 2017/02/14 09:59:16]

Even in the absence of sync, I think this implementation would prevent us from
seeing new favicon if a popular site ever updated it, right? Every time an NTP
is opened, it will override the update.

Did you consider implementing this feature as an implementation of IconCacher
(that falls back to IconCacherImpl if it doesn't have an icon)? That seems like
a good place to encapsulate this logic.

[fhorschig, 2017/02/14 17:29:02]

Good remark. It's now called by the IconCacher which tries to retrieve an
existing icon first.
 
Right, this was not at all the right place for this.
The IconFetcher not only is a better place but also allows to load the resources
only if really needed and to mock the favicon_service part for tests.

+    const PrefService& prefs,
+    favicon::FaviconService* favicons) {
   if (!ShouldShowPopularSites()) {
     return PopularSites::SitesVector();
   }
-  return ParseSiteList(*prefs.GetList(kPopularSitesJsonPref));
+  PopularSites::SitesVector sites =
+      ParseSiteList(*prefs.GetList(kPopularSitesJsonPref));
+#if defined(OS_ANDROID) || defined(OS_IOS)

[tschumann, 2017/02/14 11:56:51]

i wonder why is this part only relevant to mobile platforms but not the first
part? Do we ever want this method to be called on other platforms?

[fhorschig, 2017/02/14 17:29:02]

It used to load the defaults and if on mobile, register the resources.
Now, these parts are separated again.

+  if (!favicons) {
+    return sites;
+  }
+  favicons->SetFavicons(sites[0].url, sites[0].large_icon_url,

[tschumann, 2017/02/14 11:56:51]

+mastiz who looked into this.
Feels weird to set this every time we load the popular sites from prefs. Also
this is quite a tricky setup. Would it be possible to have the fallback on a
higher level?
Like at some point up the stack, we make the decision to load popular sites and
figure out we don't have any. There, we could simply load all data (sites +
icons) from the built-in config and bypass the favicon service. 
I'm deferring this to Mikel as I have only little understanding of that stack
and it's dependencies.

[mastiz, 2017/02/14 14:44:59]

This comment seems obsolete now. Please loop me in when this has settled down a
bit, there's quite some reviewers already :-)

[fhorschig, 2017/02/14 17:29:02]

Like Jan and Chris have pointed out, this was neither the right place nor the
right frequency.
The loading happens in the IconCacher (where all tile images are loaded) and
notifying the cacher about the default tiles happens when the cacher is build in
the chrome_most_visited_sites_factory.

[fhorschig, 2017/02/14 17:29:02]

Absolutely. It seems to be quite an interesting CL with lots of shortcomings ...

+                        favicon_base::IconType::FAVICON,
+                        ResourceBundle::GetSharedInstance().GetImageNamed(
+                            IDR_DEFAULT_POPULAR_SITES_ICON0));
+  favicons->SetFavicons(sites[1].url, sites[1].large_icon_url,
+                        favicon_base::IconType::FAVICON,
+                        ResourceBundle::GetSharedInstance().GetImageNamed(
+                            IDR_DEFAULT_POPULAR_SITES_ICON1));
+  favicons->SetFavicons(sites[2].url, sites[2].large_icon_url,
+                        favicon_base::IconType::FAVICON,
+                        ResourceBundle::GetSharedInstance().GetImageNamed(
+                            IDR_DEFAULT_POPULAR_SITES_ICON2));
+  favicons->SetFavicons(sites[3].url, sites[3].large_icon_url,
+                        favicon_base::IconType::FAVICON,
+                        ResourceBundle::GetSharedInstance().GetImageNamed(
+                            IDR_DEFAULT_POPULAR_SITES_ICON3));
+  favicons->SetFavicons(sites[4].url, sites[4].large_icon_url,
+                        favicon_base::IconType::FAVICON,
+                        ResourceBundle::GetSharedInstance().GetImageNamed(
+                            IDR_DEFAULT_POPULAR_SITES_ICON4));
+  favicons->SetFavicons(sites[5].url, sites[5].large_icon_url,
+                        favicon_base::IconType::FAVICON,
+                        ResourceBundle::GetSharedInstance().GetImageNamed(
+                            IDR_DEFAULT_POPULAR_SITES_ICON5));
+  favicons->SetFavicons(sites[6].url, sites[6].large_icon_url,
+                        favicon_base::IconType::FAVICON,
+                        ResourceBundle::GetSharedInstance().GetImageNamed(
+                            IDR_DEFAULT_POPULAR_SITES_ICON6));
+  favicons->SetFavicons(sites[7].url, sites[7].large_icon_url,
+                        favicon_base::IconType::FAVICON,
+                        ResourceBundle::GetSharedInstance().GetImageNamed(
+                            IDR_DEFAULT_POPULAR_SITES_ICON7));
+#endif
+  return sites;
 }
 
 // Creates the List of popular sites based on a snapshot available for mobile.
@@ -176,6 +219,7 @@ PopularSitesImpl::PopularSitesImpl(
     PrefService* prefs,
     const TemplateURLService* template_url_service,
     VariationsService* variations_service,
+    favicon::FaviconService* favicon_service,
     net::URLRequestContextGetter* download_context,
     const base::FilePath& directory,
     ParseJSONCallback parse_json)
@@ -187,7 +231,7 @@ PopularSitesImpl::PopularSitesImpl(
       download_context_(download_context),
       parse_json_(std::move(parse_json)),
       is_fallback_(false),
-      sites_(GetDefaultFromPrefs(*prefs_)),
+      sites_(GetDefaultFromPrefs(*prefs_, favicon_service)),

[sfiera, 2017/02/14 09:59:16]

Initializing the favicon service in PopularSitesImpl's constructor is a
surprising side-effect.

[fhorschig, 2017/02/14 17:29:02]

Agreed and removed.

       weak_ptr_factory_(this) {
   // If valid path provided, remove local files created by older versions.
   if (!directory.empty() && blocking_runner_) {