favicons: converts from delegate to return values

FaviconBackend was using a mix of delegate and return values to
convey results. This was mildly confusing. This patch converts to
a return value. The one remaining delegate function is for redirects,
which makes sense as a delegate function.

BUG=1076463
TEST=covered by tests

Change-Id: I9bc84e9afcb02a417b62180cae5fe5a89a09ef28
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2357851
Commit-Queue: Scott Violet <sky@chromium.org>
Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#798668}

components/favicon/core/favicon_backend.cc

@@ -204,47 +204,45 @@ FaviconBackend::GetFaviconForId(favicon_base::FaviconID favicon_id,
   return bitmap_results;
 }
 
-std::vector<favicon_base::FaviconRawBitmapResult>
-FaviconBackend::UpdateFaviconMappingsAndFetch(
+UpdateFaviconMappingsResult FaviconBackend::UpdateFaviconMappingsAndFetch(
     const base::flat_set<GURL>& page_urls,
     const GURL& icon_url,
     favicon_base::IconType icon_type,
     const std::vector<int>& desired_sizes) {
+  UpdateFaviconMappingsResult result;
   const favicon_base::FaviconID favicon_id =
       db_->GetFaviconIDForFaviconURL(icon_url, icon_type);
   if (!favicon_id)
-    return {};
+    return result;
 
   for (const GURL& page_url : page_urls) {
     bool mappings_updated =
         SetFaviconMappingsForPageAndRedirects(page_url, icon_type, favicon_id);
-    if (mappings_updated) {
-      delegate_->OnFaviconChangedForPageAndRedirects(page_url);
-      delegate_->ScheduleCommitForFavicons();
-    }
+    if (mappings_updated)
+      result.updated_page_urls.insert(page_url);
   }
 
-  return GetFaviconBitmapResultsForBestMatch({favicon_id}, desired_sizes);
+  result.bitmap_results =
+      GetFaviconBitmapResultsForBestMatch({favicon_id}, desired_sizes);
+  return result;
 }
 
-void FaviconBackend::DeleteFaviconMappings(
+base::flat_set<GURL> FaviconBackend::DeleteFaviconMappings(
     const base::flat_set<GURL>& page_urls,
     favicon_base::IconType icon_type) {
   TRACE_EVENT0("browser", "FaviconBackend::DeleteFaviconMappings");
 
+  base::flat_set<GURL> changed;
   for (const GURL& page_url : page_urls) {
     bool mapping_changed = SetFaviconMappingsForPageAndRedirects(
         page_url, icon_type, /*icon_id=*/0);
-
-    if (mapping_changed) {
-      // Notify the UI that this function changed an icon mapping.
-      delegate_->OnFaviconChangedForPageAndRedirects(page_url);
-      delegate_->ScheduleCommitForFavicons();
-    }
+    if (mapping_changed)
+      changed.insert(page_url);
   }
+  return changed;
 }
 
-bool FaviconBackend::MergeFavicon(
+MergeFaviconResult FaviconBackend::MergeFavicon(
     const GURL& page_url,
     const GURL& icon_url,
     favicon_base::IconType icon_type,
@@ -382,15 +380,15 @@ bool FaviconBackend::MergeFavicon(
     }
   }
 
+  MergeFaviconResult result;
   // Update the favicon mappings such that only |icon_url| is mapped to
   // |page_url|.
   if (icon_mappings.size() != 1 || icon_mappings[0].icon_url != icon_url) {
     SetFaviconMappingsForPageAndRedirects(page_url, icon_type, favicon_id);
-    delegate_->OnFaviconChangedForPageAndRedirects(page_url);
+    result.did_page_to_icon_mapping_change = true;
   }
-
-  delegate_->ScheduleCommitForFavicons();
-  return !bitmap_identical || favicon_bitmaps_copied;
+  result.did_icon_change = !bitmap_identical || favicon_bitmaps_copied;
+  return result;
 }
 
 std::set<GURL> FaviconBackend::CloneFaviconMappingsForPages(
@@ -430,9 +428,6 @@ std::set<GURL> FaviconBackend::CloneFaviconMappingsForPages(
     changed_page_urls.insert(std::make_move_iterator(v.begin()),
                              std::make_move_iterator(v.end()));
   }
-
-  if (!changed_page_urls.empty())
-    delegate_->ScheduleCommitForFavicons();
   return changed_page_urls;
 }
 
@@ -459,40 +454,42 @@ bool FaviconBackend::CanSetOnDemandFavicons(const GURL& page_url,
   return true;
 }
 
-bool FaviconBackend::SetOnDemandFavicons(const GURL& page_url,
-                                         favicon_base::IconType icon_type,
-                                         const GURL& icon_url,
-                                         const std::vector<SkBitmap>& bitmaps) {
-  return CanSetOnDemandFavicons(page_url, icon_type) &&
-         SetFavicons({page_url}, icon_type, icon_url, bitmaps,
+SetFaviconsResult FaviconBackend::SetOnDemandFavicons(
+    const GURL& page_url,
+    favicon_base::IconType icon_type,
+    const GURL& icon_url,
+    const std::vector<SkBitmap>& bitmaps) {
+  if (!CanSetOnDemandFavicons(page_url, icon_type))
+    return {};
+  return SetFavicons({page_url}, icon_type, icon_url, bitmaps,
                      FaviconBitmapType::ON_DEMAND);
 }
 
-void FaviconBackend::SetFaviconsOutOfDateForPage(const GURL& page_url) {
+bool FaviconBackend::SetFaviconsOutOfDateForPage(const GURL& page_url) {
   TRACE_EVENT0("browser", "FaviconBackend::SetFaviconsOutOfDateForPage");
 
   std::vector<IconMapping> icon_mappings;
 
   if (!db_->GetIconMappingsForPageURL(page_url, &icon_mappings))
-    return;
+    return false;
 
   for (auto m = icon_mappings.begin(); m != icon_mappings.end(); ++m)
     db_->SetFaviconOutOfDate(m->icon_id);
-  delegate_->ScheduleCommitForFavicons();
+  return true;
 }
 
 void FaviconBackend::TouchOnDemandFavicon(const GURL& icon_url) {
   TRACE_EVENT0("browser", "FaviconBackend::TouchOnDemandFavicon");
 
   db_->TouchOnDemandFavicon(icon_url, base::Time::Now());
-  delegate_->ScheduleCommitForFavicons();
 }
 
-bool FaviconBackend::SetFavicons(const base::flat_set<GURL>& page_urls,
-                                 favicon_base::IconType icon_type,
-                                 const GURL& icon_url,
-                                 const std::vector<SkBitmap>& bitmaps,
-                                 FaviconBitmapType bitmap_type) {
+SetFaviconsResult FaviconBackend::SetFavicons(
+    const base::flat_set<GURL>& page_urls,
+    favicon_base::IconType icon_type,
+    const GURL& icon_url,
+    const std::vector<SkBitmap>& bitmaps,
+    FaviconBitmapType bitmap_type) {
   TRACE_EVENT0("browser", "FaviconBackend::SetFavicons");
   DCHECK(!page_urls.empty());
 
@@ -512,22 +509,17 @@ bool FaviconBackend::SetFavicons(const base::flat_set<GURL>& page_urls,
     favicon_created = true;
   }
 
-  bool favicon_data_modified = false;
+  SetFaviconsResult result;
   if (favicon_created || bitmap_type == FaviconBitmapType::ON_VISIT)
-    favicon_data_modified = SetFaviconBitmaps(icon_id, bitmaps, bitmap_type);
+    result.did_update_bitmap = SetFaviconBitmaps(icon_id, bitmaps, bitmap_type);
 
   for (const GURL& page_url : page_urls) {
     bool mapping_changed =
         SetFaviconMappingsForPageAndRedirects(page_url, icon_type, icon_id);
-
-    if (mapping_changed) {
-      // Notify the UI that this function changed an icon mapping.
-      delegate_->OnFaviconChangedForPageAndRedirects(page_url);
-    }
+    if (mapping_changed)
+      result.updated_page_urls.insert(page_url);
   }
-
-  delegate_->ScheduleCommitForFavicons();
-  return favicon_data_modified;
+  return result;
 }
 
 FaviconBackend::FaviconBackend(std::unique_ptr<FaviconDatabase> db,

components/favicon/core/favicon_backend.h

@@ -77,32 +77,34 @@ class FaviconBackend {
   // If there is a favicon stored in the database for |icon_url|, a mapping is
   // added to the database from each element in |page_urls| (and all redirects)
   // to |icon_url|.
-  std::vector<favicon_base::FaviconRawBitmapResult>
-  UpdateFaviconMappingsAndFetch(const base::flat_set<GURL>& page_urls,
-                                const GURL& icon_url,
-                                favicon_base::IconType icon_type,
-                                const std::vector<int>& desired_sizes);
+  UpdateFaviconMappingsResult UpdateFaviconMappingsAndFetch(
+      const base::flat_set<GURL>& page_urls,
+      const GURL& icon_url,
+      favicon_base::IconType icon_type,
+      const std::vector<int>& desired_sizes);
 
-  void DeleteFaviconMappings(const base::flat_set<GURL>& page_urls,
-                             favicon_base::IconType icon_type);
+  // Deletes the mappings for the specified page urls. Returns the set of
+  // page urls that changed.
+  base::flat_set<GURL> DeleteFaviconMappings(
+      const base::flat_set<GURL>& page_urls,
+      favicon_base::IconType icon_type);
 
-  // See function of same name in HistoryService for details. Returns true if
-  // the icon changed.
-  bool MergeFavicon(const GURL& page_url,
-                    const GURL& icon_url,
-                    favicon_base::IconType icon_type,
-                    scoped_refptr<base::RefCountedMemory> bitmap_data,
-                    const gfx::Size& pixel_size);
+  // See function of same name in HistoryService for details.
+  MergeFaviconResult MergeFavicon(
+      const GURL& page_url,
+      const GURL& icon_url,
+      favicon_base::IconType icon_type,
+      scoped_refptr<base::RefCountedMemory> bitmap_data,
+      const gfx::Size& pixel_size);
 
   // If |bitmap_type| is ON_DEMAND, the icon for |icon_url| will be modified
   // only if it's not present in the database. In that case, it will be
-  // initially set as expired. Returns whether the new bitmaps were actually
-  // written. |page_urls| must not be empty.
-  bool SetFavicons(const base::flat_set<GURL>& page_urls,
-                   favicon_base::IconType icon_type,
-                   const GURL& icon_url,
-                   const std::vector<SkBitmap>& bitmaps,
-                   FaviconBitmapType bitmap_type);
+  // initially set as expired. |page_urls| must not be empty.
+  SetFaviconsResult SetFavicons(const base::flat_set<GURL>& page_urls,
+                                favicon_base::IconType icon_type,
+                                const GURL& icon_url,
+                                const std::vector<SkBitmap>& bitmaps,
+                                FaviconBitmapType bitmap_type);
 
   // Causes each page in |page_urls_to_write| to be associated to the same
   // icon as the page |page_url_to_read| for icon types matching |icon_types|.
@@ -114,17 +116,19 @@ class FaviconBackend {
       const base::flat_set<GURL>& page_urls_to_write);
 
   // See function of same name in HistoryService for details.
-  bool SetOnDemandFavicons(const GURL& page_url,
-                           favicon_base::IconType icon_type,
-                           const GURL& icon_url,
-                           const std::vector<SkBitmap>& bitmaps);
+  SetFaviconsResult SetOnDemandFavicons(const GURL& page_url,
+                                        favicon_base::IconType icon_type,
+                                        const GURL& icon_url,
+                                        const std::vector<SkBitmap>& bitmaps);
 
   // See function of same name in HistoryService for details.
   bool CanSetOnDemandFavicons(const GURL& page_url,
                               favicon_base::IconType icon_type);
 
-  // See function of same name in HistoryService for details.
-  void SetFaviconsOutOfDateForPage(const GURL& page_url);
+  // See function of same name in HistoryService for details. Returns true
+  // if the mapping was updated, false if |page_url| has no icons associated
+  // with it.
+  bool SetFaviconsOutOfDateForPage(const GURL& page_url);
 
   // See function of same name in HistoryService for details.
   void TouchOnDemandFavicon(const GURL& icon_url);

components/favicon/core/favicon_backend_delegate.h

@@ -15,17 +15,10 @@ class FaviconBackendDelegate {
  public:
   FaviconBackendDelegate() = default;
 
-  // The delegate should schedule an asynchronous commit.
-  virtual void ScheduleCommitForFavicons() = 0;
-
   // Returns the redirects for |page_url|. This should always return a
   // vector with at least one element (|page_url|).
   virtual std::vector<GURL> GetCachedRecentRedirectsForPage(
       const GURL& page_url) = 0;
-
-  // Called when the favicon for a particular page changes.
-  virtual void OnFaviconChangedForPageAndRedirects(const GURL& page_url) = 0;
-
  protected:
   virtual ~FaviconBackendDelegate() = default;
 };

components/favicon/core/favicon_backend_unittest.cc

@@ -61,7 +61,6 @@ class FaviconBackendTest : public testing::Test, public FaviconBackendDelegate {
   void TearDown() override { backend_.reset(); }
 
   // FaviconBackendDelegate:
-  void ScheduleCommitForFavicons() override {}
   std::vector<GURL> GetCachedRecentRedirectsForPage(
       const GURL& page_url) override {
     auto iter = recent_redirects_.Get(page_url);
@@ -69,7 +68,6 @@ class FaviconBackendTest : public testing::Test, public FaviconBackendDelegate {
       return iter->second;
     return {page_url};
   }
-  void OnFaviconChangedForPageAndRedirects(const GURL& page_url) override {}
 
  protected:
   void SetFavicons(const base::flat_set<GURL>& page_urls,
@@ -387,10 +385,8 @@ TEST_F(FaviconBackendTest, SetFaviconsSameFaviconURLForTwoPages) {
 
   SetFavicons({page_url1}, IconType::kFavicon, icon_url, bitmaps);
 
-  std::vector<favicon_base::FaviconRawBitmapResult> bitmap_results =
-      backend_->UpdateFaviconMappingsAndFetch({page_url2}, icon_url,
-                                              IconType::kFavicon,
-                                              GetEdgeSizesSmallAndLarge());
+  backend_->UpdateFaviconMappingsAndFetch(
+      {page_url2}, icon_url, IconType::kFavicon, GetEdgeSizesSmallAndLarge());
 
   // Check that the same FaviconID is mapped to both page URLs.
   std::vector<IconMapping> icon_mappings;
@@ -495,8 +491,10 @@ TEST_F(FaviconBackendTest, SetOnDemandFaviconsForEmptyDB) {
   std::vector<SkBitmap> bitmaps;
   bitmaps.push_back(CreateBitmap(SK_ColorRED, kSmallEdgeSize));
 
-  EXPECT_TRUE(backend_->SetOnDemandFavicons(page_url, IconType::kFavicon,
-                                            icon_url, bitmaps));
+  EXPECT_TRUE(
+      backend_
+          ->SetOnDemandFavicons(page_url, IconType::kFavicon, icon_url, bitmaps)
+          .did_update_bitmap);
 
   favicon_base::FaviconID favicon_id =
       backend_->db()->GetFaviconIDForFaviconURL(icon_url, IconType::kFavicon);
@@ -534,8 +532,10 @@ TEST_F(FaviconBackendTest, SetOnDemandFaviconsForPageInDB) {
 
   // Call SetOnDemandFavicons() with a different icon URL and bitmap data.
   bitmaps[0] = CreateBitmap(SK_ColorWHITE, kSmallEdgeSize);
-  EXPECT_FALSE(backend_->SetOnDemandFavicons(page_url, IconType::kFavicon,
-                                             icon_url2, bitmaps));
+  EXPECT_FALSE(backend_
+                   ->SetOnDemandFavicons(page_url, IconType::kFavicon,
+                                         icon_url2, bitmaps)
+                   .did_update_bitmap);
   EXPECT_EQ(0, backend_->db()->GetFaviconIDForFaviconURL(icon_url2,
                                                          IconType::kFavicon));
 
@@ -571,8 +571,10 @@ TEST_F(FaviconBackendTest, SetOnDemandFaviconsForIconInDB) {
 
   // Call SetOnDemandFavicons() with a different bitmap.
   bitmaps[0] = CreateBitmap(SK_ColorWHITE, kSmallEdgeSize);
-  EXPECT_FALSE(backend_->SetOnDemandFavicons(page_url, IconType::kFavicon,
-                                             icon_url, bitmaps));
+  EXPECT_FALSE(
+      backend_
+          ->SetOnDemandFavicons(page_url, IconType::kFavicon, icon_url, bitmaps)
+          .did_update_bitmap);
 
   EXPECT_EQ(original_favicon_id, backend_->db()->GetFaviconIDForFaviconURL(
                                      icon_url, IconType::kFavicon));

components/favicon/core/favicon_types.cc

@@ -40,4 +40,17 @@ FaviconBitmapIDSize::FaviconBitmapIDSize() = default;
 
 FaviconBitmapIDSize::~FaviconBitmapIDSize() = default;
 
+// UpdateFaviconMappingsResult -------------------------------------------------
+
+UpdateFaviconMappingsResult::UpdateFaviconMappingsResult() = default;
+UpdateFaviconMappingsResult::UpdateFaviconMappingsResult(
+    const UpdateFaviconMappingsResult& other) = default;
+UpdateFaviconMappingsResult::~UpdateFaviconMappingsResult() = default;
+
+// SetFaviconsResult -----------------------------------------------------------
+
+SetFaviconsResult::SetFaviconsResult() = default;
+SetFaviconsResult::SetFaviconsResult(const SetFaviconsResult& other) = default;
+SetFaviconsResult::~SetFaviconsResult() = default;
+
 }  // namespace favicon

components/favicon/core/favicon_types.h

@@ -110,6 +110,41 @@ struct FaviconBitmap {
   gfx::Size pixel_size;
 };
 
+struct UpdateFaviconMappingsResult {
+  UpdateFaviconMappingsResult();
+  UpdateFaviconMappingsResult(const UpdateFaviconMappingsResult& other);
+  ~UpdateFaviconMappingsResult();
+
+  std::vector<favicon_base::FaviconRawBitmapResult> bitmap_results;
+
+  // Contains the set of page urls that were updated.
+  base::flat_set<GURL> updated_page_urls;
+};
+
+struct MergeFaviconResult {
+  // If true, the mapping between the page and icon changed.
+  bool did_page_to_icon_mapping_change = false;
+
+  // True if the icon itself changed.
+  bool did_icon_change = false;
+};
+
+struct SetFaviconsResult {
+  SetFaviconsResult();
+  SetFaviconsResult(const SetFaviconsResult& other);
+  ~SetFaviconsResult();
+
+  bool did_change_database() const {
+    return did_update_bitmap || !updated_page_urls.empty();
+  }
+
+  // Set to true if the bitmap in the db was updated.
+  bool did_update_bitmap = false;
+
+  // Set of page_urls whose mapping was updated.
+  base::flat_set<GURL> updated_page_urls;
+};
+
 }  // namespace favicon
 
 #endif  // COMPONENTS_FAVICON_CORE_FAVICON_TYPES_H_

components/history/core/browser/history_backend.cc

@@ -1675,8 +1675,14 @@ HistoryBackend::UpdateFaviconMappingsAndFetch(
     const std::vector<int>& desired_sizes) {
   if (!favicon_backend_)
     return {};
-  return favicon_backend_->UpdateFaviconMappingsAndFetch(
+  auto result = favicon_backend_->UpdateFaviconMappingsAndFetch(
       page_urls, icon_url, icon_type, desired_sizes);
+  if (!result.updated_page_urls.empty()) {
+    for (auto& page_url : result.updated_page_urls)
+      SendFaviconChangedNotificationForPageAndRedirects(page_url);
+    ScheduleCommit();
+  }
+  return result.bitmap_results;
 }
 
 void HistoryBackend::DeleteFaviconMappings(
@@ -1685,7 +1691,12 @@ void HistoryBackend::DeleteFaviconMappings(
   if (!favicon_backend_ || !db_)
     return;
 
-  favicon_backend_->DeleteFaviconMappings(page_urls, icon_type);
+  auto deleted_page_urls =
+      favicon_backend_->DeleteFaviconMappings(page_urls, icon_type);
+  for (auto& deleted_page_url : deleted_page_urls)
+    SendFaviconChangedNotificationForPageAndRedirects(deleted_page_url);
+  if (!deleted_page_urls.empty())
+    ScheduleCommit();
 }
 
 void HistoryBackend::MergeFavicon(
@@ -1694,22 +1705,29 @@ void HistoryBackend::MergeFavicon(
     favicon_base::IconType icon_type,
     scoped_refptr<base::RefCountedMemory> bitmap_data,
     const gfx::Size& pixel_size) {
-  if (favicon_backend_ && db_ &&
-      favicon_backend_->MergeFavicon(page_url, icon_url, icon_type, bitmap_data,
-                                     pixel_size)) {
+  if (!favicon_backend_ || !db_)
+    return;
+
+  favicon::MergeFaviconResult result = favicon_backend_->MergeFavicon(
+      page_url, icon_url, icon_type, bitmap_data, pixel_size);
+  if (result.did_page_to_icon_mapping_change)
+    SendFaviconChangedNotificationForPageAndRedirects(page_url);
+  if (result.did_icon_change)
     SendFaviconChangedNotificationForIconURL(icon_url);
-  }
+  ScheduleCommit();
 }
 
 void HistoryBackend::SetFavicons(const base::flat_set<GURL>& page_urls,
                                  favicon_base::IconType icon_type,
                                  const GURL& icon_url,
                                  const std::vector<SkBitmap>& bitmaps) {
-  if (favicon_backend_ &&
+  if (!favicon_backend_)
+    return;
+
+  ProcessSetFaviconsResult(
       favicon_backend_->SetFavicons(page_urls, icon_type, icon_url, bitmaps,
-                                    FaviconBitmapType::ON_VISIT)) {
-    SendFaviconChangedNotificationForIconURL(icon_url);
-  }
+                                    FaviconBitmapType::ON_VISIT),
+      icon_url);
 }
 
 void HistoryBackend::CloneFaviconMappingsForPages(
@@ -1726,6 +1744,7 @@ void HistoryBackend::CloneFaviconMappingsForPages(
   if (changed_urls.empty())
     return;
 
+  ScheduleCommit();
   NotifyFaviconsChanged(changed_urls, GURL());
 }
 
@@ -1739,18 +1758,19 @@ bool HistoryBackend::SetOnDemandFavicons(const GURL& page_url,
                                          favicon_base::IconType icon_type,
                                          const GURL& icon_url,
                                          const std::vector<SkBitmap>& bitmaps) {
-  if (favicon_backend_ && db_ &&
-      favicon_backend_->SetOnDemandFavicons(page_url, icon_type, icon_url,
-                                            bitmaps)) {
-    SendFaviconChangedNotificationForIconURL(icon_url);
-    return true;
-  }
-  return false;
+  if (!favicon_backend_ || !db_)
+    return false;
+
+  return ProcessSetFaviconsResult(favicon_backend_->SetOnDemandFavicons(
+                                      page_url, icon_type, icon_url, bitmaps),
+                                  icon_url);
 }
 
 void HistoryBackend::SetFaviconsOutOfDateForPage(const GURL& page_url) {
-  if (favicon_backend_)
-    favicon_backend_->SetFaviconsOutOfDateForPage(page_url);
+  if (favicon_backend_ &&
+      favicon_backend_->SetFaviconsOutOfDateForPage(page_url)) {
+    ScheduleCommit();
+  }
 }
 
 void HistoryBackend::TouchOnDemandFavicon(const GURL& icon_url) {
@@ -1759,6 +1779,7 @@ void HistoryBackend::TouchOnDemandFavicon(const GURL& icon_url) {
   if (!favicon_backend_)
     return;
   favicon_backend_->TouchOnDemandFavicon(icon_url);
+  ScheduleCommit();
 }
 
 void HistoryBackend::SetImportedFavicons(
@@ -2313,17 +2334,23 @@ bool HistoryBackend::ClearAllMainHistory(const URLRows& kept_urls) {
   return true;
 }
 
-void HistoryBackend::ScheduleCommitForFavicons() {
-  ScheduleCommit();
-}
-
 std::vector<GURL> HistoryBackend::GetCachedRecentRedirectsForPage(
     const GURL& page_url) {
   return GetCachedRecentRedirects(page_url);
 }
 
-void HistoryBackend::OnFaviconChangedForPageAndRedirects(const GURL& page_url) {
-  SendFaviconChangedNotificationForPageAndRedirects(page_url);
+bool HistoryBackend::ProcessSetFaviconsResult(
+    const favicon::SetFaviconsResult& result,
+    const GURL& icon_url) {
+  if (!result.did_change_database())
+    return false;
+
+  ScheduleCommit();
+  if (result.did_update_bitmap)
+    SendFaviconChangedNotificationForIconURL(icon_url);
+  for (const GURL& page_url : result.updated_page_urls)
+    SendFaviconChangedNotificationForPageAndRedirects(page_url);
+  return true;
 }
 
 }  // namespace history

components/history/core/browser/history_backend.h

@@ -788,11 +788,11 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>,
   void DeleteFTSIndexDatabases();
 
   // favicon::FaviconBackendDelegate
-  void ScheduleCommitForFavicons() override;
   std::vector<GURL> GetCachedRecentRedirectsForPage(
       const GURL& page_url) override;
-  void OnFaviconChangedForPageAndRedirects(const GURL& page_url) override;
 
+  bool ProcessSetFaviconsResult(const favicon::SetFaviconsResult& result,
+                                const GURL& icon_url);
   // Data ----------------------------------------------------------------------
 
   // Delegate. See the class definition above for more information. This will

weblayer/browser/favicon/favicon_backend_wrapper.cc

@@ -76,17 +76,22 @@ FaviconBackendWrapper::GetFaviconsForUrl(
 }
 
 void FaviconBackendWrapper::SetFaviconsOutOfDateForPage(const GURL& page_url) {
-  if (favicon_backend_)
-    favicon_backend_->SetFaviconsOutOfDateForPage(page_url);
+  if (favicon_backend_ &&
+      favicon_backend_->SetFaviconsOutOfDateForPage(page_url)) {
+    ScheduleCommit();
+  }
 }
 
 void FaviconBackendWrapper::SetFavicons(const base::flat_set<GURL>& page_urls,
                                         favicon_base::IconType icon_type,
                                         const GURL& icon_url,
                                         const std::vector<SkBitmap>& bitmaps) {
-  if (favicon_backend_) {
-    favicon_backend_->SetFavicons(page_urls, icon_type, icon_url, bitmaps,
-                                  favicon::FaviconBitmapType::ON_VISIT);
+  if (favicon_backend_ &&
+      favicon_backend_
+          ->SetFavicons(page_urls, icon_type, icon_url, bitmaps,
+                        favicon::FaviconBitmapType::ON_VISIT)
+          .did_change_database()) {
+    ScheduleCommit();
   }
 }
 
@@ -100,7 +105,7 @@ void FaviconBackendWrapper::CloneFaviconMappingsForPages(
   std::set<GURL> changed_urls = favicon_backend_->CloneFaviconMappingsForPages(
       {page_url_to_read}, icon_types, page_urls_to_write);
   if (!changed_urls.empty())
-    ScheduleCommitForFavicons();
+    ScheduleCommit();
 }
 
 std::vector<favicon_base::FaviconRawBitmapResult>
@@ -118,23 +123,23 @@ FaviconBackendWrapper::UpdateFaviconMappingsAndFetch(
     const std::vector<int>& desired_sizes) {
   if (!favicon_backend_)
     return {};
-  return favicon_backend_->UpdateFaviconMappingsAndFetch(
+  auto result = favicon_backend_->UpdateFaviconMappingsAndFetch(
       page_urls, icon_url, icon_type, desired_sizes);
+  if (!result.updated_page_urls.empty())
+    ScheduleCommit();
+  return result.bitmap_results;
 }
 
 void FaviconBackendWrapper::DeleteFaviconMappings(
     const base::flat_set<GURL>& page_urls,
     favicon_base::IconType icon_type) {
   if (!favicon_backend_)
-    favicon_backend_->DeleteFaviconMappings(page_urls, icon_type);
-}
+    return;
 
-void FaviconBackendWrapper::ScheduleCommitForFavicons() {
-  if (!commit_timer_.IsRunning()) {
-    // 10 seconds matches that of HistoryBackend.
-    commit_timer_.Start(FROM_HERE, base::TimeDelta::FromSeconds(10), this,
-                        &FaviconBackendWrapper::Commit);
-  }
+  auto deleted_page_urls =
+      favicon_backend_->DeleteFaviconMappings(page_urls, icon_type);
+  if (!deleted_page_urls.empty())
+    ScheduleCommit();
 }
 
 std::vector<GURL> FaviconBackendWrapper::GetCachedRecentRedirectsForPage(
@@ -146,14 +151,16 @@ std::vector<GURL> FaviconBackendWrapper::GetCachedRecentRedirectsForPage(
   return {page_url};
 }
 
-void FaviconBackendWrapper::OnFaviconChangedForPageAndRedirects(
-    const GURL& page_url) {
-  // Nothing to do here as WebLayer doesn't notify of favicon changes through
-  // this code path.
-}
-
 FaviconBackendWrapper::~FaviconBackendWrapper() = default;
 
+void FaviconBackendWrapper::ScheduleCommit() {
+  if (!commit_timer_.IsRunning()) {
+    // 10 seconds matches that of HistoryBackend.
+    commit_timer_.Start(FROM_HERE, base::TimeDelta::FromSeconds(10), this,
+                        &FaviconBackendWrapper::Commit);
+  }
+}
+
 void FaviconBackendWrapper::Commit() {
   if (favicon_backend_)
     favicon_backend_->Commit();

weblayer/browser/favicon/favicon_backend_wrapper.h

@@ -73,10 +73,8 @@ class FaviconBackendWrapper
                              favicon_base::IconType icon_type);
 
   // favicon::FaviconBackendDelegate:
-  void ScheduleCommitForFavicons() override;
   std::vector<GURL> GetCachedRecentRedirectsForPage(
       const GURL& page_url) override;
-  void OnFaviconChangedForPageAndRedirects(const GURL& page_url) override;
 
  private:
   friend class base::RefCountedDeleteOnSequence<FaviconBackendWrapper>;
@@ -85,6 +83,7 @@ class FaviconBackendWrapper
 
   ~FaviconBackendWrapper() override;
 
+  void ScheduleCommit();
   void Commit();
 
   // Called to expire (remove) out of date icons and restart the timer.