SuggestionsStore should take the suggestion's expiry timestamp into consideration.

-----------------------------------------------------------
1. filter expired suggestions before loading from cache
2. assign default expiry timestamps (now + 72 hours), to those ones which don't have expiry timestamps, before storing in cache

BUG=387926

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

Cr-Commit-Position: refs/heads/master@{#288375}
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@288375 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/search/suggestions/suggestions_source.cc

@@ -11,13 +11,18 @@
 #include "base/bind.h"
 #include "base/memory/ref_counted_memory.h"
 #include "base/strings/string16.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/string_util.h"
+#include "base/strings/stringprintf.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/time/time.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search/suggestions/suggestions_service_factory.h"
 #include "chrome/common/url_constants.h"
 #include "components/suggestions/suggestions_service.h"
 #include "net/base/escape.h"
+#include "ui/base/l10n/time_format.h"
 #include "ui/gfx/codec/png_codec.h"
 #include "ui/gfx/image/image_skia.h"
 #include "url/gurl.h"
@@ -42,9 +47,17 @@ void RenderOutputHtml(const SuggestionsProfile& profile,
   out.push_back(kHtmlBody);
   out.push_back("<h1>Suggestions</h1>\n<ul>");
 
+  int64 now = (base::Time::NowFromSystemTime() - base::Time::UnixEpoch())
+      .ToInternalValue();
   size_t size = profile.suggestions_size();
   for (size_t i = 0; i < size; ++i) {
     const ChromeSuggestion& suggestion = profile.suggestions(i);
+    base::TimeDelta remaining_time = base::TimeDelta::FromMicroseconds(
+        suggestion.expiry_ts() - now);
+    base::string16 remaining_time_formatted = ui::TimeFormat::Detailed(
+        ui::TimeFormat::Format::FORMAT_DURATION,
+        ui::TimeFormat::Length::LENGTH_LONG,
+        -1, remaining_time);
     std::string line;
     line += "<li><a href=\"";
     line += net::EscapeForHTML(suggestion.url());
@@ -57,7 +70,9 @@ void RenderOutputHtml(const SuggestionsProfile& profile,
       line += it->second;
       line += "'>";
     }
-    line += "</a></li>\n";
+    line += "</a> Expires in ";
+    line += base::UTF16ToUTF8(remaining_time_formatted);
+    line += "</li>\n";
     out.push_back(line);
   }
   out.push_back("</ul>");

components/suggestions/proto/suggestions.proto

@@ -42,6 +42,9 @@ message ChromeSuggestion {
 
   // The provider(s) responsible for this suggestion.
   repeated ProviderId providers = 5;
+
+  // The timestamp (usec) at which this suggestion ceases to be valid.
+  optional int64 expiry_ts = 7;
 }
 
 // A list of URLs that should be filtered from the SuggestionsProfile.

components/suggestions/suggestions_service.cc

@@ -100,6 +100,9 @@ const char kSuggestionsFieldTrialControlParam[] = "control";
 const char kSuggestionsFieldTrialStateEnabled[] = "enabled";
 const char kSuggestionsFieldTrialTimeoutMs[] = "timeout_ms";
 
+// The default expiry timeout is 72 hours.
+const int64 kDefaultExpiryUsec = 72 * base::Time::kMicrosecondsPerHour;
+
 namespace {
 
 std::string GetBlacklistUrlPrefix() {
@@ -303,7 +306,7 @@ void SuggestionsService::OnURLFetchComplete(const net::URLFetcher* source) {
   bool success = request->GetResponseAsString(&suggestions_data);
   DCHECK(success);
 
-  // Compute suggestions, and dispatch then to requestors. On error still
+  // Compute suggestions, and dispatch them to requestors. On error still
   // dispatch empty suggestions.
   SuggestionsProfile suggestions;
   if (suggestions_data.empty()) {
@@ -312,6 +315,10 @@ void SuggestionsService::OnURLFetchComplete(const net::URLFetcher* source) {
   } else if (suggestions.ParseFromString(suggestions_data)) {
     LogResponseState(RESPONSE_VALID);
     thumbnail_manager_->Initialize(suggestions);
+
+    int64 now_usec = (base::Time::NowFromSystemTime() - base::Time::UnixEpoch())
+        .ToInternalValue();
+    SetDefaultExpiryTimestamp(&suggestions, now_usec + kDefaultExpiryUsec);
     suggestions_store_->StoreSuggestions(suggestions);
   } else {
     LogResponseState(RESPONSE_INVALID);
@@ -322,6 +329,18 @@ void SuggestionsService::OnURLFetchComplete(const net::URLFetcher* source) {
   ScheduleBlacklistUpload(true);
 }
 
+void SuggestionsService::SetDefaultExpiryTimestamp(
+    SuggestionsProfile* suggestions, int64 default_timestamp_usec) {
+  for (int i = 0; i < suggestions->suggestions_size(); ++i) {
+    ChromeSuggestion* suggestion = suggestions->mutable_suggestions(i);
+    // Do not set expiry if the server has already provided a more specific
+    // expiry time for this suggestion.
+    if (!suggestion->has_expiry_ts()) {
+      suggestion->set_expiry_ts(default_timestamp_usec);
+    }
+  }
+}
+
 void SuggestionsService::Shutdown() {
   // Cancel pending request and timeout closure, then serve existing requestors
   // from cache.

components/suggestions/suggestions_service.h

@@ -44,6 +44,7 @@ extern const char kSuggestionsFieldTrialBlacklistUrlParam[];
 extern const char kSuggestionsFieldTrialStateParam[];
 extern const char kSuggestionsFieldTrialControlParam[];
 extern const char kSuggestionsFieldTrialStateEnabled[];
+extern const int64 kDefaultExpiryUsec;
 
 // An interface to fetch server suggestions asynchronously.
 class SuggestionsService : public KeyedService, public net::URLFetcherDelegate {
@@ -92,6 +93,9 @@ class SuggestionsService : public KeyedService, public net::URLFetcherDelegate {
   // Register SuggestionsService related prefs in the Profile prefs.
   static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
 
+  // Sets default timestamp for suggestions which do not have expiry timestamp.
+  void SetDefaultExpiryTimestamp(SuggestionsProfile* suggestions,
+                                 int64 timestamp_usec);
  private:
   FRIEND_TEST_ALL_PREFIXES(SuggestionsServiceTest, BlacklistURLFails);
   FRIEND_TEST_ALL_PREFIXES(SuggestionsServiceTest, FetchSuggestionsData);

components/suggestions/suggestions_service_unittest.cc

@@ -46,6 +46,8 @@ const char kFakeBlacklistUrlParam[] = "baz";
 const char kTestTitle[] = "a title";
 const char kTestUrl[] = "http://go.com";
 const char kBlacklistUrl[] = "http://blacklist.com";
+const int64 kTestDefaultExpiry = 1402200000000000;
+const int64 kTestSetExpiry = 1404792000000000;
 
 scoped_ptr<net::FakeURLFetcher> CreateURLFetcher(
     const GURL& url, net::URLFetcherDelegate* delegate,
@@ -91,9 +93,25 @@ scoped_ptr<SuggestionsProfile> CreateSuggestionsProfile() {
   ChromeSuggestion* suggestion = profile->add_suggestions();
   suggestion->set_title(kTestTitle);
   suggestion->set_url(kTestUrl);
+  suggestion->set_expiry_ts(kTestSetExpiry);
   return profile.Pass();
 }
 
+// Creates one suggestion with expiry timestamp and one without.
+SuggestionsProfile CreateSuggestionsProfileWithExpiryTimestamps() {
+  SuggestionsProfile profile;
+  ChromeSuggestion* suggestion = profile.add_suggestions();
+  suggestion->set_title(kTestTitle);
+  suggestion->set_url(kTestUrl);
+  suggestion->set_expiry_ts(kTestSetExpiry);
+
+  suggestion = profile.add_suggestions();
+  suggestion->set_title(kTestTitle);
+  suggestion->set_url(kTestUrl);
+
+  return profile;
+}
+
 class MockSuggestionsStore : public suggestions::SuggestionsStore {
  public:
   MOCK_METHOD1(LoadSuggestions, bool(SuggestionsProfile*));
@@ -202,14 +220,12 @@ class SuggestionsServiceTest : public testing::Test {
     EXPECT_TRUE(suggestions_service != NULL);
     scoped_ptr<SuggestionsProfile> suggestions_profile(
         CreateSuggestionsProfile());
-
     // Set up net::FakeURLFetcherFactory.
     std::string expected_url =
         (std::string(kFakeSuggestionsURL) + "?") + kFakeSuggestionsCommonParams;
     factory_.SetFakeResponse(GURL(expected_url),
                              suggestions_profile->SerializeAsString(),
                              net::HTTP_OK, net::URLRequestStatus::SUCCESS);
-
     // Set up expectations on the SuggestionsStore. The number depends on
     // whether the second request is issued (it won't be issued if the second
     // fetch occurs before the first request has completed).
@@ -497,4 +513,14 @@ TEST_F(SuggestionsServiceTest, UpdateBlacklistDelay) {
   EXPECT_EQ(initial_delay, suggestions_service->blacklist_delay());
 }
 
+TEST_F(SuggestionsServiceTest, CheckDefaultTimeStamps) {
+  scoped_ptr<SuggestionsService> suggestions_service(
+      CreateSuggestionsServiceWithMocks());
+  SuggestionsProfile suggestions =
+      CreateSuggestionsProfileWithExpiryTimestamps();
+  suggestions_service->SetDefaultExpiryTimestamp(&suggestions,
+                                                 kTestDefaultExpiry);
+  EXPECT_EQ(kTestSetExpiry, suggestions.suggestions(0).expiry_ts());
+  EXPECT_EQ(kTestDefaultExpiry, suggestions.suggestions(1).expiry_ts());
+}
 }  // namespace suggestions

components/suggestions/suggestions_store.cc

@@ -8,6 +8,7 @@
 
 #include "base/base64.h"
 #include "base/prefs/pref_service.h"
+#include "base/time/time.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/suggestions/suggestions_pref_names.h"
 
@@ -40,9 +41,38 @@ bool SuggestionsStore::LoadSuggestions(SuggestionsProfile* suggestions) {
     return false;
   }
 
+  // Filter expired suggestions and update the stored suggestions if at least
+  // one was filtered. Return false if all suggestions are filtered.
+  int unfiltered_size = suggestions->suggestions_size();
+  FilterExpiredSuggestions(suggestions);
+  if (suggestions->suggestions_size() != unfiltered_size) {
+    if (!suggestions->suggestions_size()) {
+      suggestions->Clear();
+      ClearSuggestions();
+      return false;
+    } else {
+      StoreSuggestions(*suggestions);
+    }
+  }
+
   return true;
 }
 
+void SuggestionsStore::FilterExpiredSuggestions(
+    SuggestionsProfile* suggestions) {
+  SuggestionsProfile filtered_suggestions;
+  int64 now_usec = (base::Time::NowFromSystemTime() - base::Time::UnixEpoch())
+      .ToInternalValue();
+
+  for (int i = 0; i < suggestions->suggestions_size(); ++i) {
+    ChromeSuggestion* suggestion = suggestions->mutable_suggestions(i);
+    if (!suggestion->has_expiry_ts() || suggestion->expiry_ts() > now_usec) {
+      filtered_suggestions.add_suggestions()->Swap(suggestion);
+    }
+  }
+  suggestions->Swap(&filtered_suggestions);
+}
+
 bool SuggestionsStore::StoreSuggestions(const SuggestionsProfile& suggestions) {
   std::string suggestions_data;
   if (!suggestions.SerializeToString(&suggestions_data)) return false;

components/suggestions/suggestions_store.h

@@ -48,6 +48,9 @@ class SuggestionsStore {
   PrefService* pref_service_;
 
   DISALLOW_COPY_AND_ASSIGN(SuggestionsStore);
+
+  // Filters expired suggestions.
+  void FilterExpiredSuggestions(SuggestionsProfile* suggestions);
 };
 
 }  // namespace suggestions

components/suggestions/suggestions_store_unittest.cc

@@ -4,6 +4,7 @@
 
 #include "components/suggestions/suggestions_store.h"
 
+#include "base/time/time.h"
 #include "components/pref_registry/testing_pref_service_syncable.h"
 #include "components/suggestions/proto/suggestions.pb.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -16,12 +17,39 @@ namespace {
 
 const char kTestTitle[] = "Foo site";
 const char kTestUrl[] = "http://foo.com/";
+const int kTimeGapUsec = 100000;
+
+void AddSuggestion(SuggestionsProfile* suggestions, const char *title,
+                   const char *url, int64 expiry_ts) {
+  ChromeSuggestion* suggestion = suggestions->add_suggestions();
+  suggestion->set_url(title);
+  suggestion->set_title(url);
+  suggestion->set_expiry_ts(expiry_ts);
+}
 
 SuggestionsProfile CreateTestSuggestions() {
   SuggestionsProfile suggestions;
   ChromeSuggestion* suggestion = suggestions.add_suggestions();
-  suggestion->set_url(kTestUrl);
-  suggestion->set_title(kTestTitle);
+  suggestion->set_url(kTestTitle);
+  suggestion->set_title(kTestUrl);
+  return suggestions;
+}
+
+SuggestionsProfile CreateTestSuggestionsProfileWithExpiry(int expired_count,
+                                                          int valid_count) {
+  int64 now_usec = (base::Time::NowFromSystemTime() - base::Time::UnixEpoch())
+      .ToInternalValue();
+  srand(7);  // Constant seed for rand() function.
+  int64 offset_limit_usec = 30 * base::Time::kMicrosecondsPerDay;
+  SuggestionsProfile suggestions;
+  for (int i = 0; i < valid_count; i++){
+    int64 offset_usec = rand() % offset_limit_usec + kTimeGapUsec;
+    AddSuggestion(&suggestions, kTestTitle, kTestUrl, now_usec + offset_usec);
+  }
+  for (int i = 0; i < expired_count; i++){
+    int64 offset_usec = rand() % offset_limit_usec + kTimeGapUsec;
+    AddSuggestion(&suggestions, kTestTitle, kTestUrl, now_usec - offset_usec);
+  }
   return suggestions;
 }
 
@@ -31,6 +59,8 @@ void ValidateSuggestions(const SuggestionsProfile& expected,
   for (int i = 0; i < expected.suggestions_size(); ++i) {
     EXPECT_EQ(expected.suggestions(i).url(), actual.suggestions(i).url());
     EXPECT_EQ(expected.suggestions(i).title(), actual.suggestions(i).title());
+    EXPECT_EQ(expected.suggestions(i).expiry_ts(),
+              actual.suggestions(i).expiry_ts());
     EXPECT_EQ(expected.suggestions(i).favicon_url(),
               actual.suggestions(i).favicon_url());
     EXPECT_EQ(expected.suggestions(i).thumbnail(),
@@ -40,27 +70,77 @@ void ValidateSuggestions(const SuggestionsProfile& expected,
 
 }  // namespace
 
-TEST(SuggestionsStoreTest, LoadStoreClear) {
-  TestingPrefServiceSyncable prefs;
-  SuggestionsStore::RegisterProfilePrefs(prefs.registry());
-  SuggestionsStore suggestions_store(&prefs);
+class SuggestionsStoreTest : public testing::Test {
+ public:
+  SuggestionsStoreTest()
+    : pref_service_(new user_prefs::TestingPrefServiceSyncable) {}
+
+  virtual void SetUp() OVERRIDE {
+    SuggestionsStore::RegisterProfilePrefs(pref_service_->registry());
+    suggestions_store_.reset(new SuggestionsStore(pref_service_.get()));
+  }
+
+ protected:
+  scoped_ptr<user_prefs::TestingPrefServiceSyncable> pref_service_;
+  scoped_ptr<SuggestionsStore> suggestions_store_;
+
+  DISALLOW_COPY_AND_ASSIGN(SuggestionsStoreTest);
+};
+
+// Tests LoadSuggestions function to filter expired suggestions.
+TEST_F(SuggestionsStoreTest, LoadAllExpired) {
+  SuggestionsProfile suggestions = CreateTestSuggestionsProfileWithExpiry(5, 0);
+  SuggestionsProfile filtered_suggestions;
+
+  // Store and load. Expired suggestions should not be loaded.
+  EXPECT_TRUE(suggestions_store_->StoreSuggestions(suggestions));
+  EXPECT_FALSE(suggestions_store_->LoadSuggestions(&filtered_suggestions));
+  EXPECT_EQ(0, filtered_suggestions.suggestions_size());
+}
+
+// Tests LoadSuggestions function to filter expired suggestions.
+TEST_F(SuggestionsStoreTest, LoadValidAndExpired) {
+  SuggestionsProfile suggestions = CreateTestSuggestionsProfileWithExpiry(5, 3);
+  SuggestionsProfile filtered_suggestions;
+
+  // Store and load. Expired suggestions should not be loaded.
+  EXPECT_TRUE(suggestions_store_->StoreSuggestions(suggestions));
+  EXPECT_TRUE(suggestions_store_->LoadSuggestions(&filtered_suggestions));
+  EXPECT_EQ(3, filtered_suggestions.suggestions_size());
+}
+
+// Tests LoadSuggestions function to filter expired suggestions.
+TEST_F(SuggestionsStoreTest, CheckStoreAfterLoadExpired) {
+  SuggestionsProfile suggestions = CreateTestSuggestionsProfileWithExpiry(5, 3);
+  SuggestionsProfile filtered_suggestions;
+
+  // Store and load. Expired suggestions should not be loaded.
+  EXPECT_TRUE(suggestions_store_->StoreSuggestions(suggestions));
+  EXPECT_TRUE(suggestions_store_->LoadSuggestions(&filtered_suggestions));
+
+  SuggestionsProfile loaded_suggestions;
+  EXPECT_TRUE(suggestions_store_->LoadSuggestions(&loaded_suggestions));
+  EXPECT_EQ(3, loaded_suggestions.suggestions_size());
+  ValidateSuggestions(filtered_suggestions, loaded_suggestions);
+}
 
+TEST_F(SuggestionsStoreTest, LoadStoreClear) {
   const SuggestionsProfile suggestions = CreateTestSuggestions();
   const SuggestionsProfile empty_suggestions;
   SuggestionsProfile recovered_suggestions;
 
   // Attempt to load when prefs are empty.
-  EXPECT_FALSE(suggestions_store.LoadSuggestions(&recovered_suggestions));
+  EXPECT_FALSE(suggestions_store_->LoadSuggestions(&recovered_suggestions));
   ValidateSuggestions(empty_suggestions, recovered_suggestions);
 
   // Store then reload.
-  EXPECT_TRUE(suggestions_store.StoreSuggestions(suggestions));
-  EXPECT_TRUE(suggestions_store.LoadSuggestions(&recovered_suggestions));
+  EXPECT_TRUE(suggestions_store_->StoreSuggestions(suggestions));
+  EXPECT_TRUE(suggestions_store_->LoadSuggestions(&recovered_suggestions));
   ValidateSuggestions(suggestions, recovered_suggestions);
 
   // Clear.
-  suggestions_store.ClearSuggestions();
-  EXPECT_FALSE(suggestions_store.LoadSuggestions(&recovered_suggestions));
+  suggestions_store_->ClearSuggestions();
+  EXPECT_FALSE(suggestions_store_->LoadSuggestions(&recovered_suggestions));
   ValidateSuggestions(empty_suggestions, recovered_suggestions);
 }