Introduce FaviconServiceImpl::AddPageNoVisitForBookmark()

This CL introduces a new API
FaviconServiceImpl::AddPageNoVisitForBookmark()

Similar to other APIs in FaviconService, it acts as a proxy to the
underlying HistoryService.

This comes with the following benefits:

1- Reduces number of dependencies for the calling sites. They now
depend only on FaviconService.

2- There exist a mock for FaviconServiceImpl which isn't the case for
HistoryService. Tests inject null HistoryService, but instead they
could directly use the MockFaviconService and no need to checks
for null history service in production code.

Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I641bf257c1e24c4b3663bcd3cf7210e8d1363002
Reviewed-on: https://chromium-review.googlesource.com/1161932
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580773}

chrome/browser/favicon/favicon_utils.cc

@@ -5,7 +5,6 @@
 #include "chrome/browser/favicon/favicon_utils.h"
 
 #include "chrome/browser/favicon/favicon_service_factory.h"
-#include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search/search.h"
 #include "chrome/common/url_constants.h"
@@ -39,8 +38,6 @@ void CreateContentFaviconDriverForWebContents(
   return ContentFaviconDriver::CreateForWebContents(
       web_contents,
       FaviconServiceFactory::GetForProfile(original_profile,
-                                           ServiceAccessType::IMPLICIT_ACCESS),
-      HistoryServiceFactory::GetForProfile(original_profile,
                                            ServiceAccessType::IMPLICIT_ACCESS));
 }
 

components/favicon/content/content_favicon_driver.cc

@@ -9,7 +9,6 @@
 #include "components/favicon/content/favicon_url_util.h"
 #include "components/favicon/core/favicon_service.h"
 #include "components/favicon/core/favicon_url.h"
-#include "components/history/core/browser/history_service.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/favicon_status.h"
 #include "content/public/browser/navigation_controller.h"
@@ -40,14 +39,13 @@ void ExtractManifestIcons(
 // static
 void ContentFaviconDriver::CreateForWebContents(
     content::WebContents* web_contents,
-    FaviconService* favicon_service,
-    history::HistoryService* history_service) {
+    FaviconService* favicon_service) {
   if (FromWebContents(web_contents))
     return;
 
-  web_contents->SetUserData(
-      UserDataKey(), base::WrapUnique(new ContentFaviconDriver(
-                         web_contents, favicon_service, history_service)));
+  web_contents->SetUserData(UserDataKey(),
+                            base::WrapUnique(new ContentFaviconDriver(
+                                web_contents, favicon_service)));
 }
 
 void ContentFaviconDriver::SaveFaviconEvenIfInIncognito() {
@@ -58,10 +56,8 @@ void ContentFaviconDriver::SaveFaviconEvenIfInIncognito() {
 
   // Make sure the page is in history, otherwise adding the favicon does
   // nothing.
-  if (!history_service())
-    return;
   GURL page_url = entry->GetURL();
-  history_service()->AddPageNoVisitForBookmark(page_url, entry->GetTitle());
+  favicon_service()->AddPageNoVisitForBookmark(page_url, entry->GetTitle());
 
   const content::FaviconStatus& favicon_status = entry->GetFavicon();
   if (!favicon_service() || !favicon_status.valid ||
@@ -109,12 +105,10 @@ GURL ContentFaviconDriver::GetActiveURL() {
   return entry ? entry->GetURL() : GURL();
 }
 
-ContentFaviconDriver::ContentFaviconDriver(
-    content::WebContents* web_contents,
-    FaviconService* favicon_service,
-    history::HistoryService* history_service)
+ContentFaviconDriver::ContentFaviconDriver(content::WebContents* web_contents,
+                                           FaviconService* favicon_service)
     : content::WebContentsObserver(web_contents),
-      FaviconDriverImpl(favicon_service, history_service),
+      FaviconDriverImpl(favicon_service),
       document_on_load_completed_(false) {}
 
 ContentFaviconDriver::~ContentFaviconDriver() {

components/favicon/content/content_favicon_driver.h

@@ -34,8 +34,7 @@ class ContentFaviconDriver
   ~ContentFaviconDriver() override;
 
   static void CreateForWebContents(content::WebContents* web_contents,
-                                   FaviconService* favicon_service,
-                                   history::HistoryService* history_service);
+                                   FaviconService* favicon_service);
 
   // Returns the current tab's favicon URLs. If this is empty,
   // DidUpdateFaviconURL has not yet been called for the current navigation.
@@ -54,8 +53,7 @@ class ContentFaviconDriver
 
  protected:
   ContentFaviconDriver(content::WebContents* web_contents,
-                       FaviconService* favicon_service,
-                       history::HistoryService* history_service);
+                       FaviconService* favicon_service);
 
  private:
   friend class content::WebContentsUserData<ContentFaviconDriver>;

components/favicon/content/content_favicon_driver_unittest.cc

@@ -52,7 +52,7 @@ class ContentFaviconDriverTest : public content::RenderViewHostTestHarness {
     RenderViewHostTestHarness::SetUp();
 
     ContentFaviconDriver::CreateForWebContents(web_contents(),
-                                               &favicon_service_, nullptr);
+                                               &favicon_service_);
   }
 
   content::WebContentsTester* web_contents_tester() {

components/favicon/core/favicon_driver_impl.cc

@@ -14,7 +14,6 @@
 #include "components/favicon/core/favicon_handler.h"
 #include "components/favicon/core/favicon_service.h"
 #include "components/favicon/core/favicon_url.h"
-#include "components/history/core/browser/history_service.h"
 
 namespace favicon {
 namespace {
@@ -27,9 +26,8 @@ const bool kEnableTouchIcon = false;
 
 }  // namespace
 
-FaviconDriverImpl::FaviconDriverImpl(FaviconService* favicon_service,
-                                     history::HistoryService* history_service)
-    : favicon_service_(favicon_service), history_service_(history_service) {
+FaviconDriverImpl::FaviconDriverImpl(FaviconService* favicon_service)
+    : favicon_service_(favicon_service) {
   if (!favicon_service_)
     return;
 

components/favicon/core/favicon_driver_impl.h

@@ -14,10 +14,6 @@
 
 class GURL;
 
-namespace history {
-class HistoryService;
-}
-
 namespace favicon {
 
 class FaviconService;
@@ -41,8 +37,7 @@ class FaviconDriverImpl : public FaviconDriver,
   bool HasPendingTasksForTest();
 
  protected:
-  FaviconDriverImpl(FaviconService* favicon_service,
-                    history::HistoryService* history_service);
+  explicit FaviconDriverImpl(FaviconService* favicon_service);
   ~FaviconDriverImpl() override;
 
   // Informs FaviconService that the favicon for |url| is out of date. If
@@ -56,15 +51,12 @@ class FaviconDriverImpl : public FaviconDriver,
                           const GURL& manifest_url);
 
  protected:
-  history::HistoryService* history_service() { return history_service_; }
-
   FaviconService* favicon_service() { return favicon_service_; }
 
  private:
-  // KeyedServices used by FaviconDriverImpl. They may be null during testing,
-  // but if they are defined, they must outlive the FaviconDriverImpl.
+  // KeyedService used by FaviconDriverImpl. It may be null during testing,
+  // but if it is defined, it must outlive the FaviconDriverImpl.
   FaviconService* favicon_service_;
-  history::HistoryService* history_service_;
 
   // FaviconHandlers used to download the different kind of favicons.
   std::vector<std::unique_ptr<FaviconHandler>> handlers_;

components/favicon/core/favicon_service.h

@@ -165,6 +165,11 @@ class FaviconService : public KeyedService {
   virtual void SetImportedFavicons(
       const favicon_base::FaviconUsageDataList& favicon_usage) = 0;
 
+  // See HistoryService::AddPageNoVisitForBookmark(). Adds an entry for the
+  // specified url in the history service without creating a visit.
+  virtual void AddPageNoVisitForBookmark(const GURL& url,
+                                         const base::string16& title) = 0;
+
   // Set the favicon for |page_url| for |icon_type| in the thumbnail database.
   // Unlike SetFavicons(), this method will not delete preexisting bitmap data
   // which is associated to |page_url| if at all possible. Use this method if

components/favicon/core/favicon_service_impl.cc

@@ -224,6 +224,12 @@ void FaviconServiceImpl::SetImportedFavicons(
   history_service_->SetImportedFavicons(favicon_usage);
 }
 
+void FaviconServiceImpl::AddPageNoVisitForBookmark(
+    const GURL& url,
+    const base::string16& title) {
+  history_service_->AddPageNoVisitForBookmark(url, title);
+}
+
 void FaviconServiceImpl::MergeFavicon(
     const GURL& page_url,
     const GURL& icon_url,

components/favicon/core/favicon_service_impl.h

@@ -98,6 +98,8 @@ class FaviconServiceImpl : public FaviconService {
   void TouchOnDemandFavicon(const GURL& icon_url) override;
   void SetImportedFavicons(
       const favicon_base::FaviconUsageDataList& favicon_usage) override;
+  void AddPageNoVisitForBookmark(const GURL& url,
+                                 const base::string16& title) override;
   void MergeFavicon(const GURL& page_url,
                     const GURL& icon_url,
                     favicon_base::IconType icon_type,

components/favicon/core/test/mock_favicon_service.h

@@ -97,6 +97,8 @@ class MockFaviconService : public FaviconService {
   MOCK_METHOD1(TouchOnDemandFavicon, void(const GURL& icon_url));
   MOCK_METHOD1(SetImportedFavicons,
                void(const favicon_base::FaviconUsageDataList& favicon_usage));
+  MOCK_METHOD2(AddPageNoVisitForBookmark,
+               void(const GURL& url, const base::string16& title));
   MOCK_METHOD5(MergeFavicon,
                void(const GURL& page_url,
                     const GURL& icon_url,

components/favicon/ios/web_favicon_driver.h

@@ -27,8 +27,7 @@ class WebFaviconDriver : public web::WebStateObserver,
   ~WebFaviconDriver() override;
 
   static void CreateForWebState(web::WebState* web_state,
-                                FaviconService* favicon_service,
-                                history::HistoryService* history_service);
+                                FaviconService* favicon_service);
 
   // FaviconDriver implementation.
   gfx::Image GetFavicon() const override;
@@ -55,9 +54,7 @@ class WebFaviconDriver : public web::WebStateObserver,
  private:
   friend class web::WebStateUserData<WebFaviconDriver>;
 
-  WebFaviconDriver(web::WebState* web_state,
-                   FaviconService* favicon_service,
-                   history::HistoryService* history_service);
+  WebFaviconDriver(web::WebState* web_state, FaviconService* favicon_service);
 
   // web::WebStateObserver implementation.
   void DidFinishNavigation(web::WebState* web_state,

components/favicon/ios/web_favicon_driver.mm

@@ -37,16 +37,13 @@ using ImageDownloadCallback =
 namespace favicon {
 
 // static
-void WebFaviconDriver::CreateForWebState(
-    web::WebState* web_state,
-    FaviconService* favicon_service,
-    history::HistoryService* history_service) {
+void WebFaviconDriver::CreateForWebState(web::WebState* web_state,
+                                         FaviconService* favicon_service) {
   if (FromWebState(web_state))
     return;
 
-  web_state->SetUserData(UserDataKey(),
-                         base::WrapUnique(new WebFaviconDriver(
-                             web_state, favicon_service, history_service)));
+  web_state->SetUserData(UserDataKey(), base::WrapUnique(new WebFaviconDriver(
+                                            web_state, favicon_service)));
 }
 
 gfx::Image WebFaviconDriver::GetFavicon() const {
@@ -152,9 +149,8 @@ void WebFaviconDriver::OnFaviconDeleted(
 }
 
 WebFaviconDriver::WebFaviconDriver(web::WebState* web_state,
-                                   FaviconService* favicon_service,
-                                   history::HistoryService* history_service)
-    : FaviconDriverImpl(favicon_service, history_service),
+                                   FaviconService* favicon_service)
+    : FaviconDriverImpl(favicon_service),
       image_fetcher_(web_state->GetBrowserState()->GetSharedURLLoaderFactory()),
       web_state_(web_state) {
   web_state_->AddObserver(this);

components/sync_bookmarks/bookmark_change_processor.cc

@@ -20,7 +20,6 @@
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/bookmarks/browser/bookmark_utils.h"
 #include "components/favicon/core/favicon_service.h"
-#include "components/history/core/browser/history_service.h"
 #include "components/sync/driver/sync_client.h"
 #include "components/sync/syncable/change_record.h"
 #include "components/sync/syncable/entry.h"  // TODO(tim): Investigating bug 121587.
@@ -929,16 +928,14 @@ void BookmarkChangeProcessor::ApplyBookmarkFavicon(
     syncer::SyncClient* sync_client,
     const GURL& icon_url,
     const scoped_refptr<base::RefCountedMemory>& bitmap_data) {
-  history::HistoryService* history = sync_client->GetHistoryService();
   favicon::FaviconService* favicon_service = sync_client->GetFaviconService();
 
   // Some tests (that use FakeSyncClient) use no services.
-  if (history == nullptr)
+  if (favicon_service == nullptr)
     return;
 
-  DCHECK(favicon_service);
-  history->AddPageNoVisitForBookmark(bookmark_node->url(),
-                                     bookmark_node->GetTitle());
+  favicon_service->AddPageNoVisitForBookmark(bookmark_node->url(),
+                                             bookmark_node->GetTitle());
 
   GURL icon_url_to_use = icon_url;
 

ios/chrome/browser/reading_list/favicon_web_state_dispatcher_impl.mm

@@ -8,7 +8,6 @@
 #include "components/keyed_service/core/service_access_type.h"
 #include "ios/chrome/browser/browser_state/chrome_browser_state.h"
 #include "ios/chrome/browser/favicon/favicon_service_factory.h"
-#include "ios/chrome/browser/history/history_service_factory.h"
 #import "ios/web/public/web_state/web_state.h"
 
 #if !defined(__has_feature) || !__has_feature(objc_arc)
@@ -47,8 +46,6 @@ FaviconWebStateDispatcherImpl::RequestWebState() {
   favicon::WebFaviconDriver::CreateForWebState(
       web_state.get(),
       ios::FaviconServiceFactory::GetForBrowserState(
-          original_browser_state, ServiceAccessType::EXPLICIT_ACCESS),
-      ios::HistoryServiceFactory::GetForBrowserState(
           original_browser_state, ServiceAccessType::EXPLICIT_ACCESS));
 
   return web_state;

ios/chrome/browser/tabs/tab_helper_util.mm

@@ -104,8 +104,6 @@ void AttachTabHelpers(web::WebState* web_state, bool for_prerender) {
   favicon::WebFaviconDriver::CreateForWebState(
       web_state,
       ios::FaviconServiceFactory::GetForBrowserState(
-          original_browser_state, ServiceAccessType::IMPLICIT_ACCESS),
-      ios::HistoryServiceFactory::GetForBrowserState(
           original_browser_state, ServiceAccessType::IMPLICIT_ACCESS));
   history::WebStateTopSitesObserver::CreateForWebState(
       web_state,

ios/chrome/browser/tabs/tab_model_favicon_driver_observer_unittest.mm

@@ -111,8 +111,7 @@ TabModelFaviconDriverObserverTest::CreateAndInsertTab() {
   TabIdTabHelper::CreateForWebState(web_state.get());
   LegacyTabHelper::CreateForWebState(web_state.get());
   favicon::WebFaviconDriver::CreateForWebState(web_state.get(),
-                                               /*favicon_service=*/nullptr,
-                                               /*history_service=*/nullptr);
+                                               /*favicon_service=*/nullptr);
 
   favicon::FaviconDriver* favicon_driver =
       favicon::WebFaviconDriver::FromWebState(web_state.get());