[Icons NTP] Enable Large Icon URL storage and image fetching (Touch Icons only), behind flag.

chrome/browser/favicon/favicon_tab_helper.cc

Index: chrome/browser/favicon/favicon_tab_helper.cc
diff --git a/chrome/browser/favicon/favicon_tab_helper.cc b/chrome/browser/favicon/favicon_tab_helper.cc
index d95ffd1d23bb21c3809aa1aaef2b86fb6054b8b1..a6a2d743d0765f4940c659284a03f81c2343f1c9 100644
--- a/chrome/browser/favicon/favicon_tab_helper.cc
+++ b/chrome/browser/favicon/favicon_tab_helper.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/favicon/favicon_tab_helper.h"
 
+#include "base/metrics/field_trial.h"
+#include "base/strings/string_util.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/favicon/chrome_favicon_client.h"
 #include "chrome/browser/favicon/chrome_favicon_client_factory.h"
@@ -40,6 +42,16 @@ using content::WebContents;
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(FaviconTabHelper);
 
+namespace {
+
+// Returns whether icon NTP is enabled.
+bool IsIconNTPEnabled() {

[sky, 2015/05/06 19:29:05]

Will this ever be used on android?

[huangs, 2015/05/06 19:59:14]

Currently it isn't, and likely Android will do things differently but we have to
verify that.

Should we wrap this under !defined(OS_ANDROID) then, and do something similar to
download_largest_icon in line 59?

+  return StartsWithASCII(base::FieldTrialList::FindFullName("IconNTP"),
+                         "Enabled", true);
+}
+
+}  // namespace
+
 FaviconTabHelper::FaviconTabHelper(WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
       profile_(Profile::FromBrowserContext(web_contents->GetBrowserContext())) {
@@ -56,6 +68,9 @@ FaviconTabHelper::FaviconTabHelper(WebContents* web_contents)
   if (chrome::kEnableTouchIcon)
     touch_icon_handler_.reset(new FaviconHandler(
         service, client_, this, FaviconHandler::TOUCH, download_largest_icon));
+  if (IsIconNTPEnabled())

[sky, 2015/05/06 20:13:53]

My concern is more of here. If IsIconNTPEnabled is true on android it would mean
downloading icons twice. I think this should be an else (with a comment).
Otherwise we may end up downloading the same set of icons twice.

[huangs, 2015/05/06 20:33:55]

rogerm@ has considered this, and has a CL to make this cleaner:
https://crrev.com/1118073002/

+    large_icon_handler_.reset(new FaviconHandler(
+        service, client_, this, FaviconHandler::LARGE, true));
 }
 
 FaviconTabHelper::~FaviconTabHelper() {
@@ -65,6 +80,8 @@ void FaviconTabHelper::FetchFavicon(const GURL& url) {
   favicon_handler_->FetchFavicon(url);
   if (touch_icon_handler_.get())
     touch_icon_handler_->FetchFavicon(url);
+  if (large_icon_handler_.get())
+    large_icon_handler_->FetchFavicon(url);
 }
 
 gfx::Image FaviconTabHelper::GetFavicon() const {
@@ -300,6 +317,8 @@ void FaviconTabHelper::DidUpdateFaviconURL(
   favicon_handler_->OnUpdateFaviconURL(favicon_urls);
   if (touch_icon_handler_.get())
     touch_icon_handler_->OnUpdateFaviconURL(favicon_urls);
+  if (large_icon_handler_.get())
+    large_icon_handler_->OnUpdateFaviconURL(favicon_urls);
 }
 
 void FaviconTabHelper::DidDownloadFavicon(
@@ -323,4 +342,8 @@ void FaviconTabHelper::DidDownloadFavicon(
     touch_icon_handler_->OnDidDownloadFavicon(
         id, image_url, bitmaps, original_bitmap_sizes);
   }
+  if (large_icon_handler_.get()) {
+    large_icon_handler_->OnDidDownloadFavicon(
+        id, image_url, bitmaps, original_bitmap_sizes);
+  }
 }