[NTP] Add Clear Browser Data support to custom links

Delete any links initialized from Most Visited when their history entry
is cleared. If the link is modified by the user (i.e. edit/add), the
link will not be deleted when history is cleared.

Bug: 874185
Change-Id: I762fe713c097bfa75e86b25368cff25e116d674d
Reviewed-on: https://chromium-review.googlesource.com/1179246
Commit-Queue: Kristi Park <kristipark@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587227}

chrome/browser/ntp_tiles/chrome_custom_links_manager_factory.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/ntp_tiles/chrome_custom_links_manager_factory.h"
 
+#include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "components/ntp_tiles/constants.h"
 #include "components/ntp_tiles/custom_links_manager_impl.h"
@@ -13,6 +14,9 @@ ChromeCustomLinksManagerFactory::NewForProfile(Profile* profile) {
   if (!ntp_tiles::IsCustomLinksEnabled()) {
     return nullptr;
   }
+  history::HistoryService* history_service =
+      HistoryServiceFactory::GetForProfile(profile,
+                                           ServiceAccessType::EXPLICIT_ACCESS);
   return std::make_unique<ntp_tiles::CustomLinksManagerImpl>(
-      profile->GetPrefs());
+      profile->GetPrefs(), history_service);
 }

components/ntp_tiles/BUILD.gn

@@ -102,6 +102,7 @@ source_set("unit_tests") {
     "//base/test:test_support",
     "//components/favicon/core",
     "//components/favicon_base",
+    "//components/history/core/test",
     "//components/image_fetcher/core",
     "//components/image_fetcher/core:test_support",
     "//components/pref_registry:pref_registry",

components/ntp_tiles/DEPS

@@ -6,6 +6,7 @@ include_rules = [
   "+components/grit",
   "+components/image_fetcher",
   "+components/history/core/browser",
+  "+components/history/core/test",
   "+components/pref_registry",
   "+components/prefs",
   "+components/rappor",

components/ntp_tiles/custom_links_manager.h

@@ -7,6 +7,7 @@
 
 #include <vector>
 
+#include "base/callback_list.h"
 #include "base/strings/string16.h"
 #include "components/ntp_tiles/ntp_tile.h"
 #include "url/gurl.h"
@@ -19,23 +20,32 @@ namespace ntp_tiles {
 // Custom links replaces the Most Visited tiles and allows users to manually
 // add, edit, and delete tiles (i.e. links) up to a certain maximum. Duplicate
 // URLs are not allowed, and the links are stored locally per profile.
+//
+// If the link is initialized from |Initialize|, it is considered a Most Visited
+// link and will be deleted when its history entry is cleared. Once the user
+// modifies the link, it will no longer be considered Most Visited and will not
+// be deleted when history is cleared.
+//
 // TODO(crbug/861831): Add Chrome sync support.
 class CustomLinksManager {
  public:
   struct Link {
     GURL url;
     base::string16 title;
+    bool is_most_visited = false;
 
     bool operator==(const Link& other) const {
-      return url == other.url && title == other.title;
+      return url == other.url && title == other.title &&
+             is_most_visited == other.is_most_visited;
     }
   };
 
   virtual ~CustomLinksManager() = default;
 
   // Fills the initial links with |tiles| and sets the initalized status to
-  // true. Returns false and does nothing if custom links has already been
-  // initialized.
+  // true. These links will be considered Most Visited and will be deleted when
+  // history is cleared. Returns false and does nothing if custom links has
+  // already been initialized.
   virtual bool Initialize(const NTPTilesVector& tiles) = 0;
   // Uninitializes custom links and clears the current links from storage.
   virtual void Uninitialize() = 0;
@@ -46,14 +56,16 @@ class CustomLinksManager {
   // Returns the current links.
   virtual const std::vector<Link>& GetLinks() const = 0;
 
-  // Adds a link to the end of the list. Returns false and does nothing if
-  // custom links is not initialized, |url| is invalid, we're at the maximum
-  // number of links, or |url| already exists in the list.
+  // Adds a link to the end of the list. This link will not be deleted when
+  // history is cleared. Returns false and does nothing if custom links is not
+  // initialized, |url| is invalid, we're at the maximum number of links, or
+  // |url| already exists in the list.
   virtual bool AddLink(const GURL& url, const base::string16& title) = 0;
-  // Updates the URL and/or title of the link specified by |url|. Returns
-  // false and does nothing if custom links is not initialized, either URL is
-  // invalid, |url| does not exist in the list, |new_url| already exists in the
-  // list, or both parameters are empty.
+  // Updates the URL and/or title of the link specified by |url|. The link will
+  // no longer be considered Most Visited. Returns false and does nothing if
+  // custom links is not initialized, either URL is invalid, |url| does not
+  // exist in the list, |new_url| already exists in the list, or both parameters
+  // are empty.
   virtual bool UpdateLink(const GURL& url,
                           const GURL& new_url,
                           const base::string16& new_title) = 0;
@@ -65,6 +77,12 @@ class CustomLinksManager {
   // action (add, edit, delete, etc.). Returns false and does nothing if custom
   // links is not initialized or there is no previous state to restore.
   virtual bool UndoAction() = 0;
+
+  // Registers a callback that will be invoked when custom links are updated by
+  // sources other than this interface's methods (i.e. when links are deleted by
+  // history clear).
+  virtual std::unique_ptr<base::CallbackList<void()>::Subscription>
+  RegisterCallbackForOnChanged(base::RepeatingClosure callback) = 0;
 };
 
 }  // namespace ntp_tiles

components/ntp_tiles/custom_links_manager_impl.cc

@@ -20,9 +20,16 @@ const int kMaxNumLinks = 10;
 
 }  // namespace
 
-CustomLinksManagerImpl::CustomLinksManagerImpl(PrefService* prefs)
-    : prefs_(prefs), store_(prefs), weak_ptr_factory_(this) {
+CustomLinksManagerImpl::CustomLinksManagerImpl(
+    PrefService* prefs,
+    history::HistoryService* history_service)
+    : prefs_(prefs),
+      store_(prefs),
+      history_service_observer_(this),
+      weak_ptr_factory_(this) {
   DCHECK(prefs);
+  if (history_service)
+    history_service_observer_.Add(history_service);
   if (IsInitialized())
     current_links_ = store_.RetrieveLinks();
 }
@@ -34,7 +41,7 @@ bool CustomLinksManagerImpl::Initialize(const NTPTilesVector& tiles) {
     return false;
 
   for (const NTPTile& tile : tiles)
-    current_links_.emplace_back(Link{tile.url, tile.title});
+    current_links_.emplace_back(Link{tile.url, tile.title, true});
 
   store_.StoreLinks(current_links_);
   prefs_->SetBoolean(prefs::kCustomLinksInitialized, true);
@@ -66,7 +73,7 @@ bool CustomLinksManagerImpl::AddLink(const GURL& url,
     return false;
 
   previous_links_ = current_links_;
-  current_links_.emplace_back(Link{url, title});
+  current_links_.emplace_back(Link{url, title, false});
   store_.StoreLinks(current_links_);
   return true;
 }
@@ -97,6 +104,7 @@ bool CustomLinksManagerImpl::UpdateLink(const GURL& url,
     it->url = new_url;
   if (!new_title.empty())
     it->title = new_title;
+  it->is_most_visited = false;
 
   store_.StoreLinks(current_links_);
   return true;
@@ -139,6 +147,44 @@ CustomLinksManagerImpl::FindLinkWithUrl(const GURL& url) {
                       [&url](const Link& link) { return link.url == url; });
 }
 
+std::unique_ptr<base::CallbackList<void()>::Subscription>
+CustomLinksManagerImpl::RegisterCallbackForOnChanged(
+    base::RepeatingClosure callback) {
+  return callback_list_.Add(callback);
+}
+
+// history::HistoryServiceObserver implementation.
+void CustomLinksManagerImpl::OnURLsDeleted(
+    history::HistoryService* history_service,
+    const history::DeletionInfo& deletion_info) {
+  // We don't care about expired entries.
+  if (!IsInitialized() || deletion_info.is_from_expiration())
+    return;
+
+  size_t initial_size = current_links_.size();
+  if (deletion_info.IsAllHistory()) {
+    base::EraseIf(current_links_,
+                  [](auto& link) { return link.is_most_visited; });
+  } else {
+    for (const history::URLRow& row : deletion_info.deleted_rows()) {
+      auto it = FindLinkWithUrl(row.url());
+      if (it != current_links_.end() && it->is_most_visited)
+        current_links_.erase(it);
+    }
+  }
+  store_.StoreLinks(current_links_);
+  previous_links_ = base::nullopt;
+
+  // Alert MostVisitedSites that some links have been deleted.
+  if (initial_size != current_links_.size())
+    callback_list_.Notify();
+}
+
+void CustomLinksManagerImpl::HistoryServiceBeingDeleted(
+    history::HistoryService* history_service) {
+  history_service_observer_.RemoveAll();
+}
+
 // static
 void CustomLinksManagerImpl::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* user_prefs) {

components/ntp_tiles/custom_links_manager_impl.h

@@ -11,6 +11,9 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/optional.h"
+#include "base/scoped_observer.h"
+#include "components/history/core/browser/history_service.h"
+#include "components/history/core/browser/history_service_observer.h"
 #include "components/ntp_tiles/custom_links_manager.h"
 #include "components/ntp_tiles/custom_links_store.h"
 #include "components/ntp_tiles/ntp_tile.h"
@@ -24,10 +27,13 @@ class PrefRegistrySyncable;
 namespace ntp_tiles {
 
 // Non-test implementation of the CustomLinksManager interface.
-class CustomLinksManagerImpl : public CustomLinksManager {
+class CustomLinksManagerImpl : public CustomLinksManager,
+                               public history::HistoryServiceObserver {
  public:
   // Restores the previous state of |current_links_| from prefs.
-  explicit CustomLinksManagerImpl(PrefService* prefs);
+  CustomLinksManagerImpl(PrefService* prefs,
+                         // Can be nullptr in unittests.
+                         history::HistoryService* history_service);
 
   ~CustomLinksManagerImpl() override;
 
@@ -45,6 +51,9 @@ class CustomLinksManagerImpl : public CustomLinksManager {
   bool DeleteLink(const GURL& url) override;
   bool UndoAction() override;
 
+  std::unique_ptr<base::CallbackList<void()>::Subscription>
+  RegisterCallbackForOnChanged(base::RepeatingClosure callback) override;
+
   // Register preferences used by this class.
   static void RegisterProfilePrefs(
       user_prefs::PrefRegistrySyncable* user_prefs);
@@ -54,6 +63,14 @@ class CustomLinksManagerImpl : public CustomLinksManager {
   // Returns an iterator into |custom_links_|.
   std::vector<Link>::iterator FindLinkWithUrl(const GURL& url);
 
+  // history::HistoryServiceObserver implementation.
+  // Deletes any Most Visited links whose URL is in |deletion_info|. Clears
+  // |previous_links_|. Does not delete entries expired by HistoryService.
+  void OnURLsDeleted(history::HistoryService* history_service,
+                     const history::DeletionInfo& deletion_info) override;
+  void HistoryServiceBeingDeleted(
+      history::HistoryService* history_service) override;
+
   PrefService* const prefs_;
   CustomLinksStore store_;
   std::vector<Link> current_links_;
@@ -61,6 +78,14 @@ class CustomLinksManagerImpl : public CustomLinksManager {
   // performed.
   base::Optional<std::vector<Link>> previous_links_;
 
+  // List of callbacks to be invoked when custom links are updated by outside
+  // sources.
+  base::CallbackList<void()> callback_list_;
+
+  // Observer for the HistoryService.
+  ScopedObserver<history::HistoryService, history::HistoryServiceObserver>
+      history_service_observer_;
+
   base::WeakPtrFactory<CustomLinksManagerImpl> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(CustomLinksManagerImpl);

components/ntp_tiles/most_visited_sites.cc

@@ -184,6 +184,12 @@ void MostVisitedSites::SetMostVisitedURLsObserver(Observer* observer,
     top_sites_observer_.Add(top_sites_.get());
   }
 
+  if (custom_links_) {
+    custom_links_subscription_ =
+        custom_links_->RegisterCallbackForOnChanged(base::BindRepeating(
+            &MostVisitedSites::OnCustomLinksChanged, base::Unretained(this)));
+  }
+
   suggestions_subscription_ = suggestions_service_->AddCallback(base::Bind(
       &MostVisitedSites::OnSuggestionsProfileChanged, base::Unretained(this)));
 
@@ -611,6 +617,12 @@ NTPTilesVector MostVisitedSites::InsertHomeTile(
   return new_tiles;
 }
 
+void MostVisitedSites::OnCustomLinksChanged() {
+  DCHECK(custom_links_);
+  DCHECK(custom_links_->IsInitialized());
+  BuildCustomLinks(custom_links_->GetLinks());
+}
+
 void MostVisitedSites::BuildCustomLinks(
     const std::vector<CustomLinksManager::Link>& links) {
   DCHECK(IsCustomLinksEnabled());

components/ntp_tiles/most_visited_sites.h

@@ -14,6 +14,7 @@
 #include <vector>
 
 #include "base/callback_forward.h"
+#include "base/callback_list.h"
 #include "base/compiler_specific.h"
 #include "base/files/file_path.h"
 #include "base/gtest_prod_util.h"
@@ -258,6 +259,9 @@ class MostVisitedSites : public history::TopSitesObserver,
       const std::set<std::string>& hosts_to_skip,
       size_t num_max_tiles);
 
+  // Callback for when an update is reported by CustomLinksManager.
+  void OnCustomLinksChanged();
+
   // Creates tiles for |links| up to |max_num_sites_|. |links| will never exceed
   // a certain maximum.
   void BuildCustomLinks(const std::vector<CustomLinksManager::Link>& links);
@@ -323,6 +327,9 @@ class MostVisitedSites : public history::TopSitesObserver,
   ScopedObserver<history::TopSites, history::TopSitesObserver>
       top_sites_observer_;
 
+  std::unique_ptr<base::CallbackList<void()>::Subscription>
+      custom_links_subscription_;
+
   // The main source of personal tiles - either TOP_SITES or SUGGESTIONS_SEVICE.
   TileSource mv_source_;
 

components/ntp_tiles/most_visited_sites_unittest.cc

@@ -278,6 +278,9 @@ class MockCustomLinksManager : public CustomLinksManager {
                     const base::string16& new_title));
   MOCK_METHOD1(DeleteLink, bool(const GURL& url));
   MOCK_METHOD0(UndoAction, bool());
+  MOCK_METHOD1(RegisterCallbackForOnChanged,
+               std::unique_ptr<base::CallbackList<void()>::Subscription>(
+                   base::RepeatingClosure callback));
 };
 
 class PopularSitesFactoryForTest {
@@ -1086,6 +1089,7 @@ TEST_P(MostVisitedSitesTest, ShouldOnlyBuildCustomLinksWhenInitialized) {
 
   // Build tiles when custom links is not initialized. Tiles should be Top
   // Sites.
+  EXPECT_CALL(*mock_custom_links_, RegisterCallbackForOnChanged(_));
   EXPECT_CALL(*mock_top_sites_, GetMostVisitedURLs(_, false))
       .WillRepeatedly(InvokeCallbackArgument<0>(
           MostVisitedURLList{MakeMostVisitedURL(kTestTitle, kTestUrl)}));
@@ -1153,6 +1157,7 @@ TEST_P(MostVisitedSitesTest, ShouldFavorCustomLinksOverTopSites) {
 
   // Build tiles when custom links is not initialized. Tiles should be Top
   // Sites.
+  EXPECT_CALL(*mock_custom_links_, RegisterCallbackForOnChanged(_));
   EXPECT_CALL(*mock_top_sites_, GetMostVisitedURLs(_, false))
       .WillRepeatedly(InvokeCallbackArgument<0>(
           MostVisitedURLList{MakeMostVisitedURL(kTestTitle, kTestUrl)}));
@@ -1208,6 +1213,7 @@ TEST_P(MostVisitedSitesTest, ShouldFavorCustomLinksOverSuggestions) {
 
   // Build tiles when custom links is not initialized. Tiles should be Top
   // Sites.
+  EXPECT_CALL(*mock_custom_links_, RegisterCallbackForOnChanged(_));
   EXPECT_CALL(mock_suggestions_service_, AddCallback(_))
       .WillOnce(Invoke(&suggestions_service_callbacks_,
                        &SuggestionsService::ResponseCallbackList::Add));