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 566202b07d88c8cc228312ce4c98dbf0a5776b03..5d08d4ef3b837468645c1fe87b9fff240f10dd81 100644
--- a/components/ntp_tiles/popular_sites_impl.cc
+++ b/components/ntp_tiles/popular_sites_impl.cc
@@ -129,10 +129,23 @@ PopularSites::SitesVector ParseSiteList(const base::ListValue& list) {
 
     sites.emplace_back(title, GURL(url), GURL(favicon_url),
                        GURL(large_icon_url), GURL(thumbnail_url));
+    item->GetInteger("default_resource_id", &sites.back().default_resource_id);
   }
   return sites;
 }
 
+#if defined(OS_ANDROID) || defined(OS_IOS)
+void SetDefaultResourceForSite(base::ListValue* sites,
+                               int index,
+                               int resource_id) {
+  base::DictionaryValue* site;
+  if (!sites->GetDictionary(index, &site)) {
+    return;
+  }
+  site->SetInteger("default_resource_id", resource_id);

[sfiera, 2017/02/16 18:54:16]

Feels a little weird to be storing a resource ID in preferences. But I guess
it's not really in the preferences, just an ephemeral default value…

[fhorschig, 2017/02/17 16:24:04]

Yes. With the first real fetch, this value is gone.
I had the same thought but adding this property apart from the
definition of the default popular sites makes for some huge exceptions
and likely pitfalls.

+}
+#endif
+
 // Creates the list of popular sites based on a snapshot available for mobile.
 std::unique_ptr<base::ListValue> DefaultPopularSites() {
 #if defined(OS_ANDROID) || defined(OS_IOS)
@@ -141,6 +154,14 @@ std::unique_ptr<base::ListValue> DefaultPopularSites() {
           ResourceBundle::GetSharedInstance().GetRawDataResource(
               IDR_DEFAULT_POPULAR_SITES_JSON)));
   DCHECK(sites);
+  SetDefaultResourceForSite(sites.get(), 0, IDR_DEFAULT_POPULAR_SITES_ICON0);

[sfiera, 2017/02/16 18:54:16]

Couldn't you write this as a foreach loop? Like:

  int index = 0;
  for (int resource : {IDR_DEFAULT_POPULAR_SITES_ICON0,
                       …}) {

[fhorschig, 2017/02/17 16:24:04]

YES! This is exactly what I have been looking for. Thanks!

+  SetDefaultResourceForSite(sites.get(), 1, IDR_DEFAULT_POPULAR_SITES_ICON1);
+  SetDefaultResourceForSite(sites.get(), 2, IDR_DEFAULT_POPULAR_SITES_ICON2);
+  SetDefaultResourceForSite(sites.get(), 3, IDR_DEFAULT_POPULAR_SITES_ICON3);
+  SetDefaultResourceForSite(sites.get(), 4, IDR_DEFAULT_POPULAR_SITES_ICON4);
+  SetDefaultResourceForSite(sites.get(), 5, IDR_DEFAULT_POPULAR_SITES_ICON5);
+  SetDefaultResourceForSite(sites.get(), 6, IDR_DEFAULT_POPULAR_SITES_ICON6);
+  SetDefaultResourceForSite(sites.get(), 7, IDR_DEFAULT_POPULAR_SITES_ICON7);
   return sites;
 #endif
   return base::MakeUnique<base::ListValue>();
@@ -157,7 +178,8 @@ PopularSites::Site::Site(const base::string16& title,
       url(url),
       favicon_url(favicon_url),
       large_icon_url(large_icon_url),
-      thumbnail_url(thumbnail_url) {}
+      thumbnail_url(thumbnail_url),
+      default_resource_id(-1) {}  // Valid resources would be >=0

[sfiera, 2017/02/16 18:54:16]

I don't think any comment is needed in addition to the header.

[fhorschig, 2017/02/17 16:24:04]

Done.

 
 PopularSites::Site::Site(const Site& other) = default;