Add baked-in favicons for default popular sites on NTP

components/ntp_tiles/icon_cacher.h

Index: components/ntp_tiles/icon_cacher.h
diff --git a/components/ntp_tiles/icon_cacher.h b/components/ntp_tiles/icon_cacher.h
index 22beeb6f48c415b3616e15c8c1a94a6b4b9b3c23..e41cd85646e66476b0cad0317f9f0b99e54cb283 100644
--- a/components/ntp_tiles/icon_cacher.h
+++ b/components/ntp_tiles/icon_cacher.h
@@ -20,10 +20,11 @@ class IconCacher {
  public:
   virtual ~IconCacher() = default;
 
-  // Fetches the icon if necessary, then invokes |done| with true if it was
-  // newly fetched (false if it was already cached or could not be fetched).
+  // Fetches the icon if necessary. It invokes |icon_available| if an icon was
+  // newly fetched or loaded from a default source.

[mastiz, 2017/03/01 08:48:12]

How about the failure case? I see you removed the boolean but that seems, at
least, orthogonal to your patch.

[fhorschig, 2017/03/01 20:57:35]

There is exactly one caller (as the previous reviewer pointed out)
which doesn't even need the failure case.

[mastiz, 2017/03/02 11:05:39]

I don't feel strongly but it feels wrong that an API doesn't notify completion
in some circumstances. I would also expect this useful or testing (that is, how
would you test otherwise that the callback won't be called?).

[fhorschig, 2017/03/02 11:56:24]

Relatable. I will consider reintroducing it.
So far, we wait for pending tasks to be finished. 

The thing about the icon cacher is, that it works in the background and
failure are usually not interesting to anyone at all.
As soon as we rely on that (even if just for metrics), we should improve
this class.

+  // The callback will be invoked again if a newer icon has been fetched.
   virtual void StartFetch(PopularSites::Site site,
-                          const base::Callback<void(bool)>& done) = 0;
+                          const base::Callback<void()>& icon_available) = 0;

[mastiz, 2017/03/01 08:48:12]

Having the callback called twice is not something I like much.

As a slightly less ugly alternative, you could introduce a second callback.

More interestingly:
a) Please talk to UX whether we should actually update icons in this situation.
b) For longer term, ntp_tiles might as well observe all FaviconService icon
change notifications.

[fhorschig, 2017/03/01 20:57:35]

We now have two OnceCallbacks 
Asked and will receive an answer tomorrow. I suggest that this behavior
should be changed in the near future. This CL contains already more
changes than I prefer.
Absolutely.

[mastiz, 2017/03/02 11:05:39]

Ack, let's wait for UX feedback, since this could simplify the code.

[fhorschig, 2017/03/02 11:56:24]

Acknowledged.

 };
 
 }  // namespace ntp_tiles