Use bookmark creation date fallback for 6 weeks after installing M54

components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc

Index: components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc
diff --git a/components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc b/components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc
index 04395e5c570d511e5f7abd467f852a0437dedffc..9f21c17f4f0d82ea547ec2e3a8e636dae64365d5 100644
--- a/components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc
+++ b/components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc
@@ -12,11 +12,15 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/time/time.h"
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/ntp_snippets/bookmarks/bookmark_last_visit_utils.h"
 #include "components/ntp_snippets/category_factory.h"
 #include "components/ntp_snippets/content_suggestion.h"
 #include "components/ntp_snippets/features.h"
+#include "components/ntp_snippets/pref_names.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/pref_service.h"
 #include "components/variations/variations_associated_data.h"
 #include "grit/components_strings.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -30,10 +34,11 @@ namespace {
 const int kMaxBookmarks = 10;
 const int kMinBookmarks = 3;
 const int kMaxBookmarkAgeInDays = 42;
+const int kUseCreationDateFallbackForDays = 42;
 
-const char* kMaxBookmarksParamName = "max_count";
-const char* kMinBookmarksParamName = "min_count";
-const char* kMaxBookmarkAgeInDaysParamName = "max_age_in_days";
+const char* kMaxBookmarksParamName = "ntp_bookmarks_max_count";
+const char* kMinBookmarksParamName = "ntp_bookmarks_min_count";
+const char* kMaxBookmarkAgeInDaysParamName = "ntp_bookmarks_max_age_in_days";
 
 base::Time GetThresholdTime() {
   std::string age_in_days_string = variations::GetVariationParamValueByFeature(
@@ -61,6 +66,8 @@ int GetMinCount() {
   int min_count = 0;
   if (base::StringToInt(min_count_string, &min_count))
     return min_count;
+  if (!min_count_string.empty())
+    LOG(WARNING) << "Failed to parse min bookmarks count" << min_count_string;
 
   return kMinBookmarks;
 }
@@ -72,7 +79,8 @@ namespace ntp_snippets {
 BookmarkSuggestionsProvider::BookmarkSuggestionsProvider(
     ContentSuggestionsProvider::Observer* observer,
     CategoryFactory* category_factory,
-    bookmarks::BookmarkModel* bookmark_model)
+    bookmarks::BookmarkModel* bookmark_model,
+    PrefService* pref_service)
     : ContentSuggestionsProvider(observer, category_factory),
       category_status_(CategoryStatus::AVAILABLE_LOADING),
       provided_category_(
@@ -80,6 +88,16 @@ BookmarkSuggestionsProvider::BookmarkSuggestionsProvider(
       bookmark_model_(bookmark_model),
       fetch_requested_(false),
       end_of_list_last_visit_date_(GetThresholdTime()) {
+  int64_t first_M54_start =

[Marc Treib, 2016/08/19 11:41:12]

nit: Move all this into a helper function? Then you could initialize
creation_date_fallback_ in the initializer list.

[Philipp Keck, 2016/08/19 12:49:52]

True. Not sure if the code is best placed in a helper function because that
could be called anytime from anywhere. This code only makes sense to be executed
in the constructor, when the provider is created.

[Marc Treib, 2016/08/19 13:37:17]

Well, you could argue the same for any method/function that has only a single
call site, but sometimes it still makes sense to have them. Anyway, your call,
it's also okay as is.

[Philipp Keck, 2016/08/19 14:27:36]

My point is that this code is special in that it's not like any method or
function that can move to its only call site -- or not only. An additional
characteristic is that it needs to run when Chrome starts, because it measures
the Time::Now() when M54 is first started. So the constructor of a
service/provider that is created at startup is inherently a good place for this.

But on the other hand, you can argue that it's just as well a good palce for
calling a helper function.

+      pref_service->GetInt64(prefs::kBookmarksFirstM54Start);
+  if (first_M54_start == 0) {

[Marc Treib, 2016/08/19 11:41:12]

HasPrefPath() might be easier to read than comparing to the default value.

[Philipp Keck, 2016/08/19 12:49:52]

Done.

+    first_M54_start = base::Time::Now().ToInternalValue();
+    pref_service->SetInt64(prefs::kBookmarksFirstM54Start, first_M54_start);
+  }
+  base::TimeDelta time_since_first_M54_start =
+      base::Time::Now() - base::Time::FromInternalValue(first_M54_start);
+  creation_date_fallback_ =
+      time_since_first_M54_start.InDays() < kUseCreationDateFallbackForDays;

[Marc Treib, 2016/08/19 11:41:11]

InDays() rounds down, so this is not strictly correct. Maybe compute the
difference instead and check if it's positive?

[Philipp Keck, 2016/08/19 12:49:52]

Should be correct as is.

   bookmark_model_->AddObserver(this);
   FetchBookmarks();
 }
@@ -88,6 +106,12 @@ BookmarkSuggestionsProvider::~BookmarkSuggestionsProvider() {
   bookmark_model_->RemoveObserver(this);
 }
 
+// static
+void BookmarkSuggestionsProvider::RegisterProfilePrefs(
+    PrefRegistrySimple* registry) {
+  registry->RegisterInt64Pref(prefs::kBookmarksFirstM54Start, 0);
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // Private methods
 
@@ -163,13 +187,14 @@ void BookmarkSuggestionsProvider::OnWillChangeBookmarkMetaInfo(
     const BookmarkNode* node) {
   // Store the last visit date of the node that is about to change.
   node_to_change_last_visit_date_ =
-      GetLastVisitDateForBookmarkIfNotDismissed(node);
+      GetLastVisitDateForBookmarkIfNotDismissed(node, creation_date_fallback_);
 }
 
 void BookmarkSuggestionsProvider::BookmarkMetaInfoChanged(
     BookmarkModel* model,
     const BookmarkNode* node) {
-  base::Time time = GetLastVisitDateForBookmarkIfNotDismissed(node);
+  base::Time time =
+      GetLastVisitDateForBookmarkIfNotDismissed(node, creation_date_fallback_);
   if (time == node_to_change_last_visit_date_ ||
       time < end_of_list_last_visit_date_)
     return;
@@ -185,9 +210,10 @@ void BookmarkSuggestionsProvider::BookmarkNodeRemoved(
       int old_index,
       const bookmarks::BookmarkNode* node,
       const std::set<GURL>& no_longer_bookmarked) {
-  if (GetLastVisitDateForBookmarkIfNotDismissed(node) <
-      end_of_list_last_visit_date_)
+  if (GetLastVisitDateForBookmarkIfNotDismissed(node, creation_date_fallback_) <
+      end_of_list_last_visit_date_) {
     return;
+  }
 
   // Some node from our list got deleted, we should update the suggestions.
   FetchBookmarks();
@@ -201,7 +227,8 @@ ContentSuggestion BookmarkSuggestionsProvider::ConvertBookmark(
 
   suggestion.set_title(bookmark->GetTitle());
   suggestion.set_snippet_text(base::string16());
-  suggestion.set_publish_date(GetLastVisitDateForBookmark(bookmark));
+  suggestion.set_publish_date(
+      GetLastVisitDateForBookmark(bookmark, creation_date_fallback_));
   suggestion.set_publisher_name(base::UTF8ToUTF16(bookmark->url().host()));
   return suggestion;
 }
@@ -212,9 +239,9 @@ void BookmarkSuggestionsProvider::FetchBookmarksInternal() {
   NotifyStatusChanged(CategoryStatus::AVAILABLE);
 
   base::Time threshold_time = GetThresholdTime();
-  std::vector<const BookmarkNode*> bookmarks = GetRecentlyVisitedBookmarks(
-      bookmark_model_, GetMinCount(), GetMaxCount(),
-      threshold_time);
+  std::vector<const BookmarkNode*> bookmarks =
+      GetRecentlyVisitedBookmarks(bookmark_model_, GetMinCount(), GetMaxCount(),
+                                  threshold_time, creation_date_fallback_);
 
   std::vector<ContentSuggestion> suggestions;
   for (const BookmarkNode* bookmark : bookmarks)