Compute view timestamp based on server timestamp

Also fix a couple other issues:
1) Fix the check for minimum view interval logic to user the persisted
timestamp to account for chrome restart.
2) Base64 the serialized proto to make sure it can be written to prefs
store under all circumstances

Bug: 1321298
Change-Id: Id98b7bc34962ff3032664cb1135f5e82079d70db
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3636565
Commit-Queue: Jian Li <jianli@chromium.org>
Reviewed-by: Dan H <harringtond@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1001642}

components/feed/core/proto/v2/store.proto

@@ -70,6 +70,9 @@ message StreamData {
   repeated int64 content_ids = 11;
   // Root EventID provided by the server.
   bytes root_event_id = 12;
+  // The unix timestamp in milliseconds that the feed response is produced on
+  // the server. This is returned from the server and based on server clock.
+  int64 last_server_response_time_millis = 15;
 
   reserved 3, 5;
 }

components/feed/core/proto/v2/wire/info_card.proto

@@ -18,8 +18,8 @@ message InfoCardTrackingState {
   optional int32 explicitly_dismissed_count = 2;
   optional int32 view_count = 3;
   optional int32 click_count = 4;
-  optional uint64 first_view_timestamp = 5;
-  optional uint64 last_view_timestamp = 6;
+  optional int64 first_view_timestamp = 5;
+  optional int64 last_view_timestamp = 6;
 }
 message InfoCardServingInfo {
   repeated int32 fulfilled_info_card_types = 1;

components/feed/core/v2/api_test/feed_api_stream_unittest.cc

@@ -1864,7 +1864,8 @@ TEST_F(FeedApiTest, LoadMoreDoesNotUpdateLoggingEnabled) {
   for (bool waa_on : {true, false}) {
     for (bool privacy_notice_fulfilled : {true, false}) {
       response_translator_.InjectResponse(MakeTypicalNextPageState(
-          page++, kTestTimeEpoch, signed_in, waa_on, privacy_notice_fulfilled));
+          page++, kTestTimeEpoch, kTestTimeEpoch, signed_in, waa_on,
+          privacy_notice_fulfilled));
       stream_->LoadMore(surface, base::DoNothing());
       WaitForIdleTaskQueue();
       EXPECT_TRUE(surface.update->logging_parameters().logging_enabled());
@@ -3101,13 +3102,25 @@ TEST_F(FeedApiTest, FollowedFromWebPageMenuCount) {
 }
 
 TEST_F(FeedApiTest, InfoCardTrackingActions) {
-  StreamModelUpdateRequestGenerator model_generator;
-  response_translator_.InjectResponse(model_generator.MakeFirstPage());
+  // Set up the server and client timestamps that affect the computation of
+  // the view timestamps in the info card tracking state.
+  base::Time server_timestamp = base::Time::Now();
+  task_environment_.AdvanceClock(base::Seconds(100));
+  base::Time client_timestamp = base::Time::Now();
+  base::TimeDelta timestamp_adjustment = server_timestamp - client_timestamp;
+  task_environment_.AdvanceClock(base::Seconds(200));
+
+  // Load the initial page.
+  response_translator_.InjectResponse(
+      MakeTypicalInitialModelState(0, client_timestamp, server_timestamp));
   TestForYouSurface surface(stream_.get());
   WaitForIdleTaskQueue();
 
   base::HistogramTester histograms;
 
+  // Perform actions on one info card and verify the histograms.
+  base::Time first_view_timestamp2 = base::Time::Now() + timestamp_adjustment;
+  base::Time last_view_timestamp2 = first_view_timestamp2;
   stream_->ReportInfoCardTrackViewStarted(kForYouStream, kTestInfoCardType2);
   stream_->ReportInfoCardViewed(kForYouStream, kTestInfoCardType2,
                                 kMinimumViewIntervalSeconds);
@@ -3122,12 +3135,15 @@ TEST_F(FeedApiTest, InfoCardTrackingActions) {
   histograms.ExpectBucketCount("ContentSuggestions.Feed.InfoCard.Dismissed",
                                kTestInfoCardType2, 0);
 
+  // Perform actions on another info card and verify the histograms.
+  base::Time first_view_timestamp1 = base::Time::Now() + timestamp_adjustment;
   stream_->ReportInfoCardViewed(kForYouStream, kTestInfoCardType1,
                                 kMinimumViewIntervalSeconds);
   task_environment_.AdvanceClock(base::Seconds(kMinimumViewIntervalSeconds));
   stream_->ReportInfoCardViewed(kForYouStream, kTestInfoCardType1,
                                 kMinimumViewIntervalSeconds);
   task_environment_.AdvanceClock(base::Seconds(kMinimumViewIntervalSeconds));
+  base::Time last_view_timestamp1 = base::Time::Now() + timestamp_adjustment;
   stream_->ReportInfoCardViewed(kForYouStream, kTestInfoCardType1,
                                 kMinimumViewIntervalSeconds);
   stream_->ReportInfoCardClicked(kForYouStream, kTestInfoCardType1);
@@ -3141,11 +3157,14 @@ TEST_F(FeedApiTest, InfoCardTrackingActions) {
   histograms.ExpectBucketCount("ContentSuggestions.Feed.InfoCard.Dismissed",
                                kTestInfoCardType1, 1);
 
-  response_translator_.InjectResponse(model_generator.MakeFirstPage());
-  stream_->UnloadModel(kForYouStream);
-  stream_->ExecuteRefreshTask(RefreshTaskId::kRefreshForYouFeed);
+  // Refresh the page so that a feed query including the info card tracking
+  // states is sent.
+  response_translator_.InjectResponse(MakeTypicalRefreshModelState());
+  stream_->ManualRefresh(kForYouStream, base::DoNothing());
   WaitForIdleTaskQueue();
 
+  // Verify the info card tracking states. There should be 2 states with
+  // expected counts and view timestamps populated.
   ASSERT_EQ(2, network_.query_request_sent->feed_request()
                    .feed_query()
                    .chrome_fulfillment_info()
@@ -3155,6 +3174,10 @@ TEST_F(FeedApiTest, InfoCardTrackingActions) {
   state1.set_view_count(3);
   state1.set_click_count(1);
   state1.set_explicitly_dismissed_count(1);
+  state1.set_first_view_timestamp(
+      feedstore::ToTimestampMillis(first_view_timestamp1));
+  state1.set_last_view_timestamp(
+      feedstore::ToTimestampMillis(last_view_timestamp1));
   EXPECT_THAT(state1, EqualsProto(network_.query_request_sent->feed_request()
                                       .feed_query()
                                       .chrome_fulfillment_info()
@@ -3163,6 +3186,10 @@ TEST_F(FeedApiTest, InfoCardTrackingActions) {
   state2.set_type(kTestInfoCardType2);
   state2.set_view_count(1);
   state2.set_click_count(2);
+  state2.set_first_view_timestamp(
+      feedstore::ToTimestampMillis(first_view_timestamp2));
+  state2.set_last_view_timestamp(
+      feedstore::ToTimestampMillis(last_view_timestamp2));
   EXPECT_THAT(state2, EqualsProto(network_.query_request_sent->feed_request()
                                       .feed_query()
                                       .chrome_fulfillment_info()

components/feed/core/v2/feed_stream.cc

@@ -905,7 +905,6 @@ RequestMetadata FeedStream::GetCommonRequestMetadata(
   result.autoplay_enabled = delegate_->IsAutoplayEnabled();
   result.acknowledged_notice_keys =
       NoticeCardTracker::GetAllAckowledgedKeys(profile_prefs_);
-  result.info_card_tracking_states = info_card_tracker_.GetAllStates();
 
   if (signed_in_request) {
     result.client_instance_id = prefs::GetClientInstanceId(*profile_prefs_);
@@ -953,6 +952,13 @@ RequestMetadata FeedStream::GetRequestMetadata(const StreamType& stream_type,
   if (stream_type.IsWebFeed()) {
     result.content_order = GetValidWebFeedContentOrder(*profile_prefs_);
   }
+
+  if (stream->model) {
+    result.info_card_tracking_states = info_card_tracker_.GetAllStates(
+        stream->model->last_server_response_time_millis(),
+        stream->model->last_added_time_millis());
+  }
+
   return result;
 }
 

components/feed/core/v2/feedstore_util.cc

@@ -186,4 +186,12 @@ feed::ContentIdSet GetViewContentIds(const Metadata& metadata,
   return {};
 }
 
+void SetLastServerResponseTime(base::Time t, feedstore::StreamData& data) {
+  data.set_last_server_response_time_millis(ToTimestampMillis(t));
+}
+
+base::Time GetLastServerResponseTime(const feedstore::StreamData& data) {
+  return FromTimestampMillis(data.last_server_response_time_millis());
+}
+
 }  // namespace feedstore

components/feed/core/v2/feedstore_util.h

@@ -32,11 +32,14 @@ feed::StreamType StreamTypeFromId(base::StringPiece id);
 int64_t ToTimestampMillis(base::Time t);
 base::Time FromTimestampMillis(int64_t millis);
 void SetLastAddedTime(base::Time t, feedstore::StreamData& data);
+void SetLastServerResponseTime(base::Time t, feedstore::StreamData& data);
 
 base::Time GetLastAddedTime(const feedstore::StreamData& data);
 base::Time GetSessionIdExpiryTime(const feedstore::Metadata& metadata);
 base::Time GetStreamViewTime(const Metadata& metadata,
                              const feed::StreamType& stream_type);
+base::Time GetLastServerResponseTime(const feedstore::StreamData& data);
+
 bool IsKnownStale(const Metadata& metadata,
                   const feed::StreamType& stream_type);
 base::Time GetLastFetchTime(const Metadata& metadata,

components/feed/core/v2/protocol_translator.cc

@@ -320,6 +320,12 @@ RefreshResponseData TranslateWireResponse(
         TranslateContentLifetime(response_metadata.content_lifetime());
   }
 
+  if (response_metadata.has_response_time_ms()) {
+    feedstore::SetLastServerResponseTime(
+        feedstore::FromTimestampMillis(response_metadata.response_time_ms()),
+        result->stream_data);
+  }
+
   const auto& chrome_response_metadata =
       response_metadata.chrome_feed_response_metadata();
   // Note that we're storing the raw proto bytes for the root event ID because

components/feed/core/v2/stream/info_card_tracker.cc

@@ -6,11 +6,13 @@
 
 #include <algorithm>
 
+#include "base/base64.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/time/time.h"
 #include "base/values.h"
 #include "components/feed/core/common/pref_names.h"
 #include "components/feed/core/v2/config.h"
+#include "components/feed/core/v2/feedstore_util.h"
 #include "components/feed/core/v2/proto_util.h"
 #include "components/feed/feed_feature_list.h"
 #include "components/prefs/pref_service.h"
@@ -30,6 +32,38 @@ bool compareInfoCardTrackingState(const InfoCardTrackingState& i1,
   return (i1.type() < i2.type());
 }
 
+InfoCardTrackingState DecodeFromBase64SerializedString(
+    const std::string& base64_serialized_state) {
+  InfoCardTrackingState state;
+
+  std::string serialized_state;
+  if (!base::Base64Decode(base64_serialized_state, &serialized_state)) {
+    DLOG(ERROR) << "Error decoding persisted state from base64";
+    return state;
+  }
+
+  if (!state.ParseFromString(serialized_state))
+    DLOG(ERROR) << "Error parsing InfoCardTrackingState message";
+
+  return state;
+}
+
+int64_t GetAdjustedViewTimestamp(int64_t view_timestamp,
+                                 int64_t server_timestamp,
+                                 int64_t timestamp_adjustment) {
+  view_timestamp += timestamp_adjustment;
+  // Ensure that the view timestamp does not get earlier than the server.
+  if (view_timestamp < server_timestamp)
+    view_timestamp = server_timestamp;
+  // Ensure that the view timestamp does not exceed the lifetime of the content.
+  int64_t max_timestamp =
+      server_timestamp +
+      GetFeedConfig().content_expiration_threshold.InMilliseconds();
+  if (view_timestamp > max_timestamp)
+    view_timestamp = max_timestamp;
+  return view_timestamp;
+}
+
 }  // namespace
 
 InfoCardTracker::InfoCardTracker(PrefService* profile_prefs)
@@ -39,19 +73,32 @@ InfoCardTracker::InfoCardTracker(PrefService* profile_prefs)
 
 InfoCardTracker::~InfoCardTracker() = default;
 
-std::vector<InfoCardTrackingState> InfoCardTracker::GetAllStates() const {
+std::vector<InfoCardTrackingState> InfoCardTracker::GetAllStates(
+    int64_t server_timestamp,
+    int64_t client_timestamp) const {
   std::vector<InfoCardTrackingState> states;
   const base::Value* dict = profile_prefs_->Get(prefs::kInfoCardStates);
   if (dict && dict->is_dict()) {
+    int64_t timestamp_adjustment = server_timestamp - client_timestamp;
     for (const auto pair : dict->DictItems()) {
       int info_card_type = 0;
       if (!base::StringToInt(pair.first, &info_card_type))
         continue;
       if (!pair.second.is_string())
         continue;
-      InfoCardTrackingState state;
-      state.ParseFromString(pair.second.GetString());
+      InfoCardTrackingState state =
+          DecodeFromBase64SerializedString(pair.second.GetString());
       state.set_type(info_card_type);
+      if (state.has_first_view_timestamp()) {
+        state.set_first_view_timestamp(
+            GetAdjustedViewTimestamp(state.first_view_timestamp(),
+                                     server_timestamp, timestamp_adjustment));
+      }
+      if (state.has_last_view_timestamp()) {
+        state.set_last_view_timestamp(
+            GetAdjustedViewTimestamp(state.last_view_timestamp(),
+                                     server_timestamp, timestamp_adjustment));
+      }
       states.push_back(state);
     }
   }
@@ -61,16 +108,18 @@ std::vector<InfoCardTrackingState> InfoCardTracker::GetAllStates() const {
 
 void InfoCardTracker::OnViewed(int info_card_type,
                                int minimum_view_interval_seconds) {
-  auto now = base::TimeTicks::Now();
-  auto iter = last_view_times_.find(info_card_type);
-  if (iter != last_view_times_.end() &&
-      now - iter->second < base::Seconds(minimum_view_interval_seconds)) {
+  auto now = base::Time::Now();
+  InfoCardTrackingState state = GetState(info_card_type);
+  if (state.has_last_view_timestamp() &&
+      now - feedstore::FromTimestampMillis(state.last_view_timestamp()) <
+          base::Seconds(minimum_view_interval_seconds)) {
     return;
   }
-  last_view_times_[info_card_type] = now;
 
-  InfoCardTrackingState state = GetState(info_card_type);
   state.set_view_count(state.view_count() + 1);
+  if (!state.has_first_view_timestamp())
+    state.set_first_view_timestamp(feedstore::ToTimestampMillis(now));
+  state.set_last_view_timestamp(feedstore::ToTimestampMillis(now));
   SetState(info_card_type, state);
 }
 
@@ -92,32 +141,34 @@ void InfoCardTracker::ResetState(int info_card_type) {
 }
 
 InfoCardTrackingState InfoCardTracker::GetState(int info_card_type) const {
-  InfoCardTrackingState state;
   const base::Value* all_states =
       profile_prefs_->GetDictionary(prefs::kInfoCardStates);
-  if (all_states) {
-    const std::string* serialized_state =
-        all_states->FindStringKey(InfoCardTypeToString(info_card_type));
-    if (serialized_state) {
-      if (!state.ParseFromString(*serialized_state))
-        DLOG(ERROR) << "Error parsing InfoCardTrackingState message";
-    }
-  }
-  return state;
+  if (!all_states)
+    return InfoCardTrackingState();
+  const std::string* base64_serialized_state =
+      all_states->FindStringKey(InfoCardTypeToString(info_card_type));
+  if (!base64_serialized_state)
+    return InfoCardTrackingState();
+  return DecodeFromBase64SerializedString(*base64_serialized_state);
 }
 
 void InfoCardTracker::SetState(int info_card_type,
                                const InfoCardTrackingState& state) {
+  // SerializeToString encodes the proto into a series of bytes that is not
+  // going to be compatible with UTF-8 encoding. We need to convert them to
+  // base64 before writing to the prefs store.
   std::string serialized_state;
   state.SerializeToString(&serialized_state);
+  std::string base64_state;
+  base::Base64Encode(serialized_state, &base64_state);
 
   base::Value updated_states(base::Value::Type::DICTIONARY);
   const base::Value* states = profile_prefs_->Get(prefs::kInfoCardStates);
   if (states && states->is_dict()) {
     updated_states = states->Clone();
   }
   updated_states.SetStringKey(InfoCardTypeToString(info_card_type),
-                              serialized_state);
+                              base64_state);
   profile_prefs_->Set(prefs::kInfoCardStates, updated_states);
 }
 

components/feed/core/v2/stream/info_card_tracker.h

@@ -6,7 +6,6 @@
 #define COMPONENTS_FEED_CORE_V2_STREAM_INFO_CARD_TRACKER_H_
 
 #include <string>
-#include <unordered_map>
 #include <vector>
 
 #include "base/memory/raw_ptr.h"
@@ -29,8 +28,16 @@ class InfoCardTracker {
   InfoCardTracker(const InfoCardTracker&) = delete;
   InfoCardTracker& operator=(const InfoCardTracker&) = delete;
 
-  // Returns the list of states of all tracked info cards.
-  std::vector<feedwire::InfoCardTrackingState> GetAllStates() const;
+  // Returns the list of states of all tracked info cards. The returned view
+  // timestamps will be adjusted to be based on server's clock. The adjustment
+  // is computed based on `server_timestamp` and `client_timestamp`.
+  // `server_timestamp` is the server timestamp, in milliseconds from Epoch,
+  // when the response is produced.
+  // `client_timestamp` is the client timestamp, in milliseconds from Epoch,
+  // when the response is received.
+  std::vector<feedwire::InfoCardTrackingState> GetAllStates(
+      int64_t server_timestamp,
+      int64_t client_timestamp) const;
 
   // Called when the info card is fully visible.
   void OnViewed(int info_card_type, int minimum_view_interval_seconds);
@@ -50,7 +57,6 @@ class InfoCardTracker {
                 const feedwire::InfoCardTrackingState& state);
 
   raw_ptr<PrefService> profile_prefs_;
-  std::unordered_map<int, base::TimeTicks> last_view_times_;
 };
 
 }  // namespace feed