ntp_tiles: Extend chrome://ntp-tiles-internals with favicon data

components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc

Index: components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc
diff --git a/components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc b/components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc
index ea6ee51c7586c9cd7908b3590db9ebc14d34c681..9de6359c950b795271769ebe2ca26c5814e35c0b 100644
--- a/components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc
+++ b/components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc
@@ -13,6 +13,7 @@
 #include "base/memory/ptr_util.h"
 #include "base/task_runner_util.h"
 #include "base/values.h"
+#include "components/favicon/core/favicon_service.h"
 #include "components/ntp_tiles/most_visited_sites.h"
 #include "components/ntp_tiles/pref_names.h"
 #include "components/ntp_tiles/webui/ntp_tiles_internals_message_handler_client.h"
@@ -34,8 +35,18 @@ std::string FormatJson(const base::Value& value) {
 
 }  // namespace
 
-NTPTilesInternalsMessageHandler::NTPTilesInternalsMessageHandler()
-    : client_(nullptr), site_count_(8), weak_ptr_factory_(this) {}
+NTPTilesInternalsMessageHandler::NTPTilesInternalsMessageHandler(
+    favicon::FaviconService* favicon_service)
+    : favicon_service_(favicon_service),
+      icon_types_and_names_{

[sfiera, 2017/06/13 08:56:01]

This looks like it could be a constexpr std::array<std::pair<>> local to the .cc
file.

[mastiz, 2017/06/13 11:19:39]

Done.

+          {favicon_base::FAVICON, "FAVICON"},
+          {favicon_base::TOUCH_ICON, "TOUCH_ICON"},
+          {favicon_base::TOUCH_PRECOMPOSED_ICON, "TOUCH_PRECOMPOSED_ICON"},
+          {favicon_base::WEB_MANIFEST_ICON, "WEB_MANIFEST_ICON"},
+      },
+      client_(nullptr),
+      site_count_(8),
+      weak_ptr_factory_(this) {}
 
 NTPTilesInternalsMessageHandler::~NTPTilesInternalsMessageHandler() = default;
 
@@ -73,7 +84,7 @@ void NTPTilesInternalsMessageHandler::HandleRegisterForEvents(
     disabled.SetBoolean("whitelist", false);
     client_->CallJavascriptFunction(
         "chrome.ntp_tiles_internals.receiveSourceInfo", disabled);
-    SendTiles(NTPTilesVector());
+    SendTiles(NTPTilesVector(), FaviconResultMap());
     return;
   }
   DCHECK(args->empty());
@@ -216,7 +227,9 @@ void NTPTilesInternalsMessageHandler::SendSourceInfo() {
       "chrome.ntp_tiles_internals.receiveSourceInfo", value);
 }
 
-void NTPTilesInternalsMessageHandler::SendTiles(const NTPTilesVector& tiles) {
+void NTPTilesInternalsMessageHandler::SendTiles(
+    const NTPTilesVector& tiles,
+    const FaviconResultMap& result_map) {
   auto sites_list = base::MakeUnique<base::ListValue>();
   for (const NTPTile& tile : tiles) {
     auto entry = base::MakeUnique<base::DictionaryValue>();
@@ -225,6 +238,21 @@ void NTPTilesInternalsMessageHandler::SendTiles(const NTPTilesVector& tiles) {
     entry->SetInteger("source", static_cast<int>(tile.source));
     entry->SetString("whitelistIconPath",
                      tile.whitelist_icon_path.LossyDisplayName());
+
+    auto icon_list = base::MakeUnique<base::ListValue>();
+    for (const auto& icon_type : icon_types_and_names_) {
+      FaviconResultMap::const_iterator it = result_map.find(
+          FaviconResultMap::key_type(tile.url, icon_type.first));
+
+      if (it != result_map.end()) {
+        auto icon = base::MakeUnique<base::DictionaryValue>();
+        icon->SetString("url", it->second.icon_url.spec());
+        icon->SetString("type", icon_type.second);
+        icon_list->Append(std::move(icon));
+      }
+    }
+    entry->Set("icons", std::move(icon_list));
+
     sites_list->Append(std::move(entry));
   }
 
@@ -236,10 +264,42 @@ void NTPTilesInternalsMessageHandler::SendTiles(const NTPTilesVector& tiles) {
 
 void NTPTilesInternalsMessageHandler::OnMostVisitedURLsAvailable(
     const NTPTilesVector& tiles) {

[sfiera, 2017/06/13 08:56:01]

First, cancelable_task_tracker_.CancelAll() in case we're still trying to load
icons.

[mastiz, 2017/06/13 11:19:39]

Done.

-  SendTiles(tiles);
+  if (tiles.empty())

[sfiera, 2017/06/13 08:56:00]

Nit: braces
And early return?

[mastiz, 2017/06/13 11:19:39]

Done.

+    SendTiles(tiles, FaviconResultMap());
+
+  auto on_lookup_done = base::Bind(

[sfiera, 2017/06/13 08:56:01]

BindRepeating() to make the usage clear?

[mastiz, 2017/06/13 11:19:39]

Done.

+      &NTPTilesInternalsMessageHandler::OnFaviconLookupDone,
+      base::Unretained(this), tiles, base::Owned(new FaviconResultMap()),

[sfiera, 2017/06/13 08:56:01]

Can you add a comment about Unretained? It's safe because of
|cancelable_task_tracker_|, right?

[mastiz, 2017/06/13 11:19:39]

Done. However, I fail to see why this is different to using |weak_ptr_factory_|
which is not documented similarly.

[sfiera, 2017/06/13 11:34:13]

Whenever I see Unretained(), I wonder what mechanism allows it to be safe. When
I see weak_ptr_factory_, I know the mechanism.

+      base::Owned(new size_t(tiles.size() * icon_types_and_names_.size())));
+
+  for (const NTPTile& tile : tiles) {
+    for (const auto& icon_type : icon_types_and_names_) {
+      favicon_service_->GetLargestRawFaviconForPageURL(
+          tile.url, std::vector<int>(1U, icon_type.first),
+          /*minimum_size_in_pixels=*/0, base::Bind(on_lookup_done, tile.url),
+          &cancelable_task_tracker_);
+    }
+  }
 }
 
 void NTPTilesInternalsMessageHandler::OnIconMadeAvailable(
     const GURL& site_url) {}
 
+void NTPTilesInternalsMessageHandler::OnFaviconLookupDone(
+    const NTPTilesVector& tiles,
+    FaviconResultMap* result_map,
+    size_t* num_pending_lookups,
+    const GURL& page_url,
+    const favicon_base::FaviconRawBitmapResult& result) {
+  DCHECK_NE(0, *num_pending_lookups);
+
+  result_map->emplace(
+      std::pair<GURL, favicon_base::IconType>(page_url, result.icon_type),
+      result);
+
+  --*num_pending_lookups;
+  if (*num_pending_lookups == 0)
+    SendTiles(tiles, *result_map);
+}
+
 }  // namespace ntp_tiles