Add baked-in favicons for default popular sites on NTP

components/ntp_tiles/icon_cacher.h

 // Copyright 2017 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.
 
 #ifndef COMPONENTS_NTP_TILES_ICON_CACHER_H_
 #define COMPONENTS_NTP_TILES_ICON_CACHER_H_
 
 #include "base/callback.h"
 #include "components/ntp_tiles/popular_sites.h"
 
 namespace ntp_tiles {
 
 // Ensures that a Popular Sites icon is cached, downloading and saving it if
 // not.
 //
 // Does not provide any way to get a fetched favicon; use the FaviconService for
 // that. All this interface guarantees is that FaviconService will be able to
 // get you an icon (if it exists).
 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
 
 #endif  // COMPONENTS_NTP_TILES_ICON_CACHER_H_