Ensure PopularSite icon availability in ntp_tiles.

components/ntp_tiles/icon_cacher.h

+// Copyright 2015 The Chromium Authors. All rights reserved.

[Marc Treib, 2016/10/12 09:11:28]

2016

[sfiera, 2016/10/13 09:06:46]

Done.

+// 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 "base/memory/weak_ptr.h"
+#include "components/favicon/core/favicon_service.h"
+#include "components/favicon/core/favicon_util.h"
+#include "components/favicon_base/favicon_types.h"
+#include "components/favicon_base/favicon_util.h"
+#include "components/image_fetcher/image_fetcher.h"
+#include "components/ntp_tiles/popular_sites.h"
+#include "url/gurl.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 class does is guarantee that FaviconService will be able to
+// get you an icon (if it exists).
+class IconCacher {

[Marc Treib, 2016/10/12 09:11:28]

Hm, I'm not entirely happy with the name - caching isn't really what this class
does. FaviconFetcher maybe?

[sfiera, 2016/10/13 09:06:46]

I don't like "Favicon" in the name, because that's a specific type of icon, and
we prefer TOUCH_ICON over FAVICON if available.

I'm not sure how I feel about "Fetcher". In the common case, we won't actually
fetch anything, we'll just verify that the icon is cached and move on. But I
guess IconFetcher would be OK with me.

[Marc Treib, 2016/10/13 12:31:15]

It'd be consistent with FaviconService, which also handles both types. I guess
"favicon" is also used as a term that encompasses all kinds of "site icon"? But
I don't feel that strongly.
So it either fetches an icon or does nothing... IconMaybeFetcher? :D

+ public:
+  IconCacher(favicon::FaviconService* favicon_service,
+             std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher);

[Marc Treib, 2016/10/12 09:11:28]

#include <memory>

[sfiera, 2016/10/13 09:06:46]

Done.

+  ~IconCacher();
+
+  // 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).
+  void StartFetch(PopularSites::Site site, base::Callback<void(bool)> done);

[Marc Treib, 2016/10/12 09:11:28]

Pass the callback by const ref (to avoid some unnecessary copies).
optional: You could also add an alias for it.

[sfiera, 2016/10/13 09:06:45]

Done.

+
+ private:
+  base::CancelableTaskTracker tracker_;
+  favicon::FaviconService* const favicon_service_;
+  std::unique_ptr<image_fetcher::ImageFetcher> const image_fetcher_;
+
+  base::WeakPtrFactory<IconCacher> weak_ptr_factory_;

[Marc Treib, 2016/10/12 09:11:28]

Methods come before member variables

[sfiera, 2016/10/13 09:06:46]

Done.

+
+  // Picks right icon URL and type based on presence of large_icon_url.
+  static favicon_base::IconType IconType(const PopularSites::Site& site);
+  static const GURL& IconURL(const PopularSites::Site& site);

[Marc Treib, 2016/10/12 09:11:28]

Are these needed by tests? If not, you could move them into an anonymous
namespace in the .cc

[sfiera, 2016/10/13 09:06:46]

Done.

+
+  void OnGetFaviconImageForPageURLFinished(
+      PopularSites::Site site,
+      base::Callback<void(bool)> done,

[Marc Treib, 2016/10/12 09:11:28]

Also here: const ref

[sfiera, 2016/10/13 09:06:46]

Done.

+      const favicon_base::FaviconImageResult& result);
+
+  void OnFaviconDownloaded(PopularSites::Site site,
+                           base::Callback<void(bool)> done,

[Marc Treib, 2016/10/12 09:11:28]

And here

[sfiera, 2016/10/13 09:06:46]

Done.

+                           const std::string& id,
+                           const gfx::Image& fetched_image);
+};
+
+}  // namespace ntp_tiles
+
+#endif  // COMPONENTS_NTP_TILES_ICON_CACHER_H_