Feedv2: Various TODO cleanup

- Some TODOs no longer applied, others were fixed.
- StreamModel now de-dups content
- Changed some pointer parameters into references, now that
  the style guide permits this.

Change-Id: I8a246648efa38bb4928e615e9a79b7926da72c2a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2220607Reviewed-by: Ian Wells <iwells@chromium.org>
Commit-Queue: Dan H <harringtond@chromium.org>
Cr-Commit-Position: refs/heads/master@{#773380}

chrome/browser/android/feed/v2/feed_service_factory.h

@@ -18,7 +18,6 @@ class FeedService;
 
 // Factory to create one FeedService per browser context. Callers need to
 // watch out for nullptr when incognito, as the feed should not be used then.
-// TODO(harringtond): Under development, doesn't work yet!
 class FeedServiceFactory : public BrowserContextKeyedServiceFactory {
  public:
   FeedServiceFactory(const FeedServiceFactory&) = delete;

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

@@ -21,10 +21,7 @@ message ClientInfo {
     IOS = 2;
   }
 
-  enum AppType {
-    TEST_APP = 2;  // For use with AGA endpoint for testing.
-    CHROME = 3;
-  }
+  enum AppType { CLANK = 3; }
 
   // The type of OS that the client is running.
   optional PlatformType platform_type = 1;

components/feed/core/v2/enums.cc

@@ -49,6 +49,8 @@ std::ostream& operator<<(std::ostream& out, LoadStreamStatus value) {
       return out << "kLoadMoreModelIsNotLoaded";
     case LoadStreamStatus::kLoadNotAllowedDisabledByEnterprisePolicy:
       return out << "kLoadNotAllowedDisabledByEnterprisePolicy";
+    case LoadStreamStatus::kNetworkFetchFailed:
+      return out << "kNetworkFetchFailed";
   }
 #else
   return out << (static_cast<int>(value));

components/feed/core/v2/enums.h

@@ -26,7 +26,6 @@ enum class LoadStreamStatus {
   kNoStreamDataInStore = 4,
   kModelAlreadyLoaded = 5,
   kNoResponseBody = 6,
-  // TODO(harringtond): Let's add more specific proto translation errors.
   kProtoTranslationFailed = 7,
   kDataInStoreIsStale = 8,
   // The timestamp for stored data is in the future, so we're treating stored
@@ -37,11 +36,11 @@ enum class LoadStreamStatus {
   kCannotLoadFromNetworkThrottled = 12,
   kLoadNotAllowedEulaNotAccepted = 13,
   kLoadNotAllowedArticlesListHidden = 14,
-  // TODO(harringtond): Emit this status value.
   kCannotParseNetworkResponseBody = 15,
   kLoadMoreModelIsNotLoaded = 16,
   kLoadNotAllowedDisabledByEnterprisePolicy = 17,
-  kMaxValue = kLoadNotAllowedDisabledByEnterprisePolicy,
+  kNetworkFetchFailed = 18,
+  kMaxValue = kNetworkFetchFailed,
 };
 
 std::ostream& operator<<(std::ostream& out, LoadStreamStatus value);

components/feed/core/v2/feed_network_impl.cc

@@ -329,6 +329,8 @@ class FeedNetworkImpl::NetworkFetch {
     if (response) {
       response_info.status_code =
           simple_loader_->ResponseInfo()->headers->response_code();
+      response_info.response_body_bytes = response->size();
+
       response_body = std::move(*response);
 
       if (response_info.status_code == net::HTTP_UNAUTHORIZED) {

components/feed/core/v2/feed_stream.cc

@@ -40,7 +40,7 @@ namespace feed {
 namespace {
 
 void PopulateDebugStreamData(const LoadStreamTask::Result& load_result,
-                             PrefService* profile_prefs) {
+                             PrefService& profile_prefs) {
   DebugStreamData debug_data = ::feed::prefs::GetDebugStreamData(profile_prefs);
   std::stringstream ss;
   ss << "Code: " << load_result.final_status;
@@ -142,7 +142,7 @@ void FeedStream::TriggerStreamLoad() {
 }
 
 void FeedStream::InitialStreamLoadComplete(LoadStreamTask::Result result) {
-  PopulateDebugStreamData(result, profile_prefs_);
+  PopulateDebugStreamData(result, *profile_prefs_);
   metrics_reporter_->OnLoadStream(result.load_from_store_status,
                                   result.final_status);
 
@@ -223,8 +223,6 @@ void FeedStream::LoadMore(SurfaceId surface_id,
 
 void FeedStream::LoadMoreComplete(LoadMoreTask::Result result) {
   metrics_reporter_->OnLoadMore(result.final_status);
-  // TODO(harringtond): In the case of failure, do we need to load an error
-  // message slice?
   surface_updater_->SetLoadingMore(false);
   std::vector<base::OnceCallback<void(bool)>> moved_callbacks =
       std::move(load_more_complete_callbacks_);
@@ -282,7 +280,7 @@ void FeedStream::ProcessThereAndBackAgain(base::StringPiece data) {
 }
 
 DebugStreamData FeedStream::GetDebugStreamData() {
-  return ::feed::prefs::GetDebugStreamData(profile_prefs_);
+  return ::feed::prefs::GetDebugStreamData(*profile_prefs_);
 }
 
 void FeedStream::ForceRefreshForDebugging() {
@@ -442,7 +440,7 @@ void FeedStream::OnSignedOut() {
 void FeedStream::ExecuteRefreshTask() {
   // Schedule the next refresh attempt. If a new refresh schedule is returned
   // through this refresh, it will be overwritten.
-  SetRequestSchedule(feed::prefs::GetRequestSchedule(profile_prefs_));
+  SetRequestSchedule(feed::prefs::GetRequestSchedule(*profile_prefs_));
 
   LoadStreamStatus do_not_attempt_reason = ShouldAttemptLoad();
   if (do_not_attempt_reason != LoadStreamStatus::kNoStatus) {
@@ -490,7 +488,7 @@ void FeedStream::SetRequestSchedule(RequestSchedule schedule) {
   } else {
     refresh_task_scheduler_->Cancel();
   }
-  feed::prefs::SetRequestSchedule(schedule, profile_prefs_);
+  feed::prefs::SetRequestSchedule(schedule, *profile_prefs_);
 }
 
 void FeedStream::UnloadModel() {

components/feed/core/v2/feed_stream_unittest.cc

@@ -240,6 +240,8 @@ class TestFeedNetwork : public FeedNetwork {
     // time we want to inject a translated response for ease of test-writing.
     query_request_sent = request;
     QueryRequestResult result;
+    result.response_info.status_code = 200;
+    result.response_info.response_body_bytes = 100;
     result.response_info.fetch_duration = base::TimeDelta::FromMilliseconds(42);
     if (injected_response_) {
       result.response_body = std::make_unique<feedwire::Response>(

components/feed/core/v2/metrics_reporter.cc

@@ -52,7 +52,7 @@ void ReportUserActionHistogram(FeedUserActionType action_type) {
 MetricsReporter::MetricsReporter(const base::TickClock* clock,
                                  PrefService* profile_prefs)
     : clock_(clock), profile_prefs_(profile_prefs) {
-  persistent_data_ = prefs::GetPersistentMetricsData(profile_prefs_);
+  persistent_data_ = prefs::GetPersistentMetricsData(*profile_prefs_);
   ReportPersistentDataIfDayIsDone();
 }
 
@@ -259,7 +259,7 @@ void MetricsReporter::FinalizeMetrics() {
        iter != surfaces_waiting_for_more_content_.end();) {
     ReportGetMoreIfNeeded((iter++)->first, false);
   }
-  prefs::SetPersistentMetricsData(persistent_data_, profile_prefs_);
+  prefs::SetPersistentMetricsData(persistent_data_, *profile_prefs_);
 }
 
 void MetricsReporter::ReportOpenFeedIfNeeded(SurfaceId surface_id,
@@ -346,8 +346,8 @@ void MetricsReporter::NetworkRequestComplete(NetworkRequestType type,
 
 void MetricsReporter::OnLoadStream(LoadStreamStatus load_from_store_status,
                                    LoadStreamStatus final_status) {
-  DVLOG(1) << "OnLoadStream load_from_store_status=" << load_from_store_status
-           << " final_status=" << final_status;
+  LOG(ERROR) << "OnLoadStream load_from_store_status=" << load_from_store_status
+             << " final_status=" << final_status;
   base::UmaHistogramEnumeration(
       "ContentSuggestions.Feed.LoadStreamStatus.Initial", final_status);
   if (load_from_store_status != LoadStreamStatus::kNoStatus) {
@@ -375,7 +375,7 @@ void MetricsReporter::OnLoadMoreBegin(SurfaceId surface_id) {
 }
 
 void MetricsReporter::OnLoadMore(LoadStreamStatus status) {
-  DVLOG(1) << "OnLoadMore status=" << status;
+  LOG(ERROR) << "OnLoadMore status=" << status;
   base::UmaHistogramEnumeration(
       "ContentSuggestions.Feed.LoadStreamStatus.LoadMore", status);
 }
@@ -418,7 +418,7 @@ void MetricsReporter::ReportPersistentDataIfDayIsDone() {
   if (reset_data) {
     persistent_data_ = PersistentMetricsData();
     persistent_data_.current_day_start = base::Time::Now().LocalMidnight();
-    prefs::SetPersistentMetricsData(persistent_data_, profile_prefs_);
+    prefs::SetPersistentMetricsData(persistent_data_, *profile_prefs_);
   }
 }
 

components/feed/core/v2/prefs.cc

@@ -16,10 +16,10 @@
 namespace feed {
 namespace prefs {
 
-std::vector<int> GetThrottlerRequestCounts(PrefService* pref_service) {
+std::vector<int> GetThrottlerRequestCounts(PrefService& pref_service) {
   std::vector<int> result;
   const auto& value_list =
-      pref_service->GetList(kThrottlerRequestCountListPrefName)->GetList();
+      pref_service.GetList(kThrottlerRequestCountListPrefName)->GetList();
   for (const base::Value& value : value_list) {
     result.push_back(value.is_int() ? value.GetInt() : 0);
   }
@@ -27,50 +27,50 @@ std::vector<int> GetThrottlerRequestCounts(PrefService* pref_service) {
 }
 
 void SetThrottlerRequestCounts(std::vector<int> request_counts,
-                               PrefService* pref_service) {
+                               PrefService& pref_service) {
   std::vector<base::Value> value_list;
   for (int count : request_counts) {
     value_list.push_back(base::Value(count));
   }
 
-  pref_service->Set(kThrottlerRequestCountListPrefName,
-                    base::Value(std::move(value_list)));
+  pref_service.Set(kThrottlerRequestCountListPrefName,
+                   base::Value(std::move(value_list)));
 }
 
-base::Time GetLastRequestTime(PrefService* pref_service) {
-  return pref_service->GetTime(kThrottlerLastRequestTime);
+base::Time GetLastRequestTime(PrefService& pref_service) {
+  return pref_service.GetTime(kThrottlerLastRequestTime);
 }
 
-void SetLastRequestTime(base::Time request_time, PrefService* pref_service) {
-  return pref_service->SetTime(kThrottlerLastRequestTime, request_time);
+void SetLastRequestTime(base::Time request_time, PrefService& pref_service) {
+  return pref_service.SetTime(kThrottlerLastRequestTime, request_time);
 }
 
-DebugStreamData GetDebugStreamData(PrefService* pref_service) {
-  return DeserializeDebugStreamData(pref_service->GetString(kDebugStreamData))
+DebugStreamData GetDebugStreamData(PrefService& pref_service) {
+  return DeserializeDebugStreamData(pref_service.GetString(kDebugStreamData))
       .value_or(DebugStreamData());
 }
 
 void SetDebugStreamData(const DebugStreamData& data,
-                        PrefService* pref_service) {
-  pref_service->SetString(kDebugStreamData, SerializeDebugStreamData(data));
+                        PrefService& pref_service) {
+  pref_service.SetString(kDebugStreamData, SerializeDebugStreamData(data));
 }
 
 void SetRequestSchedule(const RequestSchedule& schedule,
-                        PrefService* pref_service) {
-  pref_service->Set(kRequestSchedule, RequestScheduleToValue(schedule));
+                        PrefService& pref_service) {
+  pref_service.Set(kRequestSchedule, RequestScheduleToValue(schedule));
 }
 
-RequestSchedule GetRequestSchedule(PrefService* pref_service) {
-  return RequestScheduleFromValue(*pref_service->Get(kRequestSchedule));
+RequestSchedule GetRequestSchedule(PrefService& pref_service) {
+  return RequestScheduleFromValue(*pref_service.Get(kRequestSchedule));
 }
 
 void SetPersistentMetricsData(const PersistentMetricsData& data,
-                              PrefService* pref_service) {
-  pref_service->Set(kMetricsData, PersistentMetricsDataToValue(data));
+                              PrefService& pref_service) {
+  pref_service.Set(kMetricsData, PersistentMetricsDataToValue(data));
 }
 
-PersistentMetricsData GetPersistentMetricsData(PrefService* pref_service) {
-  return PersistentMetricsDataFromValue(*pref_service->Get(kMetricsData));
+PersistentMetricsData GetPersistentMetricsData(PrefService& pref_service) {
+  return PersistentMetricsDataFromValue(*pref_service.Get(kMetricsData));
 }
 
 }  // namespace prefs

components/feed/core/v2/prefs.h

@@ -21,25 +21,25 @@ namespace prefs {
 
 // For counting previously made requests, one integer for each
 // |NetworkRequestType|.
-std::vector<int> GetThrottlerRequestCounts(PrefService* pref_service);
+std::vector<int> GetThrottlerRequestCounts(PrefService& pref_service);
 void SetThrottlerRequestCounts(std::vector<int> request_counts,
-                               PrefService* pref_service);
+                               PrefService& pref_service);
 
 // Time of the last request. For determining whether the next day's quota should
 // be released.
-base::Time GetLastRequestTime(PrefService* pref_service);
-void SetLastRequestTime(base::Time request_time, PrefService* pref_service);
+base::Time GetLastRequestTime(PrefService& pref_service);
+void SetLastRequestTime(base::Time request_time, PrefService& pref_service);
 
-DebugStreamData GetDebugStreamData(PrefService* pref_service);
-void SetDebugStreamData(const DebugStreamData& data, PrefService* pref_service);
+DebugStreamData GetDebugStreamData(PrefService& pref_service);
+void SetDebugStreamData(const DebugStreamData& data, PrefService& pref_service);
 
 void SetRequestSchedule(const RequestSchedule& schedule,
-                        PrefService* pref_service);
-RequestSchedule GetRequestSchedule(PrefService* pref_service);
+                        PrefService& pref_service);
+RequestSchedule GetRequestSchedule(PrefService& pref_service);
 
-PersistentMetricsData GetPersistentMetricsData(PrefService* pref_service);
+PersistentMetricsData GetPersistentMetricsData(PrefService& pref_service);
 void SetPersistentMetricsData(const PersistentMetricsData& data,
-                              PrefService* pref_service);
+                              PrefService& pref_service);
 
 }  // namespace prefs
 }  // namespace feed

components/feed/core/v2/proto_util.cc

@@ -159,11 +159,23 @@ bool CompareContentId(const feedwire::ContentId& a,
          std::tie(b.content_domain(), b_id, b_type);
 }
 
+bool CompareContent(const feedstore::Content& a, const feedstore::Content& b) {
+  const ContentId& a_id = a.content_id();
+  const ContentId& b_id = b.content_id();
+  if (a_id.id() < b_id.id())
+    return true;
+  if (a_id.id() > b_id.id())
+    return false;
+  if (a_id.type() < b_id.type())
+    return true;
+  if (a_id.type() > b_id.type())
+    return false;
+  return a.frame() < b.frame();
+}
+
 feedwire::ClientInfo CreateClientInfo(const RequestMetadata& request_metadata) {
   feedwire::ClientInfo client_info;
   // TODO(harringtond): Fill out client_instance_id.
-  // TODO(harringtond): Fill out advertising_id.
-  // TODO(harringtond): Fill out device_country.
 
   feedwire::DisplayInfo& display_info = *client_info.add_display_info();
   display_info.set_screen_density(request_metadata.display_metrics.density);
@@ -179,7 +191,7 @@ feedwire::ClientInfo CreateClientInfo(const RequestMetadata& request_metadata) {
 #elif defined(OS_IOS)
   client_info.set_platform_type(feedwire::ClientInfo::IOS);
 #endif
-  client_info.set_app_type(feedwire::ClientInfo::TEST_APP);
+  client_info.set_app_type(feedwire::ClientInfo::CLANK);
   *client_info.mutable_platform_version() = GetPlatformVersionMessage();
   *client_info.mutable_app_version() =
       GetAppVersionMessage(request_metadata.chrome_info);
@@ -206,8 +218,8 @@ feedwire::Request CreateFeedQueryLoadMoreRequest(
 }  // namespace feed
 
 namespace feedstore {
-void SetLastAddedTime(base::Time t, feedstore::StreamData* data) {
-  data->set_last_added_time_millis(
+void SetLastAddedTime(base::Time t, feedstore::StreamData& data) {
+  data.set_last_added_time_millis(
       (t - base::Time::UnixEpoch()).InMilliseconds());
 }
 

components/feed/core/v2/proto_util.h

@@ -17,6 +17,7 @@ namespace feedwire {
 class Request;
 }  // namespace feedwire
 namespace feedstore {
+class Content;
 class StreamData;
 }  // namespace feedstore
 
@@ -29,6 +30,7 @@ std::string ContentIdString(const feedwire::ContentId&);
 bool Equal(const feedwire::ContentId& a, const feedwire::ContentId& b);
 bool CompareContentId(const feedwire::ContentId& a,
                       const feedwire::ContentId& b);
+bool CompareContent(const feedstore::Content& a, const feedstore::Content& b);
 
 class ContentIdCompareFunctor {
  public:
@@ -38,6 +40,14 @@ class ContentIdCompareFunctor {
   }
 };
 
+class ContentCompareFunctor {
+ public:
+  bool operator()(const feedstore::Content& a,
+                  const feedstore::Content& b) const {
+    return CompareContent(a, b);
+  }
+};
+
 feedwire::ClientInfo CreateClientInfo(const RequestMetadata& request_metadata);
 
 feedwire::Request CreateFeedQueryRefreshRequest(
@@ -54,7 +64,7 @@ feedwire::Request CreateFeedQueryLoadMoreRequest(
 
 namespace feedstore {
 
-void SetLastAddedTime(base::Time t, feedstore::StreamData* data);
+void SetLastAddedTime(base::Time t, feedstore::StreamData& data);
 base::Time GetLastAddedTime(const feedstore::StreamData& data);
 
 }  // namespace feedstore

components/feed/core/v2/proto_util_unittest.cc

@@ -23,8 +23,7 @@ TEST(ProtoUtilTest, CreateClientInfo) {
   request_metadata.language_tag = "en-US";
 
   feedwire::ClientInfo result = CreateClientInfo(request_metadata);
-  // TODO(harringtond): change back to CHROME when it is supported.
-  EXPECT_EQ(feedwire::ClientInfo::TEST_APP, result.app_type());
+  EXPECT_EQ(feedwire::ClientInfo::CLANK, result.app_type());
   EXPECT_EQ(feedwire::Version::RELEASE, result.app_version().build_type());
   EXPECT_EQ(1, result.app_version().major());
   EXPECT_EQ(2, result.app_version().minor());

components/feed/core/v2/protocol_translator.cc

@@ -87,10 +87,10 @@ struct ConvertedDataOperation {
 };
 
 bool TranslateFeature(feedwire::Feature* feature,
-                      ConvertedDataOperation* result) {
+                      ConvertedDataOperation& result) {
   feedstore::StreamStructure::Type type =
       TranslateNodeType(feature->renderable_unit());
-  result->stream_structure.set_type(type);
+  result.stream_structure.set_type(type);
 
   if (type == feedstore::StreamStructure::CONTENT) {
     feedwire::Content* wire_content = feature->mutable_content_extension();
@@ -101,10 +101,10 @@ bool TranslateFeature(feedwire::Feature* feature,
     // TODO(iwells): We still need score, availability_time_seconds,
     // offline_metadata, and representation_data to populate content_info.
 
-    result->content.emplace();
-    *(result->content->mutable_content_id()) =
-        result->stream_structure.content_id();
-    result->content->set_allocated_frame(
+    result.content.emplace();
+    *(result.content->mutable_content_id()) =
+        result.stream_structure.content_id();
+    result.content->set_allocated_frame(
         wire_content->mutable_xsurface_content()->release_xsurface_output());
   }
   return true;
@@ -112,26 +112,26 @@ bool TranslateFeature(feedwire::Feature* feature,
 
 base::Optional<feedstore::StreamSharedState> TranslateSharedState(
     feedwire::ContentId content_id,
-    feedwire::RenderData* wire_shared_state) {
-  if (wire_shared_state->render_data_type() != feedwire::RenderData::XSURFACE) {
+    feedwire::RenderData& wire_shared_state) {
+  if (wire_shared_state.render_data_type() != feedwire::RenderData::XSURFACE) {
     return base::nullopt;
   }
 
   feedstore::StreamSharedState shared_state;
   *shared_state.mutable_content_id() = std::move(content_id);
   shared_state.set_allocated_shared_state_data(
-      wire_shared_state->mutable_xsurface_container()->release_render_data());
+      wire_shared_state.mutable_xsurface_container()->release_render_data());
   return shared_state;
 }
 
 bool TranslatePayload(base::Time now,
                       feedwire::DataOperation operation,
                       ConvertedGlobalData* global_data,
-                      ConvertedDataOperation* result) {
+                      ConvertedDataOperation& result) {
   switch (operation.payload_case()) {
     case feedwire::DataOperation::kFeature: {
       feedwire::Feature* feature = operation.mutable_feature();
-      result->stream_structure.set_allocated_parent_id(
+      result.stream_structure.set_allocated_parent_id(
           feature->release_parent_id());
 
       if (!TranslateFeature(feature, result))
@@ -139,16 +139,16 @@ bool TranslatePayload(base::Time now,
     } break;
     case feedwire::DataOperation::kNextPageToken: {
       feedwire::Token* token = operation.mutable_next_page_token();
-      result->stream_structure.set_allocated_parent_id(
+      result.stream_structure.set_allocated_parent_id(
           token->release_parent_id());
-      result->next_page_token = std::move(
+      result.next_page_token = std::move(
           *token->mutable_next_page_token()->mutable_next_page_token());
     } break;
     case feedwire::DataOperation::kRenderData: {
-      result->shared_state =
-          TranslateSharedState(result->stream_structure.content_id(),
-                               operation.mutable_render_data());
-      if (!result->shared_state)
+      result.shared_state =
+          TranslateSharedState(result.stream_structure.content_id(),
+                               *operation.mutable_render_data());
+      if (!result.shared_state)
         return false;
     } break;
     case feedwire::DataOperation::kRequestSchedule: {
@@ -186,7 +186,7 @@ base::Optional<ConvertedDataOperation> TranslateDataOperationInternal(
       result.stream_structure.set_allocated_content_id(
           operation.mutable_metadata()->release_content_id());
 
-      if (!TranslatePayload(now, std::move(operation), global_data, &result))
+      if (!TranslatePayload(now, std::move(operation), global_data, result))
         return base::nullopt;
       break;
 
@@ -282,13 +282,15 @@ RefreshResponseData TranslateWireResponse(
     }
   }
 
-  // TODO(harringtond): If there's more than one shared state, record some
-  // sort of error.
   if (!result->shared_states.empty()) {
+    if (result->shared_states.size() > 1) {
+      DLOG(ERROR)
+          << "Receieved more than one shared state. Only the first is used.";
+    }
     *result->stream_data.mutable_shared_state_id() =
         result->shared_states.front().content_id();
   }
-  feedstore::SetLastAddedTime(current_time, &result->stream_data);
+  feedstore::SetLastAddedTime(current_time, result->stream_data);
 
   RefreshResponseData response_data;
   response_data.model_update_request = std::move(result);

components/feed/core/v2/public/types.h

@@ -34,6 +34,11 @@ using EphemeralChangeId = util::IdTypeU32<class EphemeralChangeIdClass>;
 using SurfaceId = util::IdTypeU32<class SurfaceIdClass>;
 
 struct NetworkResponseInfo {
+  NetworkResponseInfo();
+  ~NetworkResponseInfo();
+  NetworkResponseInfo(const NetworkResponseInfo&);
+  NetworkResponseInfo& operator=(const NetworkResponseInfo&);
+
   // A union of net::Error (if the request failed) and the http
   // status code(if the request succeeded in reaching the server).
   int32_t status_code = 0;
@@ -41,6 +46,7 @@ struct NetworkResponseInfo {
   base::Time fetch_time;
   std::string bless_nonce;
   GURL base_request_url;
+  size_t response_body_bytes = 0;
 };
 
 // For the snippets-internals page.

components/feed/core/v2/request_throttler.cc

@@ -47,7 +47,7 @@ bool RequestThrottler::RequestQuota(NetworkRequestType request_type) {
   // Fetch request counts from prefs. There's an entry for each request type.
   // We may need to resize the list.
   std::vector<int> request_counts =
-      feed::prefs::GetThrottlerRequestCounts(pref_service_);
+      feed::prefs::GetThrottlerRequestCounts(*pref_service_);
   const size_t request_count_index = static_cast<size_t>(request_type);
   if (request_counts.size() <= request_count_index)
     request_counts.resize(request_count_index + 1);
@@ -56,7 +56,7 @@ bool RequestThrottler::RequestQuota(NetworkRequestType request_type) {
   if (requests_already_made >= max_requests_per_day)
     return false;
   requests_already_made++;
-  feed::prefs::SetThrottlerRequestCounts(request_counts, pref_service_);
+  feed::prefs::SetThrottlerRequestCounts(request_counts, *pref_service_);
   return true;
 }
 
@@ -65,12 +65,12 @@ void RequestThrottler::ResetCountersIfDayChanged() {
   // making un-throttled requests to server.
   const base::Time now = clock_->Now();
   const bool day_changed =
-      DaysSinceOrigin(feed::prefs::GetLastRequestTime(pref_service_)) !=
+      DaysSinceOrigin(feed::prefs::GetLastRequestTime(*pref_service_)) !=
       DaysSinceOrigin(now);
-  feed::prefs::SetLastRequestTime(now, pref_service_);
+  feed::prefs::SetLastRequestTime(now, *pref_service_);
 
   if (day_changed)
-    feed::prefs::SetThrottlerRequestCounts({}, pref_service_);
+    feed::prefs::SetThrottlerRequestCounts({}, *pref_service_);
 }
 
 }  // namespace feed

components/feed/core/v2/stream_model.cc

@@ -116,9 +116,6 @@ void StreamModel::Update(
   }
 
   // Update non-tree data.
-  // TODO(harringtond): Once we start using StreamData.next_action_id, this line
-  // would be problematic. We should probably move next_action_id into a
-  // different record in FeedStore.
   stream_data_ = update_request->stream_data;
 
   if (has_clear_all) {
@@ -142,11 +139,7 @@ void StreamModel::Update(
     }
   }
 
-  // TODO(harringtond): Some StreamData fields not yet used.
-  //    next_action_id - do we need to load the model before uploading
-  //         actions? If not, we probably will want to move this out of
-  //         StreamData.
-  //    content_id - probably just ignore for now
+  // TODO(harringtond): We're not using StreamData's content_id for anything.
 
   UpdateFlattenedTree();
 }

components/feed/core/v2/stream_model.h

@@ -134,8 +134,8 @@ class StreamModel {
 
   Observer* observer_ = nullptr;  // Unowned.
   StoreObserver* store_observer_ = nullptr;  // Unowned.
-  stream_model::ContentIdMap id_map_;
-  stream_model::FeatureTree base_feature_tree_{&id_map_};
+  stream_model::ContentMap content_map_;
+  stream_model::FeatureTree base_feature_tree_{&content_map_};
   // |base_feature_tree_| with |ephemeral_changes_| applied.
   // Null if there are no ephemeral changes.
   std::unique_ptr<stream_model::FeatureTree> feature_tree_after_changes_;

components/feed/core/v2/stream_model/ephemeral_change.cc

@@ -41,7 +41,7 @@ std::unique_ptr<FeatureTree> ApplyEphemeralChanges(
         tree_with_changes->ApplyStreamStructure(operation.structure());
       }
       if (operation.has_content()) {
-        tree_with_changes->AddContent(operation.content());
+        tree_with_changes->CopyAndAddContent(operation.content());
       }
     }
   }

components/feed/core/v2/stream_model/feature_tree.cc

@@ -12,10 +12,10 @@
 namespace feed {
 namespace stream_model {
 
-ContentIdMap::ContentIdMap() = default;
-ContentIdMap::~ContentIdMap() = default;
+ContentMap::ContentMap() = default;
+ContentMap::~ContentMap() = default;
 
-ContentTag ContentIdMap::GetContentTag(const feedwire::ContentId& id) {
+ContentTag ContentMap::GetContentTag(const feedwire::ContentId& id) {
   auto iter = mapping_.find(id);
   if (iter != mapping_.end())
     return iter->second;
@@ -24,8 +24,42 @@ ContentTag ContentIdMap::GetContentTag(const feedwire::ContentId& id) {
   return tag;
 }
 
-ContentRevision ContentIdMap::NextContentRevision() {
-  return revision_generator_.GenerateNextId();
+const feedstore::Content* ContentMap::FindContent(
+    ContentRevision content_revision) {
+  const size_t index = content_revision.GetUnsafeValue();
+  if (revision_to_content_.size() <= index) {
+    return nullptr;
+  }
+  return revision_to_content_[index];
+}
+
+ContentRevision ContentMap::LookupContentRevision(
+    const feedstore::Content& content) {
+  auto iter = content_.find(content);
+  return (iter != content_.end()) ? iter->second : ContentRevision();
+}
+
+ContentRevision ContentMap::AddContent(feedstore::Content content) {
+  auto result = content_.emplace(std::move(content), ContentRevision());
+  // Already exists
+  if (!result.second)
+    return result.first->second;
+
+  // Newly inserted.
+  const ContentRevision new_revision = revision_generator_.GenerateNextId();
+  result.first->second = new_revision;
+  if (revision_to_content_.size() <= new_revision.GetUnsafeValue()) {
+    revision_to_content_.resize(new_revision.GetUnsafeValue() + 1);
+  }
+  revision_to_content_[new_revision.GetUnsafeValue()] = &result.first->first;
+  return new_revision;
+}
+
+void ContentMap::Clear() {
+  // We don't clear the ID generators, so no IDs are re-used.
+  mapping_.clear();
+  content_.clear();
+  revision_to_content_.clear();
 }
 
 StreamNode::StreamNode() = default;
@@ -33,11 +67,11 @@ StreamNode::~StreamNode() = default;
 StreamNode::StreamNode(const StreamNode&) = default;
 StreamNode& StreamNode::operator=(const StreamNode&) = default;
 
-FeatureTree::FeatureTree(ContentIdMap* id_map) : id_map_(id_map) {}
+FeatureTree::FeatureTree(ContentMap* content_map) : content_map_(content_map) {}
 
 FeatureTree::FeatureTree(const FeatureTree* base)
     : base_(base),
-      id_map_(base->id_map_),
+      content_map_(base->content_map_),
       computed_root_(base->computed_root_),
       root_tag_(base->root_tag_),
       nodes_(base->nodes_) {}
@@ -59,10 +93,7 @@ StreamNode* FeatureTree::FindNode(ContentTag id) {
 }
 
 const feedstore::Content* FeatureTree::FindContent(ContentRevision id) const {
-  auto iter = content_.find(id);
-  if (iter != content_.end())
-    return &iter->second;
-  return base_ ? base_->FindContent(id) : nullptr;
+  return content_map_->FindContent(id);
 }
 
 void FeatureTree::ApplyStreamStructure(
@@ -70,7 +101,11 @@ void FeatureTree::ApplyStreamStructure(
   switch (structure.operation()) {
     case feedstore::StreamStructure::CLEAR_ALL:
       nodes_.clear();
-      content_.clear();
+      // Clearing content is not required for correctness, but we can do it to
+      // free memory as long as there's no base feature tree that can reference
+      // the content.
+      if (!base_)
+        content_map_->Clear();
       computed_root_ = false;
       break;
     case feedstore::StreamStructure::UPDATE_OR_APPEND: {
@@ -126,19 +161,19 @@ void FeatureTree::ResizeNodesIfNeeded(ContentTag id) {
 }
 
 void FeatureTree::AddContent(feedstore::Content content) {
-  AddContent(id_map_->NextContentRevision(), std::move(content));
+  const ContentTag tag = GetContentTag(content.content_id());
+  const ContentRevision revision_id =
+      content_map_->AddContent(std::move(content));
+  GetOrMakeNode(tag)->content_revision = revision_id;
 }
 
-void FeatureTree::AddContent(ContentRevision revision_id,
-                             feedstore::Content content) {
-  // TODO(harringtond): Consider de-duping content.
-  // Currently, we copy content for ephemeral changes. Both when the ephemeral
-  // change is created, and when it is committed. We should consider eliminating
-  // these copies.
+void FeatureTree::CopyAndAddContent(const feedstore::Content& content) {
+  ContentRevision revision_id = content_map_->LookupContentRevision(content);
+  if (revision_id.is_null()) {
+    revision_id = content_map_->AddContent(content);
+  }
   const ContentTag tag = GetContentTag(content.content_id());
-  DCHECK(!content_.count(revision_id));
   GetOrMakeNode(tag)->content_revision = revision_id;
-  content_[revision_id] = std::move(content);
 }
 
 void FeatureTree::ResolveRoot() {

components/feed/core/v2/stream_model/feature_tree.h

@@ -18,25 +18,36 @@
 namespace feed {
 namespace stream_model {
 
-// Uniquely identifies a feedwire::ContentId. Provided by |ContentIdMap|.
+// Uniquely identifies a feedwire::ContentId. Provided by |ContentMap|.
 using ContentTag = util::IdTypeU32<class ContentTagClass>;
 using ContentRevision = feed::ContentRevision;
 
-// Maps ContentId into ContentTag, and generates ContentRevision IDs.
-class ContentIdMap {
+// Owns instances of feedstore::Content pointed to by the feature tree, and
+// maps ContentId into ContentTag.
+class ContentMap {
  public:
-  ContentIdMap();
-  ~ContentIdMap();
-  ContentIdMap(const ContentIdMap&) = delete;
-  ContentIdMap& operator=(const ContentIdMap&) = delete;
+  ContentMap();
+  ~ContentMap();
+  ContentMap(const ContentMap&) = delete;
+  ContentMap& operator=(const ContentMap&) = delete;
 
   ContentTag GetContentTag(const feedwire::ContentId& id);
-  ContentRevision NextContentRevision();
+
+  const feedstore::Content* FindContent(ContentRevision content_revision);
+  ContentRevision LookupContentRevision(const feedstore::Content& content);
+  ContentRevision AddContent(feedstore::Content content);
+
+  void Clear();
 
  private:
   ContentTag::Generator tag_generator_;
   ContentRevision::Generator revision_generator_;
   std::map<feedwire::ContentId, ContentTag, ContentIdCompareFunctor> mapping_;
+
+  // These two containers work together to store and index content.
+  // Each unique piece of content is stored once, and never removed.
+  std::map<feedstore::Content, ContentRevision, ContentCompareFunctor> content_;
+  std::vector<const feedstore::Content*> revision_to_content_;
 };
 
 // A node in FeatureTree.
@@ -61,7 +72,7 @@ struct StreamNode {
 };
 
 // The feature tree which underlies StreamModel.
-// This tree is different that most, the rules are as follows:
+// This tree is different than most, the rules are as follows:
 // * A node may or may not have a parent, so this is more of a forest than a
 //   tree.
 // * When nodes are removed, their set of children are remembered. If the node
@@ -78,7 +89,7 @@ class FeatureTree {
  public:
   // Constructor. |id_map| is retained by FeatureTree, and must have a greater
   // scope than FeatureTree.
-  explicit FeatureTree(ContentIdMap* id_map);
+  explicit FeatureTree(ContentMap* id_map);
   // Create a |FeatureTree| which starts as a copy of |base|.
   // Copies structure from |base|, and keeps a reference for content access.
   explicit FeatureTree(const FeatureTree* base);
@@ -90,8 +101,11 @@ class FeatureTree {
   // Mutations.
 
   void ApplyStreamStructure(const feedstore::StreamStructure& structure);
+  // Adds |content| to the tree.
   void AddContent(feedstore::Content content);
-  void AddContent(ContentRevision revision_id, feedstore::Content content);
+  // Same as |AddContent()|, but can avoid a copy of |content| if it already
+  // exists.
+  void CopyAndAddContent(const feedstore::Content& content);
 
   // Data access.
 
@@ -99,7 +113,7 @@ class FeatureTree {
   StreamNode* FindNode(ContentTag id);
   const feedstore::Content* FindContent(ContentRevision id) const;
   ContentTag GetContentTag(const feedwire::ContentId& id) {
-    return id_map_->GetContentTag(id);
+    return content_map_->GetContentTag(id);
   }
 
   // Returns the list of content that should be visible.
@@ -115,7 +129,7 @@ class FeatureTree {
   bool RemoveFromParent(StreamNode* parent, ContentTag node_id);
 
   const FeatureTree* base_ = nullptr;  // Unowned.
-  ContentIdMap* id_map_;               // Unowned.
+  ContentMap* content_map_;            // Unowned.
   // Finding the root:
   // We pick the root node as the last STREAM node which has no parent.
   // In most cases, we can identify the root as the tree is built.
@@ -126,9 +140,6 @@ class FeatureTree {
   // All nodes in the forest, included removed nodes.
   // This datastructure was selected to make copies efficient.
   std::vector<StreamNode> nodes_;
-  // TODO(harringtond): It may be possible to remove old revisions of content
-  // to save memory.
-  std::map<ContentRevision, feedstore::Content> content_;
 };
 
 }  // namespace stream_model

components/feed/core/v2/surface_updater.cc

@@ -134,6 +134,7 @@ feedui::ZeroStateSlice::Type GetZeroStateType(LoadStreamStatus status) {
     case LoadStreamStatus::kProtoTranslationFailed:
     case LoadStreamStatus::kCannotLoadFromNetworkOffline:
     case LoadStreamStatus::kCannotLoadFromNetworkThrottled:
+    case LoadStreamStatus::kNetworkFetchFailed:
       return feedui::ZeroStateSlice::CANT_REFRESH;
     case LoadStreamStatus::kNoStatus:
     case LoadStreamStatus::kLoadedFromStore:

components/feed/core/v2/tasks/load_stream_from_store_task.cc

@@ -70,8 +70,6 @@ void LoadStreamFromStoreTask::LoadStreamDone(
     }
   }
 
-  // TODO(harringtond): Add other failure cases?
-
   std::vector<ContentId> referenced_content_ids;
   for (const feedstore::StreamStructureSet& structure_set :
        result.stream_structures) {

components/feed/core/v2/tasks/load_stream_task.cc

@@ -123,9 +123,14 @@ void LoadStreamTask::QueryRequestComplete(
 
   network_response_info_ = result.response_info;
 
+  if (result.response_info.status_code != 200)
+    return Done(LoadStreamStatus::kNetworkFetchFailed);
+
   if (!result.response_body) {
-    Done(LoadStreamStatus::kNoResponseBody);
-    return;
+    if (result.response_info.response_body_bytes > 0)
+      return Done(LoadStreamStatus::kCannotParseNetworkResponseBody);
+    else
+      return Done(LoadStreamStatus::kNoResponseBody);
   }
 
   RefreshResponseData response_data =
@@ -133,10 +138,8 @@ void LoadStreamTask::QueryRequestComplete(
           *result.response_body,
           StreamModelUpdateRequest::Source::kNetworkUpdate,
           stream_->GetClock()->Now());
-  if (!response_data.model_update_request) {
-    Done(LoadStreamStatus::kProtoTranslationFailed);
-    return;
-  }
+  if (!response_data.model_update_request)
+    return Done(LoadStreamStatus::kProtoTranslationFailed);
 
   stream_->GetStore()->OverwriteStream(
       std::make_unique<StreamModelUpdateRequest>(

components/feed/core/v2/test/stream_builder.cc

@@ -167,7 +167,7 @@ std::unique_ptr<StreamModelUpdateRequest> MakeTypicalInitialModelState(
   *initial_update->stream_data.mutable_content_id() = MakeRootId();
   *initial_update->stream_data.mutable_shared_state_id() = MakeSharedStateId(i);
   initial_update->stream_data.set_next_page_token("page-2");
-  SetLastAddedTime(last_added_time, &initial_update->stream_data);
+  SetLastAddedTime(last_added_time, initial_update->stream_data);
 
   return initial_update;
 }
@@ -193,7 +193,7 @@ std::unique_ptr<StreamModelUpdateRequest> MakeTypicalNextPageState(
   *initial_update->stream_data.mutable_shared_state_id() = MakeSharedStateId(0);
   initial_update->stream_data.set_next_page_token(
       "page-" + base::NumberToString(page_number + 1));
-  SetLastAddedTime(last_added_time, &initial_update->stream_data);
+  SetLastAddedTime(last_added_time, initial_update->stream_data);
 
   return initial_update;
 }

components/feed/core/v2/testdata/request.textproto

@@ -8,8 +8,7 @@ feed_request {
       build_type: DEV
       api_version: 29
     }
-    # TODO(iwells): Change this to CLANK when possible.
-    app_type: TEST_APP
+    app_type: CLANK
     app_version {
       major: 79
       minor: 0

components/feed/core/v2/types.cc

@@ -68,6 +68,12 @@ base::Optional<DebugStreamData> UnpickleDebugStreamData(
 
 }  // namespace
 
+NetworkResponseInfo::NetworkResponseInfo() = default;
+NetworkResponseInfo::~NetworkResponseInfo() = default;
+NetworkResponseInfo::NetworkResponseInfo(const NetworkResponseInfo&) = default;
+NetworkResponseInfo& NetworkResponseInfo::operator=(
+    const NetworkResponseInfo&) = default;
+
 std::string ToString(ContentRevision c) {
   return base::NumberToString(c.value());
 }

tools/metrics/histograms/enums.xml

@@ -27770,6 +27770,8 @@ Called by update_feature_policy_enum.py.-->
       label="Failed - cannot load more content because the model is not yet
              loaded"/>
   <int value="17" label="Failed - feed is disabled by enterprise policy"/>
+  <int value="18"
+      label="Failed - network fetch failed or returned a non-200 status"/>
 </enum>
 
 <enum name="FeedRequestReason">