Moves knowledge of Profile out of FaviconHandler, into FaviconTabHelper.

This CL introduces an interface that will be used to access all of chrome/ API.
It is implemented by the FaviconTabHelper, and allows moving the
GetFaviconService() method out of the FaviconHandler.

BUG=359095

Review URL: https://codereview.chromium.org/228783002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@262960 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/favicon/favicon_handler.cc

@@ -210,13 +210,16 @@ FaviconHandler::FaviconCandidate::FaviconCandidate(
 ////////////////////////////////////////////////////////////////////////////////
 
 FaviconHandler::FaviconHandler(Profile* profile,
+                               FaviconClient* client,
                                FaviconHandlerDelegate* delegate,
                                Type icon_type)
     : got_favicon_from_history_(false),
       favicon_expired_or_incomplete_(false),
-      icon_types_(icon_type == FAVICON ? chrome::FAVICON :
-          chrome::TOUCH_ICON | chrome::TOUCH_PRECOMPOSED_ICON),
+      icon_types_(icon_type == FAVICON
+                      ? chrome::FAVICON
+                      : chrome::TOUCH_ICON | chrome::TOUCH_PRECOMPOSED_ICON),
       profile_(profile),
+      client_(client),
       delegate_(delegate) {
   DCHECK(profile_);
   DCHECK(delegate_);
@@ -236,7 +239,7 @@ void FaviconHandler::FetchFavicon(const GURL& url) {
   // Request the favicon from the history service. In parallel to this the
   // renderer is going to notify us (well WebContents) when the favicon url is
   // available.
-  if (GetFaviconService()) {
+  if (client_->GetFaviconService()) {
     GetFaviconForURLFromFaviconService(
         url_,
         icon_types_,
@@ -247,11 +250,6 @@ void FaviconHandler::FetchFavicon(const GURL& url) {
   }
 }
 
-FaviconService* FaviconHandler::GetFaviconService() {
-  return FaviconServiceFactory::GetForProfile(
-      profile_, Profile::EXPLICIT_ACCESS);
-}
-
 bool FaviconHandler::UpdateFaviconCandidate(const GURL& url,
                                             const GURL& image_url,
                                             const gfx::Image& image,
@@ -272,7 +270,7 @@ void FaviconHandler::SetFavicon(
     const GURL& icon_url,
     const gfx::Image& image,
     chrome::IconType icon_type) {
-  if (GetFaviconService() && ShouldSaveFavicon(url))
+  if (client_->GetFaviconService() && ShouldSaveFavicon(url))
     SetHistoryFavicons(url, icon_url, icon_type, image);
 
   if (UrlMatches(url, url_) && icon_type == chrome::FAVICON) {
@@ -332,7 +330,7 @@ void FaviconHandler::OnUpdateFaviconURL(
   if (image_urls_.empty())
     return;
 
-  if (!GetFaviconService())
+  if (!client_->GetFaviconService())
     return;
 
   ProcessCurrentUrl();
@@ -439,7 +437,7 @@ void FaviconHandler::UpdateFaviconMappingAndFetch(
   // UpdateFaviconMappingsAndFetch().
   std::vector<GURL> icon_urls;
   icon_urls.push_back(icon_url);
-  GetFaviconService()->UpdateFaviconMappingsAndFetch(
+  client_->GetFaviconService()->UpdateFaviconMappingsAndFetch(
       page_url, icon_urls, icon_type, preferred_icon_size(), callback, tracker);
 }
 
@@ -448,7 +446,7 @@ void FaviconHandler::GetFaviconFromFaviconService(
     chrome::IconType icon_type,
     const FaviconService::FaviconResultsCallback& callback,
     base::CancelableTaskTracker* tracker) {
-  GetFaviconService()->GetFavicon(
+  client_->GetFaviconService()->GetFavicon(
       icon_url, icon_type, preferred_icon_size(), callback, tracker);
 }
 
@@ -457,9 +455,9 @@ void FaviconHandler::GetFaviconForURLFromFaviconService(
     int icon_types,
     const FaviconService::FaviconResultsCallback& callback,
     base::CancelableTaskTracker* tracker) {
-  GetFaviconService()->GetFaviconForURL(
-      FaviconService::FaviconForURLParams(page_url, icon_types,
-                                          preferred_icon_size()),
+  client_->GetFaviconService()->GetFaviconForURL(
+      FaviconService::FaviconForURLParams(
+          page_url, icon_types, preferred_icon_size()),
       callback,
       tracker);
 }
@@ -468,7 +466,8 @@ void FaviconHandler::SetHistoryFavicons(const GURL& page_url,
                                         const GURL& icon_url,
                                         chrome::IconType icon_type,
                                         const gfx::Image& image) {
-  GetFaviconService()->SetFavicons(page_url, icon_url, icon_type, image);
+  client_->GetFaviconService()->SetFavicons(
+      page_url, icon_url, icon_type, image);
 }
 
 bool FaviconHandler::ShouldSaveFavicon(const GURL& url) {
@@ -545,7 +544,7 @@ void FaviconHandler::DownloadFaviconOrAskFaviconService(
   if (favicon_expired_or_incomplete_) {
     // We have the mapping, but the favicon is out of date. Download it now.
     ScheduleDownload(page_url, icon_url, icon_type);
-  } else if (GetFaviconService()) {
+  } else if (client_->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.

chrome/browser/favicon/favicon_handler.h

@@ -19,6 +19,7 @@
 #include "ui/gfx/image/image.h"
 #include "url/gurl.h"
 
+class FaviconClient;
 class FaviconHandlerDelegate;
 class Profile;
 class SkBitmap;
@@ -84,6 +85,7 @@ class FaviconHandler {
   };
 
   FaviconHandler(Profile* profile,
+                 FaviconClient* client,
                  FaviconHandlerDelegate* delegate,
                  Type icon_type);
   virtual ~FaviconHandler();
@@ -152,8 +154,6 @@ class FaviconHandler {
                                   chrome::IconType icon_type,
                                   const gfx::Image& image);
 
-  virtual FaviconService* GetFaviconService();
-
   // Returns true if the favicon should be saved.
   virtual bool ShouldSaveFavicon(const GURL& url);
 
@@ -286,6 +286,9 @@ class FaviconHandler {
   // The Profile associated with this handler.
   Profile* profile_;
 
+  // The client which implements embedder-specific Favicon operations.
+  FaviconClient* client_;  // weak
+
   // This handler's delegate.
   FaviconHandlerDelegate* delegate_;  // weak
 

chrome/browser/favicon/favicon_handler_unittest.cc

@@ -167,6 +167,13 @@ class HistoryRequestHandler {
 
 }  // namespace
 
+class TestFaviconClient : public FaviconClient {
+ public:
+  virtual FaviconService* GetFaviconService() OVERRIDE {
+    // Just give none NULL value, so overridden methods can be hit.
+    return (FaviconService*)(1);
+  }
+};
 
 class TestFaviconHandlerDelegate : public FaviconHandlerDelegate {
  public:
@@ -207,9 +214,10 @@ class TestFaviconHandler : public FaviconHandler {
  public:
   TestFaviconHandler(const GURL& page_url,
                      Profile* profile,
+                     FaviconClient* client,
                      FaviconHandlerDelegate* delegate,
                      Type type)
-      : FaviconHandler(profile, delegate, type),
+      : FaviconHandler(profile, client, delegate, type),
         entry_(NavigationEntry::Create()),
         download_id_(0),
         num_favicon_updates_(0) {
@@ -302,11 +310,6 @@ class TestFaviconHandler : public FaviconHandler {
         page_url, icon_url, icon_type, bitmap_data));
   }
 
-  virtual FaviconService* GetFaviconService() OVERRIDE {
-    // Just give none NULL value, so overridden methods can be hit.
-    return (FaviconService*)(1);
-  }
-
   virtual bool ShouldSaveFavicon(const GURL& url) OVERRIDE {
     return true;
   }
@@ -439,10 +442,11 @@ TEST_F(FaviconHandlerTest, GetFaviconFromHistory) {
   const GURL icon_url("http://www.google.com/favicon");
 
   TestFaviconHandlerDelegate delegate;
+  TestFaviconClient client;
   Profile* profile = Profile::FromBrowserContext(
       web_contents()->GetBrowserContext());
-  TestFaviconHandler helper(page_url, profile,
-                            &delegate, FaviconHandler::FAVICON);
+  TestFaviconHandler helper(
+      page_url, profile, &client, &delegate, FaviconHandler::FAVICON);
 
   helper.FetchFavicon(page_url);
   HistoryRequestHandler* history_handler = helper.history_handler();
@@ -481,10 +485,11 @@ TEST_F(FaviconHandlerTest, DownloadFavicon) {
   const GURL icon_url("http://www.google.com/favicon");
 
   TestFaviconHandlerDelegate delegate;
+  TestFaviconClient client;
   Profile* profile = Profile::FromBrowserContext(
       web_contents()->GetBrowserContext());
-  TestFaviconHandler helper(page_url, profile,
-                            &delegate, FaviconHandler::FAVICON);
+  TestFaviconHandler helper(
+      page_url, profile, &client, &delegate, FaviconHandler::FAVICON);
 
   helper.FetchFavicon(page_url);
   HistoryRequestHandler* history_handler = helper.history_handler();
@@ -550,10 +555,11 @@ TEST_F(FaviconHandlerTest, UpdateAndDownloadFavicon) {
   const GURL new_icon_url("http://www.google.com/new_favicon");
 
   TestFaviconHandlerDelegate delegate;
+  TestFaviconClient client;
   Profile* profile = Profile::FromBrowserContext(
       web_contents()->GetBrowserContext());
-  TestFaviconHandler helper(page_url, profile,
-                            &delegate, FaviconHandler::FAVICON);
+  TestFaviconHandler helper(
+      page_url, profile, &client, &delegate, FaviconHandler::FAVICON);
 
   helper.FetchFavicon(page_url);
   HistoryRequestHandler* history_handler = helper.history_handler();
@@ -633,10 +639,11 @@ TEST_F(FaviconHandlerTest, FaviconInHistoryInvalid) {
   const GURL icon_url("http://www.google.com/favicon");
 
   TestFaviconHandlerDelegate delegate;
+  TestFaviconClient client;
   Profile* profile = Profile::FromBrowserContext(
       web_contents()->GetBrowserContext());
-  TestFaviconHandler helper(page_url, profile,
-                            &delegate, FaviconHandler::FAVICON);
+  TestFaviconHandler helper(
+      page_url, profile, &client, &delegate, FaviconHandler::FAVICON);
 
   helper.FetchFavicon(page_url);
   HistoryRequestHandler* history_handler = helper.history_handler();
@@ -707,10 +714,11 @@ TEST_F(FaviconHandlerTest, UpdateFavicon) {
   const GURL new_icon_url("http://www.google.com/new_favicon");
 
   TestFaviconHandlerDelegate delegate;
+  TestFaviconClient client;
   Profile* profile = Profile::FromBrowserContext(
       web_contents()->GetBrowserContext());
-  TestFaviconHandler helper(page_url, profile,
-                            &delegate, FaviconHandler::FAVICON);
+  TestFaviconHandler helper(
+      page_url, profile, &client, &delegate, FaviconHandler::FAVICON);
 
   helper.FetchFavicon(page_url);
   HistoryRequestHandler* history_handler = helper.history_handler();
@@ -770,10 +778,11 @@ TEST_F(FaviconHandlerTest, Download2ndFaviconURLCandidate) {
   const GURL new_icon_url("http://www.google.com/new_favicon");
 
   TestFaviconHandlerDelegate delegate;
+  TestFaviconClient client;
   Profile* profile = Profile::FromBrowserContext(
       web_contents()->GetBrowserContext());
-  TestFaviconHandler helper(page_url, profile,
-                            &delegate, FaviconHandler::TOUCH);
+  TestFaviconHandler helper(
+      page_url, profile, &client, &delegate, FaviconHandler::TOUCH);
 
   helper.FetchFavicon(page_url);
   HistoryRequestHandler* history_handler = helper.history_handler();
@@ -883,10 +892,11 @@ TEST_F(FaviconHandlerTest, UpdateDuringDownloading) {
   const GURL new_icon_url("http://www.google.com/new_favicon");
 
   TestFaviconHandlerDelegate delegate;
+  TestFaviconClient client;
   Profile* profile = Profile::FromBrowserContext(
       web_contents()->GetBrowserContext());
-  TestFaviconHandler helper(page_url, profile,
-                            &delegate, FaviconHandler::TOUCH);
+  TestFaviconHandler helper(
+      page_url, profile, &client, &delegate, FaviconHandler::TOUCH);
 
   helper.FetchFavicon(page_url);
   HistoryRequestHandler* history_handler = helper.history_handler();
@@ -1027,8 +1037,9 @@ TEST_F(FaviconHandlerTest, MultipleFavicons) {
   // 1) Test that if there are several single resolution favicons to choose from
   // that the largest exact match is chosen.
   TestFaviconHandlerDelegate delegate1;
-  TestFaviconHandler handler1(kPageURL, profile,
-                              &delegate1, FaviconHandler::FAVICON);
+  TestFaviconClient client;
+  TestFaviconHandler handler1(
+      kPageURL, profile, &client, &delegate1, FaviconHandler::FAVICON);
   const int kSizes1[] = { 16, 24, 32, 48, 256 };
   std::vector<FaviconURL> urls1(kSourceIconURLs,
                                 kSourceIconURLs + arraysize(kSizes1));
@@ -1048,8 +1059,8 @@ TEST_F(FaviconHandlerTest, MultipleFavicons) {
   // 2) Test that if there are several single resolution favicons to choose
   // from, the exact match is preferred even if it results in upsampling.
   TestFaviconHandlerDelegate delegate2;
-  TestFaviconHandler handler2(kPageURL, profile,
-                              &delegate2, FaviconHandler::FAVICON);
+  TestFaviconHandler handler2(
+      kPageURL, profile, &client, &delegate2, FaviconHandler::FAVICON);
   const int kSizes2[] = { 16, 24, 48, 256 };
   std::vector<FaviconURL> urls2(kSourceIconURLs,
                                 kSourceIconURLs + arraysize(kSizes2));
@@ -1063,8 +1074,8 @@ TEST_F(FaviconHandlerTest, MultipleFavicons) {
   // 3) Test that favicons which need to be upsampled a little or downsampled
   // a little are preferred over huge favicons.
   TestFaviconHandlerDelegate delegate3;
-  TestFaviconHandler handler3(kPageURL, profile,
-                              &delegate3, FaviconHandler::FAVICON);
+  TestFaviconHandler handler3(
+      kPageURL, profile, &client, &delegate3, FaviconHandler::FAVICON);
   const int kSizes3[] = { 256, 48 };
   std::vector<FaviconURL> urls3(kSourceIconURLs,
                                 kSourceIconURLs + arraysize(kSizes3));
@@ -1076,8 +1087,8 @@ TEST_F(FaviconHandlerTest, MultipleFavicons) {
             handler3.GetEntry()->GetFavicon().url);
 
   TestFaviconHandlerDelegate delegate4;
-  TestFaviconHandler handler4(kPageURL, profile,
-                              &delegate4, FaviconHandler::FAVICON);
+  TestFaviconHandler handler4(
+      kPageURL, profile, &client, &delegate4, FaviconHandler::FAVICON);
   const int kSizes4[] = { 17, 256 };
   std::vector<FaviconURL> urls4(kSourceIconURLs,
                                 kSourceIconURLs + arraysize(kSizes4));

chrome/browser/favicon/favicon_tab_helper.cc

@@ -38,11 +38,11 @@ DEFINE_WEB_CONTENTS_USER_DATA_KEY(FaviconTabHelper);
 FaviconTabHelper::FaviconTabHelper(WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
       profile_(Profile::FromBrowserContext(web_contents->GetBrowserContext())) {
-  favicon_handler_.reset(new FaviconHandler(profile_, this,
-                                            FaviconHandler::FAVICON));
+  favicon_handler_.reset(
+      new FaviconHandler(profile_, this, this, FaviconHandler::FAVICON));
   if (chrome::kEnableTouchIcon)
-    touch_icon_handler_.reset(new FaviconHandler(profile_, this,
-                                                 FaviconHandler::TOUCH));
+    touch_icon_handler_.reset(
+        new FaviconHandler(profile_, this, this, FaviconHandler::TOUCH));
 }
 
 FaviconTabHelper::~FaviconTabHelper() {
@@ -192,6 +192,11 @@ void FaviconTabHelper::DidUpdateFaviconURL(
     touch_icon_handler_->OnUpdateFaviconURL(page_id, candidates);
 }
 
+FaviconService* FaviconTabHelper::GetFaviconService() {
+  return FaviconServiceFactory::GetForProfile(profile_,
+                                              Profile::EXPLICIT_ACCESS);
+}
+
 void FaviconTabHelper::DidDownloadFavicon(
     int id,
     int http_status_code,

chrome/browser/favicon/favicon_tab_helper.h

@@ -9,6 +9,7 @@
 
 #include "base/basictypes.h"
 #include "base/callback.h"
+#include "components/favicon/core/browser/favicon_client.h"
 #include "components/favicon/core/favicon_handler_delegate.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
@@ -30,6 +31,7 @@ class SkBitmap;
 // downloaded and saved in the history backend.
 //
 class FaviconTabHelper : public content::WebContentsObserver,
+                         public FaviconClient,
                          public FaviconHandlerDelegate,
                          public content::WebContentsUserData<FaviconTabHelper> {
  public:
@@ -80,6 +82,9 @@ class FaviconTabHelper : public content::WebContentsObserver,
       const std::vector<SkBitmap>& bitmaps,
       const std::vector<gfx::Size>& original_bitmap_sizes);
 
+  // FaviconClient implementation:
+  virtual FaviconService* GetFaviconService() OVERRIDE;
+
  private:
   explicit FaviconTabHelper(content::WebContents* web_contents);
   friend class content::WebContentsUserData<FaviconTabHelper>;

components/favicon.gypi

@@ -8,6 +8,7 @@
       'target_name': 'favicon_core',
       'type': 'none',
       'sources': [
+        'favicon/core/browser/favicon_client.h',
         'favicon/core/favicon_handler_delegate.h',
       ],
     },

components/favicon/core/browser/favicon_client.h

+// Copyright 2014 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.
+
+#ifndef COMPONENTS_FAVICON_CORE_BROWSER_FAVICON_CLIENT_H_
+#define COMPONENTS_FAVICON_CORE_BROWSER_FAVICON_CLIENT_H_
+
+class FaviconService;
+
+// This class abstracts operations that depend on the embedder's environment,
+// e.g.
+// Chrome.
+class FaviconClient {
+ public:
+  virtual FaviconService* GetFaviconService() = 0;
+};
+
+#endif  // COMPONENTS_FAVICON_CORE_BROWSER_FAVICON_CLIENT_H_