Refactor TaskManager's favicon retrieval approach

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

 // Copyright 2015 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.
 
 #include "chrome/browser/task_management/providers/web_contents/renderer_task.h"
 
 #include "base/i18n/rtl.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/browser_process.h"
@@ skipping 60 matching lines @@
       renderer_resources_sampler_(
           CreateRendererResourcesSampler(render_process_host_)),
       render_process_id_(render_process_host_->GetID()),
       v8_memory_allocated_(0),
       v8_memory_used_(0),
       webcache_stats_(),
       profile_name_(GetRendererProfileName(render_process_host_)) {
   // 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() {
   if (!web_contents_->GetDelegate())
     return;
 
   web_contents_->GetDelegate()->ActivateContents(web_contents_);
 }
 
 void RendererTask::Refresh(const base::TimeDelta& update_interval,
@@ skipping 37 matching lines @@
 }
 
 bool RendererTask::ReportsWebCacheStats() const {
   return true;
 }
 
 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) {
   DCHECK(web_contents);
   base::string16 title = web_contents->GetTitle();
   if (title.empty()) {
     GURL url = web_contents->GetURL();
     title = base::UTF8ToUTF16(url.spec());
     // Force URL to be LTR.
     title = base::i18n::GetDisplayStringInLTRDirectionality(title);
@@ skipping 10 matching lines @@
     base::i18n::AdjustStringForLocaleDirection(&title);
   }
   return title;
 }
 
 // static
 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();
   if (image.IsEmpty())
     return nullptr;
 
   return image.ToImageSkia();
 }
 
 // static
@@ skipping 18 matching lines @@
     if (is_incognito)
       message_id = IDS_TASK_MANAGER_EXTENSION_INCOGNITO_PREFIX;
     else
       message_id = IDS_TASK_MANAGER_EXTENSION_PREFIX;
   }
 
   return l10n_util::GetStringFUTF16(message_id, title);
 }
 
 }  // namespace task_management