Support touch icon in FaviconHelper

chrome/browser/favicon_helper.cc

Index: chrome/browser/favicon_helper.cc
diff --git a/chrome/browser/favicon_helper.cc b/chrome/browser/favicon_helper.cc
index 7ad421ca0381a5c057d12b406763cb916c1f2329..eee2a4f88b3518c5acad7ae4dbbdbd555c8e8d9d 100644
--- a/chrome/browser/favicon_helper.cc
+++ b/chrome/browser/favicon_helper.cc
@@ -22,11 +22,14 @@
 #include "ui/gfx/codec/png_codec.h"
 #include "ui/gfx/favicon_size.h"
 
-FaviconHelper::FaviconHelper(TabContents* tab_contents)
+FaviconHelper::FaviconHelper(TabContents* tab_contents,
+                             int icon_types,
+                             int preferred_icon_size)
     : TabContentsObserver(tab_contents),
-      got_favicon_url_(false),
       got_favicon_from_history_(false),
-      favicon_expired_(false) {
+      favicon_expired_(false),
+      icon_types_(icon_types),
+      preferred_icon_size_(preferred_icon_size) {

[sky, 2011/03/22 19:48:48]

Initialize current_candidate_idx too

[michaelbai, 2011/03/22 23:59:03]

Done.

 }
 
 FaviconHelper::~FaviconHelper() {
@@ -46,13 +49,15 @@ void FaviconHelper::FetchFavicon(const GURL& url) {
 
   url_ = url;
 
-  favicon_expired_ = got_favicon_from_history_ = got_favicon_url_ = false;
+  favicon_expired_ = got_favicon_from_history_ = false;
+  current_candidate_idx_ = 0;
+  favicon_candidates_.empty();
 
   // Request the favicon from the history service. In parallel to this the
   // renderer is going to notify us (well TabContents) when the favicon url is
   // available.
   if (GetFaviconService()) {
-    GetFaviconService()->GetFaviconForURL(url_, history::FAVICON,
+    GetFaviconService()->GetFaviconForURL(url_, icon_types_,
         &cancelable_consumer_,
         NewCallback(this, &FaviconHelper::OnFaviconDataForInitialURL));
   }
@@ -60,9 +65,10 @@ void FaviconHelper::FetchFavicon(const GURL& url) {
 
 int FaviconHelper::DownloadImage(const GURL& image_url,
                                  int image_size,
+                                 history::IconType icon_type,
                                  ImageDownloadCallback* callback) {
   DCHECK(callback);  // Must provide a callback.
-  return ScheduleDownload(GURL(), image_url, image_size, callback);
+  return ScheduleDownload(GURL(), image_url, image_size, icon_type, callback);
 }
 
 Profile* FaviconHelper::profile() {
@@ -76,19 +82,20 @@ FaviconService* FaviconHelper::GetFaviconService() {
 void FaviconHelper::SetFavicon(
     const GURL& url,
     const GURL& image_url,
-    const SkBitmap& image) {
-  const SkBitmap& sized_image =
-      (image.width() == kFaviconSize && image.height() == kFaviconSize)
-      ? image : ConvertToFaviconSize(image);
+    const SkBitmap& image,
+    history::IconType icon_type) {
+  const SkBitmap& sized_image = (preferred_icon_size_ == 0 ||
+      (preferred_icon_size_ == image.width() &&
+          preferred_icon_size_ == image.height())) ?
+      image : ConvertToFaviconSize(image);
 
   if (GetFaviconService() && ShouldSaveFavicon(url)) {
     std::vector<unsigned char> image_data;
     gfx::PNGCodec::EncodeBGRASkBitmap(sized_image, false, &image_data);
-    GetFaviconService()->SetFavicon(url, image_url, image_data,
-                                    history::FAVICON);
+    GetFaviconService()->SetFavicon(url, image_url, image_data, icon_type);
   }
 
-  if (url == url_) {
+  if (url == url_ && icon_type == history::FAVICON) {
     NavigationEntry* entry = GetEntry();
     if (entry)
       UpdateFavicon(entry, sized_image);
@@ -114,41 +121,42 @@ void FaviconHelper::UpdateFavicon(NavigationEntry* entry,
   tab_contents()->NotifyNavigationStateChanged(TabContents::INVALIDATE_TAB);
 }
 
-void FaviconHelper::OnUpdateFaviconURL(int32 page_id, const GURL& icon_url) {
+void FaviconHelper::OnUpdateFaviconURL(
+    int32 page_id,
+    std::vector<FaviconURL> candidates) {
+  size_t candidates_count = favicon_candidates_.size();

[sky, 2011/03/22 19:48:48]

If candidates contains a FaviconURL whose type matches what you want don't you
need to remove the existing one in favicon_candidates_? Similarly wouldn't you
need to reset candidate_index_?

[michaelbai, 2011/03/22 23:59:03]

No, I don't think we should reset favicon_candidates_, according previous logic,
the url will be ignored if FetchFavicon hasn't been called. 

[sky, 2011/03/23 00:04:35]

What happens if OnUpdateFaviconURL is invoked twice with different URLs?

[michaelbai, 2011/03/23 15:49:43]

The new URL is appended to the list, the first URL has priority.
In which case the OnUpdateFaviconURL() could be invoked twice? What's the
expected behavior if so?
 

+  for (std::vector<FaviconURL>::iterator i = candidates.begin();
+       i != candidates.end(); ++i) {
+    if (!i->icon_url.is_empty() && (i->icon_type & icon_types_))
+      favicon_candidates_.push_back(*i);
+  }
+
   // TODO(davemoore) Should clear on empty url. Currently we ignore it.
   // This appears to be what FF does as well.
-  if (icon_url.is_empty())
+  // No URL was added.
+  if (favicon_candidates_.size() == candidates_count)
     return;
 
   NavigationEntry* entry = GetEntry();
   if (!entry)
     return;
 
-  got_favicon_url_ = true;
-
-  if (!GetFaviconService())
-    return;
-
-  if (!favicon_expired_ && entry->favicon().is_valid() &&
-      entry->favicon().url() == icon_url) {
-    // We already have the icon, no need to proceed.
-    return;
-  }
-
-  entry->favicon().set_url(icon_url);
-
-  if (got_favicon_from_history_)
-    DownloadFaviconOrAskHistory(entry);
+  // DownloadFaviconOrAskHistory() only if we have gotten FaviconData from
+  // history and it's url or type is not same as current candidate.
+  if (GetFaviconService() && got_favicon_from_history_ && (favicon_expired_ ||

[sky, 2011/03/22 19:48:48]

wrap favicon_expired_ onto a new line so that this is easier to read.

[michaelbai, 2011/03/22 23:59:03]

Done.

+      !history_icon_.is_valid() || !IsSameFaviconURL(*candidate(),

[sky, 2011/03/22 19:48:48]

Don't you need to check entry->favicon().is_valid() too? At least when the type
is favicon.

[michaelbai, 2011/03/22 23:59:03]

Rewrote according previous logic.

+          history_icon_.icon_url, history_icon_.icon_type)))
+    DownloadFaviconOrAskHistory(entry->url(), candidate()->icon_url,
+         static_cast<history::IconType>(candidate()->icon_type));
 }
 
 bool FaviconHelper::OnMessageReceived(const IPC::Message& message) {
-  bool handled = true;
+  bool message_handled = true;

[sky, 2011/03/22 19:48:48]

Do you need the message_handled stuff anymore?

[michaelbai, 2011/03/22 23:59:03]

Yes, ViewHostMsg_DidDownloadFavicon still need to be handled

   IPC_BEGIN_MESSAGE_MAP(FaviconHelper, message)
-    IPC_MESSAGE_HANDLER(ViewHostMsg_UpdateFaviconURL, OnUpdateFaviconURL)
     IPC_MESSAGE_HANDLER(ViewHostMsg_DidDownloadFavicon, OnDidDownloadFavicon)
-    IPC_MESSAGE_UNHANDLED(handled = false)
+    IPC_MESSAGE_UNHANDLED(message_handled = false)
   IPC_END_MESSAGE_MAP()
-  return handled;
+  return message_handled;
 }
 
 void FaviconHelper::OnDidDownloadFavicon(int id,
@@ -165,9 +173,11 @@ void FaviconHelper::OnDidDownloadFavicon(int id,
   if (i->second.callback) {
     i->second.callback->Run(id, errored, image);
   } else if (!errored) {
-    SetFavicon(i->second.url, image_url, image);
+    SetFavicon(i->second.url, image_url, image, i->second.icon_type);
+  } else if (++current_candidate_idx_ < favicon_candidates_.size() &&
+        GetEntry()) {
+      FetchFavicon(candidate()->icon_url);
   }
-
   download_requests_.erase(i);
 }
 
@@ -182,19 +192,20 @@ NavigationEntry* FaviconHelper::GetEntry() {
   return NULL;
 }
 
-void FaviconHelper::OnFaviconDataForInitialURL(
-    FaviconService::Handle handle,
-    history::FaviconData favicon) {
+void FaviconHelper::OnFaviconDataForInitialURL(FaviconService::Handle handle,
+                                               history::FaviconData favicon) {
   NavigationEntry* entry = GetEntry();
   if (!entry)
     return;
 
   got_favicon_from_history_ = true;
+  history_icon_ = favicon;
 
   favicon_expired_ = (favicon.known_icon && favicon.expired);
 
-  if (favicon.known_icon && !entry->favicon().is_valid() &&
-      (!got_favicon_url_ || entry->favicon().url() == favicon.icon_url)) {
+  if (favicon.known_icon && favicon.icon_type == history::FAVICON &&

[sky, 2011/03/22 19:48:48]

This doesn't check !entry->favicon().is_valid()

[michaelbai, 2011/03/22 23:59:03]

Done.

+      (!candidate() || IsSameFaviconURL(*candidate(), favicon.icon_url,
+                                        favicon.icon_type))) {
     // The db knows the favicon (although it may be out of date) and the entry
     // doesn't have an icon. Set the favicon now, and if the favicon turns out
     // to be expired (or the wrong url) we'll fetch later on. This way the
@@ -206,35 +217,37 @@ void FaviconHelper::OnFaviconDataForInitialURL(
   }
 
   if (favicon.known_icon && !favicon.expired) {
-    if (got_favicon_url_ && entry->favicon().url() != favicon.icon_url) {
-      // Mapping in the database is wrong. DownloadFaviconOrAskHistory will
+    if (candidate() && !IsSameFaviconURL(*candidate(),favicon.icon_url,

[sky, 2011/03/22 19:48:48]

missing space at ,favicon....

[michaelbai, 2011/03/22 23:59:03]

Done.

+                                         favicon.icon_type)) {
+      // Mapping in the database is wrong. DownloadFavIconOrAskHistory will
       // update the mapping for this url and download the favicon if we don't
       // already have it.
-      DownloadFaviconOrAskHistory(entry);
+      DownloadFaviconOrAskHistory(entry->url(), candidate()->icon_url,
+          static_cast<history::IconType>(candidate()->icon_type));
     }
-  } else if (got_favicon_url_) {
+  } else if (candidate()) {
     // We know the official url for the favicon, by either don't have the
     // favicon or its expired. Continue on to DownloadFaviconOrAskHistory to
     // either download or check history again.
-    DownloadFaviconOrAskHistory(entry);
+    DownloadFaviconOrAskHistory(entry->url(), candidate()->icon_url,
+        static_cast<history::IconType>(candidate()->icon_type));
   }
   // else we haven't got the icon url. When we get it we'll ask the
   // renderer to download the icon.
 }
 
-void FaviconHelper::DownloadFaviconOrAskHistory(NavigationEntry* entry) {
-  DCHECK(entry);  // We should only get here if entry is valid.
+void FaviconHelper::DownloadFaviconOrAskHistory(const GURL& page_url,
+                                                const GURL& icon_url,
+                                                history::IconType icon_type) {
   if (favicon_expired_) {
     // We have the mapping, but the favicon is out of date. Download it now.
-    ScheduleDownload(entry->url(), entry->favicon().url(), kFaviconSize, NULL);
+    ScheduleDownload(page_url, icon_url, preferred_icon_size_, icon_type, NULL);
   } else if (GetFaviconService()) {
     // We don't know the favicon, but we may have previously downloaded the
     // favicon for another page that shares the same favicon. Ask for the
     // favicon given the favicon URL.
     if (profile()->IsOffTheRecord()) {
-      GetFaviconService()->GetFavicon(
-          entry->favicon().url(),
-          history::FAVICON,
+      GetFaviconService()->GetFavicon(icon_url, icon_type,
           &cancelable_consumer_,
           NewCallback(this, &FaviconHelper::OnFaviconData));
     } else {
@@ -244,42 +257,44 @@ void FaviconHelper::DownloadFaviconOrAskHistory(NavigationEntry* entry) {
       //    include the mapping between the page url and the favicon url.
       // This is asynchronous. The history service will call back when done.
       // Issue the request and associate the current page ID with it.
-      GetFaviconService()->UpdateFaviconMappingAndFetch(
-          entry->url(),
-          entry->favicon().url(),
-          history::FAVICON,
-          &cancelable_consumer_,
+      GetFaviconService()->UpdateFaviconMappingAndFetch(page_url, icon_url,
+          icon_type, &cancelable_consumer_,
           NewCallback(this, &FaviconHelper::OnFaviconData));
     }
   }
 }
 
-void FaviconHelper::OnFaviconData(
-    FaviconService::Handle handle,
-    history::FaviconData favicon) {
+void FaviconHelper::OnFaviconData(FaviconService::Handle handle,
+                                  history::FaviconData favicon) {
+  // We shouldn't be here if there is no candidate.
+  DCHECK(candidate());
   NavigationEntry* entry = GetEntry();
   if (!entry)
     return;
 
   // No need to update the favicon url. By the time we get here
   // UpdateFaviconURL will have set the favicon url.
-
-  if (favicon.is_valid()) {
+  if (favicon.is_valid() && favicon.icon_type == history::FAVICON) {
     // There is a favicon, set it now. If expired we'll download the current
     // one again, but at least the user will get some icon instead of the
     // default and most likely the current one is fine anyway.
     UpdateFavicon(entry, favicon.image_data);
   }
 
-  if (!favicon.known_icon || favicon.expired) {
-    // We don't know the favicon, or it is out of date. Request the current one.
-    ScheduleDownload(entry->url(), entry->favicon().url(), kFaviconSize, NULL);
+  if (!favicon.known_icon || favicon.expired ||
+      !IsSameFaviconURL(*candidate(), favicon.icon_url, favicon.icon_type)) {
+    // We don't know the favicon, it is out of date or its type is not same as
+    // one got from page. Request the current one.
+    ScheduleDownload(entry->url(), candidate()->icon_url, preferred_icon_size_,
+        static_cast<history::IconType>(candidate()->icon_type), NULL);
   }
+  history_icon_ = favicon;
 }
 
 int FaviconHelper::ScheduleDownload(const GURL& url,
                                     const GURL& image_url,
                                     int image_size,
+                                    history::IconType icon_type,
                                     ImageDownloadCallback* callback) {
   const int download_id = tab_contents()->render_view_host()->DownloadFavicon(
       image_url, image_size);
@@ -287,7 +302,8 @@ int FaviconHelper::ScheduleDownload(const GURL& url,
   if (download_id) {
     // Download ids should be unique.
     DCHECK(download_requests_.find(download_id) == download_requests_.end());
-    download_requests_[download_id] = DownloadRequest(url, image_url, callback);
+    download_requests_[download_id] =
+        DownloadRequest(url, image_url, callback, icon_type);
   }
 
   return download_id;
@@ -297,7 +313,7 @@ SkBitmap FaviconHelper::ConvertToFaviconSize(const SkBitmap& image) {
   int width = image.width();
   int height = image.height();
   if (width > 0 && height > 0) {
-    calc_favicon_target_size(&width, &height);
+    calc_favicon_target_size(preferred_icon_size_, &width, &height);
     return skia::ImageOperations::Resize(
           image, skia::ImageOperations::RESIZE_LANCZOS3,
           width, height);