Refactor TaskManager's favicon retrieval approach

chrome/browser/task_management/providers/web_contents/renderer_task.cc

Index: chrome/browser/task_management/providers/web_contents/renderer_task.cc
diff --git a/chrome/browser/task_management/providers/web_contents/renderer_task.cc b/chrome/browser/task_management/providers/web_contents/renderer_task.cc
index 371a7cb7b4b95b00749a6dfbd09dc3b7f31bc231..01a51f46bdbe58786aced0bf95486a12222b5c67 100644
--- a/chrome/browser/task_management/providers/web_contents/renderer_task.cc
+++ b/chrome/browser/task_management/providers/web_contents/renderer_task.cc
@@ -78,9 +78,17 @@ RendererTask::RendererTask(const base::string16& title,
   // All renderer tasks are capable of reporting network usage, so the default
   // invalid value of -1 doesn't apply here.
   OnNetworkBytesRead(0);
+
+  // Tag the web_contents with a |ContentFaviconDriver| (if needed) so that
+  // we can use it to observe favicons changes.
+  favicon::CreateContentFaviconDriverForWebContents(web_contents);
+  favicon::ContentFaviconDriver::FromWebContents(web_contents)->AddObserver(
+      this);
 }
 
 RendererTask::~RendererTask() {
+  favicon::ContentFaviconDriver::FromWebContents(web_contents())->
+      RemoveObserver(this);
 }
 
 void RendererTask::Activate() {
@@ -138,6 +146,17 @@ blink::WebCache::ResourceTypeStats RendererTask::GetWebCacheStats() const {
   return webcache_stats_;
 }
 
+void RendererTask::OnFaviconAvailable(const gfx::Image& image) {
+  // We ignore this event because it means the |image| has been retrieved, but
+  // not necessarily updated.

[pkotwicz, 2015/09/11 23:51:33]

Can you remove this comment? OnFaviconAvailable() has a super confusing name. It
is called whenever OnFaviconUpdated() is called on the desktop.

On Android, this is called when a apple-touch-icon is available in addition to
when a regular favicon is available. OnFaviconUpdated() is not called when a
apple-touch-icon is available.

[afakhry, 2015/09/12 00:42:21]

I removed the comment. But I have to say I don't know what "apple-touch-icon"
is. I want to confirm however that it's safe to ignore this icon.

[pkotwicz, 2015/09/14 14:39:17]

It is safe to ignore

apple-touch-icons were added by Apple to denote favicons with special effects
added. The intention of these icons is to provide a large icon when the web page
is added to the home screen. (You can read more about them at
https://en.wikipedia.org/wiki/Favicon) Chrome only cares about apple-touch-icons
on iOS and Android.

+}
+
+void RendererTask::OnFaviconUpdated(favicon::FaviconDriver* favicon_driver,
+                                    bool icon_url_changed) {
+  if (icon_url_changed)

[ncarter (slow), 2015/09/11 20:08:45]

What does it mean if this is called but |icon_url_changed| is false?

[pkotwicz, 2015/09/11 23:51:33]

You actually want to call UpdateFavicon() even if |icon_url_changed| is false.
This will be called with |icon_url_changed| == false if the favicon in the
database is expired and we have just finished downloading a new one.

The favicon pixels may be different. http://www.google.com/favicon.ico is
different today than it was a month ago

[afakhry, 2015/09/12 00:42:21]

Thanks for the clarification. Done!

+    UpdateFavicon();
+}
+
 // static
 base::string16 RendererTask::GetTitleFromWebContents(
     content::WebContents* web_contents) {
@@ -168,9 +187,6 @@ const gfx::ImageSkia* RendererTask::GetFaviconFromWebContents(
     content::WebContents* web_contents) {
   DCHECK(web_contents);
 
-  // Tag the web_contents with a |ContentFaviconDriver| (if needed) so that
-  // we can use it to retrieve the favicon if there is one.
-  favicon::CreateContentFaviconDriverForWebContents(web_contents);
   gfx::Image image =
       favicon::ContentFaviconDriver::FromWebContents(web_contents)->
           GetFavicon();