Use bookmark creation date fallback for 6 weeks after installing M54

components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h"
 
 #include <utility>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/location.h"
 #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"
 #include "ui/gfx/image/image.h"
 
 using bookmarks::BookmarkModel;
 using bookmarks::BookmarkNode;
 
 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 = "bookmarks_max_count";
+const char* kMinBookmarksParamName = "bookmarks_min_count";
+const char* kMaxBookmarkAgeInDaysParamName = "bookmarks_max_age_in_days";
 
 base::Time GetThresholdTime() {
   std::string age_in_days_string = variations::GetVariationParamValueByFeature(
     ntp_snippets::kBookmarkSuggestionsFeature, kMaxBookmarkAgeInDaysParamName);
   int age_in_days = 0;
-  if (!base::StringToInt(age_in_days_string, &age_in_days))
+  if (!base::StringToInt(age_in_days_string, &age_in_days)) {
+    if (!age_in_days_string.empty())
+      LOG(WARNING) << "Failed to parse bookmark age " << age_in_days_string;
     age_in_days = kMaxBookmarkAgeInDays;
-
+  }
   return base::Time::Now() - base::TimeDelta::FromDays(age_in_days);
 }
 
 int GetMaxCount() {
   std::string max_count_string = variations::GetVariationParamValueByFeature(
       ntp_snippets::kBookmarkSuggestionsFeature, kMaxBookmarksParamName);
   int max_count = 0;
   if (base::StringToInt(max_count_string, &max_count))
     return max_count;
+  if (!max_count_string.empty())
+    LOG(WARNING) << "Failed to parse max bookmarks count" << max_count_string;
 
   return kMaxBookmarks;
 }
 
 int GetMinCount() {
   std::string min_count_string = variations::GetVariationParamValueByFeature(
       ntp_snippets::kBookmarkSuggestionsFeature, kMinBookmarksParamName);
   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;
 }
 
 }  // namespace
 
 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_(
           category_factory->FromKnownCategory(KnownCategories::BOOKMARKS)),
       bookmark_model_(bookmark_model),
       fetch_requested_(false),
       end_of_list_last_visit_date_(GetThresholdTime()) {
+  base::Time first_M54_start;

[Bernhard Bauer, 2016/08/19 16:46:41]

Nit: the "m" would still be lowercase.

[Philipp Keck, 2016/08/19 17:15:51]

Done.

+  if (pref_service->HasPrefPath(prefs::kBookmarksFirstM54Start)) {
+    first_M54_start = base::Time::FromInternalValue(
+        pref_service->GetInt64(prefs::kBookmarksFirstM54Start));

[Bernhard Bauer, 2016/08/19 16:46:41]

You could just get the value unconditionally and set it to Now() if it's 0.

[Philipp Keck, 2016/08/19 17:15:51]

:D That's what I did before. Marc argued for this because it's more readable.
For the other solution, you have to know that 0 is the default value (it's
specified in line 119) and then think that "default value returns means didn't
exist before". Normally, it's still readable because the pattern "variable =
getValueMaybe(); if (variable not set) variable = getOtherValue();" is pretty
common.

But in this case I see Marc's point: The else branch does not only set the
variable, it also writes a pref. And that's really logic that's independent of
the variable and the default value. If it was alone (i.e. if we didn't use
first_m54_start afterwards), it should read like this:
if (isFirstStart) setPref("firstStartTime", Now())
And "!HasPrefPath('firstStartTime')" comes pretty close to "isFirstStart", I
think, whereas the whole default value thing does not.

+  } else {
+    first_M54_start = base::Time::Now();
+    pref_service->SetInt64(prefs::kBookmarksFirstM54Start,
+                           first_M54_start.ToInternalValue());
+  }
+  base::TimeDelta time_since_first_M54_start =
+      base::Time::Now() - first_M54_start;

[Bernhard Bauer, 2016/08/19 16:46:41]

OCD-me is a bit concerned about having two separate calls to Now() here :) I
don't think it's likely that the time difference will be more than 42 days, but
it would still be nice if it was exactly 0 if this is the first M54 start.

[Philipp Keck, 2016/08/19 17:15:51]

Done.

+  creation_date_fallback_ =
+      time_since_first_M54_start.InDays() < kUseCreationDateFallbackForDays;
   bookmark_model_->AddObserver(this);
   FetchBookmarks();
 }
 
 BookmarkSuggestionsProvider::~BookmarkSuggestionsProvider() {
   bookmark_model_->RemoveObserver(this);
 }
 
+// static
+void BookmarkSuggestionsProvider::RegisterProfilePrefs(
+    PrefRegistrySimple* registry) {
+  registry->RegisterInt64Pref(prefs::kBookmarksFirstM54Start, 0);
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // Private methods
 
 std::vector<Category> BookmarkSuggestionsProvider::GetProvidedCategories() {
   return std::vector<Category>({provided_category_});
 }
 
 CategoryStatus BookmarkSuggestionsProvider::GetCategoryStatus(
     Category category) {
   DCHECK_EQ(category, provided_category_);
@@ skipping 55 matching lines @@
     fetch_requested_ = false;
     FetchBookmarksInternal();
   }
 }
 
 void BookmarkSuggestionsProvider::OnWillChangeBookmarkMetaInfo(
     BookmarkModel* model,
     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;
 
   // Last visit date of a node has changed (and is relevant for the list), we
   // should update the suggestions.
   FetchBookmarks();
 }
 
 void BookmarkSuggestionsProvider::BookmarkNodeRemoved(
       bookmarks::BookmarkModel* model,
       const bookmarks::BookmarkNode* parent,
       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();
 }
 
 ContentSuggestion BookmarkSuggestionsProvider::ConvertBookmark(
     const BookmarkNode* bookmark) {
   ContentSuggestion suggestion(
       MakeUniqueID(provided_category_, bookmark->url().spec()),
       bookmark->url());
 
   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;
 }
 
 void BookmarkSuggestionsProvider::FetchBookmarksInternal() {
   DCHECK(bookmark_model_->loaded());
 
   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)
     suggestions.emplace_back(ConvertBookmark(bookmark));
 
   if (suggestions.empty())
     end_of_list_last_visit_date_ = threshold_time;
   else
     end_of_list_last_visit_date_ = suggestions.back().publish_date();
 
@@ skipping 10 matching lines @@
 
 void BookmarkSuggestionsProvider::NotifyStatusChanged(
     CategoryStatus new_status) {
   if (category_status_ == new_status)
     return;
   category_status_ = new_status;
   observer()->OnCategoryStatusChanged(this, provided_category_, new_status);
 }
 
 }  // namespace ntp_snippets