Make the LocalSiteCharacteristicsDataStore a HistoryServiceObserver

Implement HistoryServiceObserver::OnURLsDeleted to remove all
observations when the history gets cleared.

BUG: 773382
Change-Id: I65026cfda256c252e33aea01d6c8db8fef30dd10
Reviewed-on: https://chromium-review.googlesource.com/1030853
Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554514}

chrome/browser/resource_coordinator/local_site_characteristics_data_impl.cc

@@ -128,12 +128,7 @@ LocalSiteCharacteristicsDataImpl::LocalSiteCharacteristicsDataImpl(
       active_webcontents_count_(0U),
       delegate_(delegate) {
   DCHECK_NE(nullptr, delegate_);
-  // Initialize the feature elements with the default value, this is required
-  // because some fields might otherwise never be initialized.
-  for (auto* iter : GetAllFeaturesFromProto(&site_characteristics_))
-    InitSiteCharacteristicsFeatureProtoWithDefaultValues(iter);
-
-  site_characteristics_.set_last_loaded(kZeroIntervalInternalRepresentation);
+  InitWithDefaultValues();
 }
 
 base::TimeDelta LocalSiteCharacteristicsDataImpl::FeatureObservationDuration(
@@ -146,7 +141,7 @@ base::TimeDelta LocalSiteCharacteristicsDataImpl::FeatureObservationDuration(
 
   // If this site is still loaded and the feature isn't in use then the
   // observation time since load needs to be added.
-  if (active_webcontents_count_ > 0U &&
+  if (IsLoaded() &&
       InternalRepresentationToTimeDelta(feature_proto.use_timestamp())
           .is_zero()) {
     base::TimeDelta observation_time_since_load =
@@ -163,7 +158,7 @@ LocalSiteCharacteristicsDataImpl::~LocalSiteCharacteristicsDataImpl() {
   // It's currently required that the site gets unloaded before destroying this
   // object.
   // TODO(sebmarchand): Check if this is a valid assumption.
-  DCHECK_EQ(0U, active_webcontents_count_);
+  DCHECK(!IsLoaded());
 
   DCHECK_NE(nullptr, delegate_);
   delegate_->OnLocalSiteCharacteristicsDataImplDestroyed(this);
@@ -191,6 +186,29 @@ void LocalSiteCharacteristicsDataImpl::
   proto->set_use_timestamp(kZeroIntervalInternalRepresentation);
 }
 
+void LocalSiteCharacteristicsDataImpl::InitWithDefaultValues() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  // Initialize the feature elements with the default value, this is required
+  // because some fields might otherwise never be initialized.
+  for (auto* iter : GetAllFeaturesFromProto(&site_characteristics_))
+    InitSiteCharacteristicsFeatureProtoWithDefaultValues(iter);
+
+  site_characteristics_.set_last_loaded(kZeroIntervalInternalRepresentation);
+}
+
+void LocalSiteCharacteristicsDataImpl::ClearObservations() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  // Reset all the observations.
+  InitWithDefaultValues();
+
+  // Set the last loaded time to the current time if there's some loaded
+  // instances of this site.
+  if (IsLoaded()) {
+    site_characteristics_.set_last_loaded(
+        TimeDeltaToInternalRepresentation(GetTickDeltaSinceEpoch()));
+  }
+}
+
 SiteFeatureUsage LocalSiteCharacteristicsDataImpl::GetFeatureUsage(
     const SiteCharacteristicsFeatureProto& feature_proto,
     const base::TimeDelta min_obs_time) const {
@@ -213,7 +231,7 @@ SiteFeatureUsage LocalSiteCharacteristicsDataImpl::GetFeatureUsage(
 void LocalSiteCharacteristicsDataImpl::NotifyFeatureUsage(
     SiteCharacteristicsFeatureProto* feature_proto) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK_GT(active_webcontents_count_, 0U);
+  DCHECK(IsLoaded());
 
   feature_proto->set_use_timestamp(
       TimeDeltaToInternalRepresentation(GetTickDeltaSinceEpoch()));

chrome/browser/resource_coordinator/local_site_characteristics_data_impl.h

@@ -70,6 +70,15 @@ class LocalSiteCharacteristicsDataImpl
   // TODO(sebmarchand): Add the methods necessary to record other types of
   // observations (e.g. memory and CPU usage).
 
+  base::TimeDelta last_loaded_time_for_testing() const {
+    return InternalRepresentationToTimeDelta(
+        site_characteristics_.last_loaded());
+  }
+
+  const SiteCharacteristicsProto& site_characteristics_for_testing() const {
+    return site_characteristics_;
+  }
+
  protected:
   friend class base::RefCounted<LocalSiteCharacteristicsDataImpl>;
   friend class LocalSiteCharacteristicsDataImplTest;
@@ -98,21 +107,9 @@ class LocalSiteCharacteristicsDataImpl
   base::TimeDelta FeatureObservationDuration(
       const SiteCharacteristicsFeatureProto& feature_proto) const;
 
-  // Accessors, for testing:
-  base::TimeDelta last_loaded_time_for_testing() const {
-    return InternalRepresentationToTimeDelta(
-        site_characteristics_.last_loaded());
-  }
-
-  const SiteCharacteristicsProto& site_characteristics_for_testing() const {
-    return site_characteristics_;
-  }
-
-  const std::string& origin_str() const { return origin_str_; }
-
+ private:
   static const int64_t kZeroIntervalInternalRepresentation;
 
- private:
   // Add |extra_observation_duration| to the observation window of a given
   // feature if it hasn't been used yet, do nothing otherwise.
   static void IncrementFeatureObservationDuration(
@@ -124,6 +121,14 @@ class LocalSiteCharacteristicsDataImpl
   static void InitSiteCharacteristicsFeatureProtoWithDefaultValues(
       SiteCharacteristicsFeatureProto* proto);
 
+  // Initialize this object with default values.
+  // NOTE: Do not call this directly while the site is loaded as this will not
+  // properly update the last_loaded time, instead call |ClearObservations|.
+  void InitWithDefaultValues();
+
+  // Clear all the past observations about this site.
+  void ClearObservations();
+
   // Returns the usage of |site_feature| for this site.
   SiteFeatureUsage GetFeatureUsage(
       const SiteCharacteristicsFeatureProto& feature_proto,
@@ -133,6 +138,10 @@ class LocalSiteCharacteristicsDataImpl
   // feature gets used.
   void NotifyFeatureUsage(SiteCharacteristicsFeatureProto* feature_proto);
 
+  const std::string& origin_str() const { return origin_str_; }
+
+  bool IsLoaded() const { return active_webcontents_count_ > 0U; }
+
   // This site's characteristics, contains the features and other values are
   // measured.
   SiteCharacteristicsProto site_characteristics_;

chrome/browser/resource_coordinator/local_site_characteristics_data_impl_unittest.cc

@@ -25,7 +25,6 @@ class TestLocalSiteCharacteristicsDataImpl
     : public LocalSiteCharacteristicsDataImpl {
  public:
   using LocalSiteCharacteristicsDataImpl::FeatureObservationDuration;
-  using LocalSiteCharacteristicsDataImpl::last_loaded_time_for_testing;
   using LocalSiteCharacteristicsDataImpl::OnDestroyDelegate;
   using LocalSiteCharacteristicsDataImpl::site_characteristics_for_testing;
   using LocalSiteCharacteristicsDataImpl::TimeDeltaToInternalRepresentation;

chrome/browser/resource_coordinator/local_site_characteristics_data_store.cc

@@ -6,12 +6,21 @@
 
 #include "base/memory/ptr_util.h"
 #include "base/stl_util.h"
+#include "chrome/browser/history/history_service_factory.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/resource_coordinator/local_site_characteristics_data_reader.h"
+#include "components/history/core/browser/history_service.h"
 
 namespace resource_coordinator {
 
-LocalSiteCharacteristicsDataStore::LocalSiteCharacteristicsDataStore() =
-    default;
+LocalSiteCharacteristicsDataStore::LocalSiteCharacteristicsDataStore(
+    Profile* profile)
+    : history_observer_(this) {
+  history::HistoryService* history =
+      HistoryServiceFactory::GetForProfileWithoutCreating(profile);
+  if (history)
+    history_observer_.Add(history);
+}
 
 LocalSiteCharacteristicsDataStore::~LocalSiteCharacteristicsDataStore() =
     default;
@@ -55,4 +64,40 @@ void LocalSiteCharacteristicsDataStore::
   DCHECK_EQ(1U, num_erased);
 }
 
+LocalSiteCharacteristicsDataStore::LocalSiteCharacteristicsMap::iterator
+LocalSiteCharacteristicsDataStore::ResetLocalSiteCharacteristicsEntry(
+    LocalSiteCharacteristicsMap::iterator entry) {
+  if (entry->second->IsLoaded()) {
+    entry->second->ClearObservations();
+    entry++;
+  } else {
+    entry = origin_data_map_.erase(entry);
+  }
+  return entry;
+}
+
+void LocalSiteCharacteristicsDataStore::OnURLsDeleted(
+    history::HistoryService* history_service,
+    const history::DeletionTimeRange& time_range,
+    bool expired,
+    const history::URLRows& deleted_rows,
+    const std::set<GURL>& favicon_urls) {
+  // TODO(sebmarchand): Removes these entry from the on-disk database once it's
+  // implemented.
+  if (time_range.IsAllTime()) {
+    for (auto iter = origin_data_map_.begin();
+         iter != origin_data_map_.end();) {
+      iter = ResetLocalSiteCharacteristicsEntry(iter);
+    }
+  } else {
+    for (auto deleted_row : deleted_rows) {
+      auto map_iter =
+          origin_data_map_.find(deleted_row.url().GetOrigin().GetContent());
+      if (map_iter != origin_data_map_.end()) {
+        ResetLocalSiteCharacteristicsEntry(map_iter);
+      }
+    }
+  }
+}
+
 }  // namespace resource_coordinator

chrome/browser/resource_coordinator/local_site_characteristics_data_store.h

@@ -6,9 +6,14 @@
 #define CHROME_BROWSER_RESOURCE_COORDINATOR_LOCAL_SITE_CHARACTERISTICS_DATA_STORE_H_
 
 #include "base/containers/flat_map.h"
+#include "base/gtest_prod_util.h"
 #include "base/macros.h"
+#include "base/scoped_observer.h"
 #include "chrome/browser/resource_coordinator/local_site_characteristics_data_impl.h"
 #include "chrome/browser/resource_coordinator/site_characteristics_data_store.h"
+#include "components/history/core/browser/history_service_observer.h"
+
+class Profile;
 
 namespace resource_coordinator {
 
@@ -16,12 +21,13 @@ namespace resource_coordinator {
 // characteristics database as a backend.
 class LocalSiteCharacteristicsDataStore
     : public SiteCharacteristicsDataStore,
-      public internal::LocalSiteCharacteristicsDataImpl::OnDestroyDelegate {
+      public internal::LocalSiteCharacteristicsDataImpl::OnDestroyDelegate,
+      public history::HistoryServiceObserver {
  public:
   using LocalSiteCharacteristicsMap =
       base::flat_map<std::string, internal::LocalSiteCharacteristicsDataImpl*>;
 
-  LocalSiteCharacteristicsDataStore();
+  explicit LocalSiteCharacteristicsDataStore(Profile* profile);
   ~LocalSiteCharacteristicsDataStore() override;
 
   // SiteCharacteristicDataStore:
@@ -33,6 +39,10 @@ class LocalSiteCharacteristicsDataStore
   }
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(LocalSiteCharacteristicsDataStoreTest, EndToEnd);
+  FRIEND_TEST_ALL_PREFIXES(LocalSiteCharacteristicsDataStoreTest,
+                           HistoryServiceObserver);
+
   // Returns a pointer to the LocalSiteCharacteristicsDataImpl object
   // associated with |origin|, create one and add it to |origin_data_map_|
   // if it doesn't exist.
@@ -43,10 +53,26 @@ class LocalSiteCharacteristicsDataStore
   void OnLocalSiteCharacteristicsDataImplDestroyed(
       internal::LocalSiteCharacteristicsDataImpl* impl) override;
 
+  // history::HistoryServiceObserver:
+  void OnURLsDeleted(history::HistoryService* history_service,
+                     const history::DeletionTimeRange& time_range,
+                     bool expired,
+                     const history::URLRows& deleted_rows,
+                     const std::set<GURL>& favicon_urls) override;
+
+  // Reset the observations about a given LocalSiteCharacteristicsMap entry,
+  // removes this entry from the map if it's not loaded by any tab.
+  // Returns an iterator to the next element in the map.
+  LocalSiteCharacteristicsMap::iterator ResetLocalSiteCharacteristicsEntry(
+      LocalSiteCharacteristicsMap::iterator entry);
+
   // Map a serialized origin to a LocalSiteCharacteristicDataInternal
   // pointer.
   LocalSiteCharacteristicsMap origin_data_map_;
 
+  ScopedObserver<history::HistoryService, LocalSiteCharacteristicsDataStore>
+      history_observer_;
+
   DISALLOW_COPY_AND_ASSIGN(LocalSiteCharacteristicsDataStore);
 };
 

chrome/browser/resource_coordinator/local_site_characteristics_data_store_unittest.cc

@@ -5,39 +5,141 @@
 #include "chrome/browser/resource_coordinator/local_site_characteristics_data_store.h"
 
 #include "base/macros.h"
+#include "base/test/simple_test_tick_clock.h"
 #include "chrome/browser/resource_coordinator/local_site_characteristics_data_impl.h"
+#include "chrome/browser/resource_coordinator/time.h"
+#include "chrome/test/base/testing_profile.h"
+#include "components/history/core/browser/history_types.h"
+#include "components/history/core/browser/url_row.h"
+#include "content/public/test/test_browser_thread_bundle.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace resource_coordinator {
 
-using LocalSiteCharacteristicsDataStoreTest = testing::Test;
+namespace {
+const char kTestOrigin[] = "http://www.foo.com";
+const char kTestOrigin2[] = "http://www.bar.com";
+}  // namespace
+
+class LocalSiteCharacteristicsDataStoreTest : public ::testing::Test {
+ public:
+  LocalSiteCharacteristicsDataStoreTest()
+      : scoped_set_tick_clock_for_testing_(&test_clock_),
+        data_store_(&profile_) {
+    test_clock_.SetNowTicks(base::TimeTicks::UnixEpoch());
+    test_clock_.Advance(base::TimeDelta::FromHours(1));
+  }
+
+ protected:
+  base::SimpleTestTickClock test_clock_;
+  ScopedSetTickClockForTesting scoped_set_tick_clock_for_testing_;
+  content::TestBrowserThreadBundle test_browser_thread_bundle_;
+  TestingProfile profile_;
+  LocalSiteCharacteristicsDataStore data_store_;
+};
 
 TEST_F(LocalSiteCharacteristicsDataStoreTest, EndToEnd) {
-  const std::string kOrigin = "foo.com";
-  LocalSiteCharacteristicsDataStore data_store;
-  EXPECT_TRUE(data_store.origin_data_map_for_testing().empty());
-  auto reader = data_store.GetReaderForOrigin(kOrigin);
+  auto reader = data_store_.GetReaderForOrigin(kTestOrigin);
   EXPECT_NE(nullptr, reader.get());
-  EXPECT_EQ(1U, data_store.origin_data_map_for_testing().size());
+  EXPECT_EQ(1U, data_store_.origin_data_map_for_testing().size());
 
-  internal::LocalSiteCharacteristicsDataImpl* impl =
-      data_store.origin_data_map_for_testing().find(kOrigin)->second;
-  EXPECT_NE(nullptr, impl);
+  internal::LocalSiteCharacteristicsDataImpl* data =
+      data_store_.origin_data_map_for_testing().find(kTestOrigin)->second;
+  EXPECT_NE(nullptr, data);
 
   EXPECT_EQ(SiteFeatureUsage::kSiteFeatureUsageUnknown,
             reader->UpdatesTitleInBackground());
-  impl->NotifySiteLoaded();
-  impl->NotifyUpdatesTitleInBackground();
-  impl->NotifySiteUnloaded();
+  data->NotifySiteLoaded();
+  data->NotifyUpdatesTitleInBackground();
+  EXPECT_EQ(SiteFeatureUsage::kSiteFeatureInUse,
+            reader->UpdatesTitleInBackground());
+  data->NotifySiteUnloaded();
   EXPECT_EQ(SiteFeatureUsage::kSiteFeatureInUse,
             reader->UpdatesTitleInBackground());
 
-  auto reader_copy = data_store.GetReaderForOrigin(kOrigin);
-  EXPECT_EQ(1U, data_store.origin_data_map_for_testing().size());
+  auto reader_copy = data_store_.GetReaderForOrigin(kTestOrigin);
+  EXPECT_EQ(1U, data_store_.origin_data_map_for_testing().size());
+  auto reader2 = data_store_.GetReaderForOrigin(kTestOrigin2);
+  EXPECT_EQ(2U, data_store_.origin_data_map_for_testing().size());
+  reader2.reset();
+  EXPECT_EQ(1U, data_store_.origin_data_map_for_testing().size());
   reader_copy.reset();
 
   reader.reset();
-  EXPECT_TRUE(data_store.origin_data_map_for_testing().empty());
+  EXPECT_TRUE(data_store_.origin_data_map_for_testing().empty());
+
+  data_store_.OnURLsDeleted(nullptr, history::DeletionTimeRange::AllTime(),
+                            false, history::URLRows(), std::set<GURL>());
+}
+
+TEST_F(LocalSiteCharacteristicsDataStoreTest, HistoryServiceObserver) {
+  // Load a first origin, and then make use of a feature on it.
+
+  const std::string kOrigin1Url = GURL(kTestOrigin).GetOrigin().GetContent();
+  auto reader = data_store_.GetReaderForOrigin(kOrigin1Url);
+  EXPECT_TRUE(reader);
+  internal::LocalSiteCharacteristicsDataImpl* data =
+      data_store_.origin_data_map_for_testing().find(kOrigin1Url)->second;
+  EXPECT_NE(nullptr, data);
+
+  constexpr base::TimeDelta kDelay = base::TimeDelta::FromHours(1);
+
+  EXPECT_EQ(SiteFeatureUsage::kSiteFeatureUsageUnknown,
+            reader->UpdatesTitleInBackground());
+  data->NotifySiteLoaded();
+  base::TimeDelta last_loaded_time = data->last_loaded_time_for_testing();
+  data->NotifyUpdatesTitleInBackground();
+  EXPECT_EQ(SiteFeatureUsage::kSiteFeatureInUse,
+            reader->UpdatesTitleInBackground());
+  test_clock_.Advance(kDelay);
+
+  // Load a second origin, make use of a feature on it too.
+  const std::string kOrigin2Url = GURL(kTestOrigin2).GetOrigin().GetContent();
+  auto reader2 = data_store_.GetReaderForOrigin(kOrigin2Url);
+  EXPECT_TRUE(reader2);
+  internal::LocalSiteCharacteristicsDataImpl* data2 =
+      data_store_.origin_data_map_for_testing().find(kOrigin2Url)->second;
+  EXPECT_NE(nullptr, data2);
+  data2->NotifySiteLoaded();
+  data2->NotifyUpdatesFaviconInBackground();
+
+  // This site hasn'be been unloaded yet, so the last loaded time shouldn't have
+  // changed.
+  EXPECT_EQ(data->last_loaded_time_for_testing(), last_loaded_time);
+
+  history::URLRows urls_to_delete = {
+      history::URLRow(GURL(kTestOrigin)),
+      history::URLRow(GURL("http://www.url-not-in-map.com"))};
+  data_store_.OnURLsDeleted(nullptr, history::DeletionTimeRange::Invalid(),
+                            false, urls_to_delete, std::set<GURL>());
+
+  // The information for this site have been reset, so the last loaded time
+  // should now be equal to the current time and the title update feature
+  // observation should have been cleared.
+  EXPECT_NE(data->last_loaded_time_for_testing(), last_loaded_time);
+  EXPECT_EQ(data->last_loaded_time_for_testing(),
+            test_clock_.NowTicks() - base::TimeTicks::UnixEpoch());
+  EXPECT_EQ(SiteFeatureUsage::kSiteFeatureUsageUnknown,
+            reader->UpdatesTitleInBackground());
+  // The second site shouldn't have been cleared.
+  EXPECT_EQ(SiteFeatureUsage::kSiteFeatureInUse,
+            reader2->UpdatesFaviconInBackground());
+
+  test_clock_.Advance(kDelay);
+
+  // Delete all the information stored in the data store.
+  data_store_.OnURLsDeleted(nullptr, history::DeletionTimeRange::AllTime(),
+                            false, history::URLRows(), std::set<GURL>());
+
+  EXPECT_EQ(SiteFeatureUsage::kSiteFeatureUsageUnknown,
+            reader2->UpdatesFaviconInBackground());
+  EXPECT_EQ(data->last_loaded_time_for_testing(),
+            test_clock_.NowTicks() - base::TimeTicks::UnixEpoch());
+  EXPECT_EQ(data2->last_loaded_time_for_testing(),
+            test_clock_.NowTicks() - base::TimeTicks::UnixEpoch());
+
+  data->NotifySiteUnloaded();
+  data2->NotifySiteUnloaded();
 }
 
 }  // namespace resource_coordinator