Add support for zero-state

- Added new ZeroStateSlice
- The UI now should receive a zero state slice if
  loading fails.

I've changed SurfaceInterface so that the initial state isn't provided
on a different function call. This makes things a bit easier in
FeedStream.


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

components/feed/core/proto/v2/ui.proto

@@ -38,10 +38,25 @@ message StreamUpdate {
 
 // A horizontal slice of UI to be presented in the vertical-scrolling feed.
 message Slice {
-  oneof SliceData { XSurfaceSlice xsurface_slice = 1; }
+  oneof SliceData {
+    XSurfaceSlice xsurface_slice = 1;
+    ZeroStateSlice zero_state_slice = 3;
+  }
   string slice_id = 2;
 }
 
+// This slice is sent when no feed data can be loaded.
+message ZeroStateSlice {
+  enum Type {
+    UNKNOWN = 0;
+    // A generic error that explains there are no cards available.
+    NO_CARDS_AVAILABLE = 1;
+    // An error indicating there were problems refreshing the feed.
+    CANT_REFRESH = 2;
+  };
+  Type type = 1;
+}
+
 message XSurfaceSlice {
   bytes xsurface_frame = 1;
 }

components/feed/core/v2/feed_stream.cc

@@ -30,37 +30,51 @@
 namespace feed {
 
 // Tracks UI changes in |StreamModel| and forwards them to |SurfaceInterface|s.
-// Has the same lifetime as |StreamModel|.
-class FeedStream::ModelMonitor : public StreamModel::Observer {
+// TODO(harringtond): implement spinner slice.
+class FeedStream::SurfaceUpdater : public StreamModel::Observer {
  public:
   using ContentRevision = ContentRevision;
-  ModelMonitor(StreamModel* model,
-               const base::ObserverList<SurfaceInterface>* surfaces)
-      : model_(model), surfaces_(surfaces) {
-    model_->SetObserver(this);
-    const std::vector<ContentRevision>& content_list = model_->GetContentList();
-    current_content_set_.insert(content_list.begin(), content_list.end());
+  explicit SurfaceUpdater(const base::ObserverList<SurfaceInterface>* surfaces)
+      : surfaces_(surfaces) {}
+  ~SurfaceUpdater() override = default;
+  SurfaceUpdater(const SurfaceUpdater&) = delete;
+  SurfaceUpdater& operator=(const SurfaceUpdater&) = delete;
+
+  void SetModel(StreamModel* model) {
+    if (model_ == model)
+      return;
+    if (model_)
+      model_->SetObserver(nullptr);
+    model_ = model;
+    if (model_) {
+      model_->SetObserver(this);
+
+      const std::vector<ContentRevision>& content_list =
+          model_->GetContentList();
+      current_content_set_.insert(content_list.begin(), content_list.end());
+      for (SurfaceInterface& surface : *surfaces_) {
+        surface.StreamUpdate(GetUpdateForNewSurface(*model_));
+      }
+    }
   }
-  ~ModelMonitor() override = default;
-  ModelMonitor(const ModelMonitor&) = delete;
-  ModelMonitor& operator=(const ModelMonitor&) = delete;
 
   // StreamModel::Observer.
   void OnUiUpdate(const StreamModel::UiUpdate& update) override {
-    // TODO(harringtond): add shared states too.
+    DCHECK(model_);  // The update comes from the model.
+
     if (!update.content_list_changed)
       return;
     feedui::StreamUpdate stream_update;
     const std::vector<ContentRevision>& content_list = model_->GetContentList();
     for (ContentRevision content_revision : content_list) {
-      AddSliceUpdate(content_revision,
+      AddSliceUpdate(*model_, content_revision,
                      current_content_set_.count(content_revision) == 0,
                      &stream_update);
     }
     for (const StreamModel::UiUpdate::SharedStateInfo& info :
          update.shared_states) {
       if (info.updated)
-        AddSharedState(info.shared_state_id, &stream_update);
+        AddSharedState(*model_, info.shared_state_id, &stream_update);
     }
 
     current_content_set_.clear();
@@ -73,7 +87,28 @@ class FeedStream::ModelMonitor : public StreamModel::Observer {
 
   // Sends the initial stream state to a newly connected surface.
   void SurfaceAdded(SurfaceInterface* surface) {
-    surface->InitialStreamState(GetUpdateForNewSurface());
+    if (model_) {
+      surface->StreamUpdate(GetUpdateForNewSurface(*model_));
+    }
+  }
+
+  void LoadStreamFailed(LoadStreamStatus load_stream_status) {
+    auto zero_state_type = feedui::ZeroStateSlice::NO_CARDS_AVAILABLE;
+    switch (load_stream_status) {
+      case LoadStreamStatus::kProtoTranslationFailed:
+      case LoadStreamStatus::kNoResponseBody:
+      case LoadStreamStatus::kCannotLoadFromNetworkOffline:
+      case LoadStreamStatus::kCannotLoadFromNetworkThrottled:
+        zero_state_type = feedui::ZeroStateSlice::CANT_REFRESH;
+        break;
+      default:
+        break;
+    }
+    // Note that with multiple surface, it's possible that we send a zero-state
+    // to a single surface multiple times.
+    for (SurfaceInterface& surface : *surfaces_) {
+      SendZeroStateUpdate(zero_state_type, &surface);
+    }
   }
 
  private:
@@ -83,10 +118,32 @@ class FeedStream::ModelMonitor : public StreamModel::Observer {
                        sizeof(integer_value));
   }
 
-  void AddSharedState(const std::string& shared_state_id,
-                      feedui::StreamUpdate* stream_update) {
+  static feedui::StreamUpdate GetUpdateForNewSurface(const StreamModel& model) {
+    feedui::StreamUpdate result;
+    for (ContentRevision content_revision : model.GetContentList()) {
+      AddSliceUpdate(model, content_revision, /*is_content_new=*/true, &result);
+    }
+    for (std::string& id : model.GetSharedStateIds()) {
+      AddSharedState(model, id, &result);
+    }
+
+    return result;
+  }
+
+  static void SendZeroStateUpdate(feedui::ZeroStateSlice::Type zero_state_type,
+                                  SurfaceInterface* surface) {
+    feedui::StreamUpdate update;
+    feedui::Slice* slice = update.add_updated_slices()->mutable_slice();
+    slice->mutable_zero_state_slice()->set_type(zero_state_type);
+    slice->set_slice_id("zero-state");
+    surface->StreamUpdate(update);
+  }
+
+  static void AddSharedState(const StreamModel& model,
+                             const std::string& shared_state_id,
+                             feedui::StreamUpdate* stream_update) {
     const std::string* shared_state_data =
-        model_->FindSharedStateData(shared_state_id);
+        model.FindSharedStateData(shared_state_id);
     if (!shared_state_data)
       return;
     feedui::SharedState* added_shared_state =
@@ -95,14 +152,15 @@ class FeedStream::ModelMonitor : public StreamModel::Observer {
     added_shared_state->set_xsurface_shared_state(*shared_state_data);
   }
 
-  void AddSliceUpdate(ContentRevision content_revision,
-                      bool is_content_new,
-                      feedui::StreamUpdate* stream_update) {
+  static void AddSliceUpdate(const StreamModel& model,
+                             ContentRevision content_revision,
+                             bool is_content_new,
+                             feedui::StreamUpdate* stream_update) {
     if (is_content_new) {
       feedui::Slice* slice =
           stream_update->add_updated_slices()->mutable_slice();
       slice->set_slice_id(ToSliceId(content_revision));
-      const feedstore::Content* content = model_->FindContent(content_revision);
+      const feedstore::Content* content = model.FindContent(content_revision);
       DCHECK(content);
       slice->mutable_xsurface_slice()->set_xsurface_frame(content->frame());
     } else {
@@ -111,20 +169,9 @@ class FeedStream::ModelMonitor : public StreamModel::Observer {
     }
   }
 
-  feedui::StreamUpdate GetUpdateForNewSurface() {
-    feedui::StreamUpdate result;
-    for (ContentRevision content_revision : model_->GetContentList()) {
-      AddSliceUpdate(content_revision, /*is_content_new=*/true, &result);
-    }
-    for (std::string& id : model_->GetSharedStateIds()) {
-      AddSharedState(id, &result);
-    }
-
-    return result;
-  }
-
   // Owned by |FeedStream|.
-  StreamModel* model_;
+  // Warning!: Null when the model is not yet loaded.
+  StreamModel* model_ = nullptr;
   const base::ObserverList<SurfaceInterface>* surfaces_;
 
   std::set<ContentRevision> current_content_set_;
@@ -164,6 +211,8 @@ FeedStream::FeedStream(
   static WireResponseTranslator default_translator;
   wire_response_translator_ = &default_translator;
 
+  surface_updater_ = std::make_unique<SurfaceUpdater>(&surfaces_);
+
   // Inserting this task first ensures that |store_| is initialized before
   // it is used.
   task_queue_.AddTask(std::make_unique<WaitForStoreInitializeTask>(store_));
@@ -182,20 +231,18 @@ void FeedStream::InitializeScheduling() {
 FeedStream::~FeedStream() = default;
 
 void FeedStream::TriggerStreamLoad() {
-  if (model_monitor_ || model_loading_in_progress_)
+  if (model_ || model_loading_in_progress_)
     return;
 
-  if (!delegate_->IsEulaAccepted()) {
-    stream_event_observer_->OnLoadStream(
-        LoadStreamStatus::kNoStatus,
-        LoadStreamStatus::kLoadNotAllowedEulaNotAccepted);
+  // If we should not load the stream, abort and send a zero-state update.
+  if (!IsArticlesListVisible()) {
+    LoadStreamTaskComplete(LoadStreamTask::Result(
+        LoadStreamStatus::kLoadNotAllowedArticlesListHidden));
     return;
   }
-
-  if (!IsArticlesListVisible()) {
-    stream_event_observer_->OnLoadStream(
-        LoadStreamStatus::kNoStatus,
-        LoadStreamStatus::kLoadNotAllowedArticlesListHidden);
+  if (!delegate_->IsEulaAccepted()) {
+    LoadStreamTaskComplete(LoadStreamTask::Result(
+        LoadStreamStatus::kLoadNotAllowedEulaNotAccepted));
     return;
   }
 
@@ -212,15 +259,17 @@ void FeedStream::LoadStreamTaskComplete(LoadStreamTask::Result result) {
            << result.load_from_store_status
            << " final_status=" << result.final_status;
   model_loading_in_progress_ = false;
+
+  // If loading failed, update surfaces with an appropriate zero-state error.
+  if (!model_) {
+    surface_updater_->LoadStreamFailed(result.final_status);
+  }
 }
 
 void FeedStream::AttachSurface(SurfaceInterface* surface) {
   surfaces_.AddObserver(surface);
-  if (model_monitor_) {
-    model_monitor_->SurfaceAdded(surface);
-  } else {
-    TriggerStreamLoad();
-  }
+  surface_updater_->SurfaceAdded(surface);
+  TriggerStreamLoad();
 }
 
 void FeedStream::DetachSurface(SurfaceInterface* surface) {
@@ -385,18 +434,14 @@ void FeedStream::MaybeTriggerRefresh(TriggerType trigger,
 void FeedStream::LoadModel(std::unique_ptr<StreamModel> model) {
   DCHECK(!model_);
   model_ = std::move(model);
-  model_monitor_ =
-      std::make_unique<FeedStream::ModelMonitor>(model_.get(), &surfaces_);
   model_->SetStoreObserver(this);
-  for (SurfaceInterface& surface : surfaces_) {
-    model_monitor_->SurfaceAdded(&surface);
-  }
+  surface_updater_->SetModel(model_.get());
 }
 
 void FeedStream::UnloadModel() {
   if (!model_)
     return;
-  model_monitor_.reset();
+  surface_updater_->SetModel(nullptr);
   model_.reset();
 }
 

components/feed/core/v2/feed_stream.h

@@ -173,7 +173,7 @@ class FeedStream : public FeedStreamApi,
       std::unique_ptr<UserClassifier> user_classifier);
 
  private:
-  class ModelMonitor;
+  class SurfaceUpdater;
   class ModelStoreChangeMonitor;
   void MaybeTriggerRefresh(TriggerType trigger,
                            bool clear_all_before_refresh = false);
@@ -202,8 +202,7 @@ class FeedStream : public FeedStreamApi,
   // Whether the model is being loaded. Used to prevent multiple simultaneous
   // attempts to load the model.
   bool model_loading_in_progress_ = false;
-  // Monitors |model_|. Null when |model_| is null.
-  std::unique_ptr<ModelMonitor> model_monitor_;
+  std::unique_ptr<SurfaceUpdater> surface_updater_;
   // The stream model. Null if not yet loaded.
   // Internally, this should only be changed by |LoadModel()| and
   // |UnloadModel()|.

components/feed/core/v2/feed_stream_unittest.cc

@@ -95,11 +95,11 @@ std::string ModelStateFor(FeedStore* store) {
 class TestSurface : public FeedStream::SurfaceInterface {
  public:
   // FeedStream::SurfaceInterface.
-  void InitialStreamState(const feedui::StreamUpdate& stream_update) override {
-    initial_state = stream_update;
-  }
   void StreamUpdate(const feedui::StreamUpdate& stream_update) override {
+    if (!initial_state)
+      initial_state = stream_update;
     update = stream_update;
+    ++update_count_;
   }
 
   // Test functions.
@@ -107,10 +107,35 @@ class TestSurface : public FeedStream::SurfaceInterface {
   void Clear() {
     initial_state = base::nullopt;
     update = base::nullopt;
+    update_count_ = 0;
+  }
+
+  // Describe what is shown on the surface in a format that can be easily
+  // asserted against.
+  std::string Describe() {
+    if (!initial_state)
+      return "empty";
+
+    if (update->updated_slices().size() == 1 &&
+        update->updated_slices()[0].has_slice() &&
+        update->updated_slices()[0].slice().has_zero_state_slice()) {
+      return "zero-state";
+    }
+
+    std::stringstream ss;
+    ss << update->updated_slices().size() << " slices";
+    // If there's more than one update, we want to know that.
+    if (update_count_ > 1) {
+      ss << " " << update_count_ << " updates";
+    }
+    return ss.str();
   }
 
   base::Optional<feedui::StreamUpdate> initial_state;
   base::Optional<feedui::StreamUpdate> update;
+
+ private:
+  int update_count_ = 0;
 };
 
 class TestUserClassifier : public UserClassifier {
@@ -370,9 +395,9 @@ TEST_F(FeedStreamTest, SurfaceReceivesInitialContentLoadedAfterAttach) {
     stream_->LoadModelForTesting(std::move(model));
   }
 
-  ASSERT_TRUE(surface.initial_state);
+  ASSERT_EQ("2 slices", surface.Describe());
   const feedui::StreamUpdate& initial_state = surface.initial_state.value();
-  ASSERT_EQ(2, initial_state.updated_slices().size());
+
   EXPECT_NE("", initial_state.updated_slices(0).slice().slice_id());
   EXPECT_EQ("f:0", initial_state.updated_slices(0)
                        .slice()
@@ -407,7 +432,7 @@ TEST_F(FeedStreamTest, SurfaceReceivesUpdatedContent) {
   const feedui::StreamUpdate& initial_state = surface.initial_state.value();
   const feedui::StreamUpdate& update = surface.update.value();
 
-  ASSERT_EQ(2, update.updated_slices().size());
+  ASSERT_EQ("2 slices 2 updates", surface.Describe());
   // First slice is just an ID that matches the old 1st slice ID.
   EXPECT_EQ(initial_state.updated_slices(0).slice().slice_id(),
             update.updated_slices(0).slice_id());
@@ -433,7 +458,6 @@ TEST_F(FeedStreamTest, SurfaceReceivesSecondUpdatedContent) {
   });
 
   // Clear the last update and add #3.
-  surface.update = {};
   stream_->ExecuteOperations({
       MakeOperation(MakeCluster(3, MakeRootId())),
       MakeOperation(MakeContentNode(3, MakeClusterId(3))),
@@ -442,7 +466,7 @@ TEST_F(FeedStreamTest, SurfaceReceivesSecondUpdatedContent) {
 
   // The last update should have only one new piece of content.
   // This verifies the current content set is tracked properly.
-  ASSERT_TRUE(surface.update);
+  ASSERT_EQ("4 slices 3 updates", surface.Describe());
 
   ASSERT_EQ(4, surface.update->updated_slices().size());
   EXPECT_FALSE(surface.update->updated_slices(0).has_slice());
@@ -464,6 +488,7 @@ TEST_F(FeedStreamTest, DetachSurface) {
   stream_->AttachSurface(&surface);
   EXPECT_TRUE(surface.initial_state);
   stream_->DetachSurface(&surface);
+  surface.Clear();
 
   // Arbitrary stream change. Surface should not see the update.
   stream_->ExecuteOperations({
@@ -481,8 +506,7 @@ TEST_F(FeedStreamTest, LoadFromNetwork) {
 
   EXPECT_TRUE(network_.query_request_sent);
   EXPECT_TRUE(response_translator_.InjectedResponseConsumed());
-  ASSERT_TRUE(surface.initial_state);
-  ASSERT_EQ(2, surface.initial_state->updated_slices().size());
+  EXPECT_EQ("2 slices", surface.Describe());
   // Verify the model is filled correctly.
   EXPECT_STRINGS_EQUAL(ModelStateFor(MakeTypicalInitialModelState()),
                        stream_->GetModel()->DumpStateForTesting());
@@ -530,10 +554,10 @@ TEST_F(FeedStreamTest, DoNotLoadFromNetworkWhenOffline) {
   TestSurface surface;
   stream_->AttachSurface(&surface);
   WaitForIdleTaskQueue();
-  // TODO(harringtond): Implement zero-state and check that it is triggered.
+
   EXPECT_EQ(LoadStreamStatus::kCannotLoadFromNetworkOffline,
             event_observer_.load_stream_status);
-  EXPECT_FALSE(network_.query_request_sent);
+  EXPECT_EQ("zero-state", surface.Describe());
 }
 
 TEST_F(FeedStreamTest, DoNotLoadStreamWhenArticleListIsHidden) {
@@ -542,9 +566,10 @@ TEST_F(FeedStreamTest, DoNotLoadStreamWhenArticleListIsHidden) {
   TestSurface surface;
   stream_->AttachSurface(&surface);
   WaitForIdleTaskQueue();
-  // TODO(harringtond): Implement zero-state and check that it is triggered.
+
   EXPECT_EQ(LoadStreamStatus::kLoadNotAllowedArticlesListHidden,
             event_observer_.load_stream_status);
+  EXPECT_EQ("zero-state", surface.Describe());
 }
 
 TEST_F(FeedStreamTest, DoNotLoadStreamWhenEulaIsNotAccepted) {
@@ -553,10 +578,10 @@ TEST_F(FeedStreamTest, DoNotLoadStreamWhenEulaIsNotAccepted) {
   TestSurface surface;
   stream_->AttachSurface(&surface);
   WaitForIdleTaskQueue();
-  // TODO(harringtond): Implement zero-state and check that it is triggered.
-  EXPECT_FALSE(network_.query_request_sent);
+
   EXPECT_EQ(LoadStreamStatus::kLoadNotAllowedEulaNotAccepted,
             event_observer_.load_stream_status);
+  EXPECT_EQ("zero-state", surface.Describe());
 }
 
 TEST_F(FeedStreamTest, DoNotLoadFromNetworkAfterHistoryIsDeleted) {
@@ -568,7 +593,8 @@ TEST_F(FeedStreamTest, DoNotLoadFromNetworkAfterHistoryIsDeleted) {
   stream_->AttachSurface(&surface);
   WaitForIdleTaskQueue();
 
-  ASSERT_FALSE(surface.initial_state);
+  EXPECT_EQ("zero-state", surface.Describe());
+
   EXPECT_EQ(LoadStreamStatus::kCannotLoadFromNetworkSupressedForHistoryDelete,
             event_observer_.load_stream_status);
 
@@ -577,8 +603,7 @@ TEST_F(FeedStreamTest, DoNotLoadFromNetworkAfterHistoryIsDeleted) {
   stream_->AttachSurface(&surface);
   WaitForIdleTaskQueue();
 
-  ASSERT_TRUE(surface.initial_state);
-  ASSERT_EQ(2, surface.initial_state->updated_slices().size());
+  EXPECT_EQ("2 slices 2 updates", surface.Describe());
 }
 
 TEST_F(FeedStreamTest, ShouldMakeFeedQueryRequestConsumesQuota) {
@@ -602,9 +627,8 @@ TEST_F(FeedStreamTest, LoadStreamFromStore) {
   stream_->AttachSurface(&surface);
   WaitForIdleTaskQueue();
 
-  ASSERT_TRUE(surface.initial_state);
+  ASSERT_EQ("2 slices", surface.Describe());
   EXPECT_FALSE(network_.query_request_sent);
-  EXPECT_EQ(2, surface.initial_state->updated_slices().size());
   // Verify the model is filled correctly.
   EXPECT_STRINGS_EQUAL(ModelStateFor(MakeTypicalInitialModelState()),
                        stream_->GetModel()->DumpStateForTesting());
@@ -617,8 +641,8 @@ TEST_F(FeedStreamTest, DetachSurfaceWhileLoadingModel) {
   stream_->DetachSurface(&surface);
   WaitForIdleTaskQueue();
 
+  EXPECT_EQ("empty", surface.Describe());
   EXPECT_TRUE(network_.query_request_sent);
-  EXPECT_FALSE(surface.initial_state);
 }
 
 TEST_F(FeedStreamTest, AttachMultipleSurfacesLoadsModelOnce) {

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

@@ -5,6 +5,8 @@
 #ifndef COMPONENTS_FEED_CORE_V2_PUBLIC_FEED_STREAM_API_H_
 #define COMPONENTS_FEED_CORE_V2_PUBLIC_FEED_STREAM_API_H_
 
+#include <vector>
+
 #include "base/observer_list_types.h"
 #include "base/util/type_safety/id_type.h"
 #include "components/feed/core/proto/v2/wire/content_id.pb.h"
@@ -30,10 +32,8 @@ class FeedStreamApi {
  public:
   class SurfaceInterface : public base::CheckedObserver {
    public:
-    // Called once after registering the observer. Provides complete
-    // initial state.
-    virtual void InitialStreamState(const feedui::StreamUpdate&) = 0;
-    // Called only after InitialStreamState, for each subsequent update.
+    // Called after registering the observer to provide the full stream state.
+    // Also called whenever the stream changes.
     virtual void StreamUpdate(const feedui::StreamUpdate&) = 0;
   };
 

components/feed/core/v2/stream_model.cc

@@ -57,7 +57,8 @@ const feedstore::Content* StreamModel::FindContent(
     ContentRevision revision) const {
   return GetFinalFeatureTree()->FindContent(revision);
 }
-const std::string* StreamModel::FindSharedStateData(const std::string& id) {
+const std::string* StreamModel::FindSharedStateData(
+    const std::string& id) const {
   auto iter = shared_states_.find(id);
   if (iter != shared_states_.end()) {
     return &iter->second.data;

components/feed/core/v2/stream_model.h

@@ -96,7 +96,7 @@ class StreamModel {
   const feedstore::Content* FindContent(ContentRevision revision) const;
 
   // Returns the shared state data identified by |id|.
-  const std::string* FindSharedStateData(const std::string& id);
+  const std::string* FindSharedStateData(const std::string& id) const;
 
   // Apply |operations| to the model.
   void ExecuteOperations(std::vector<feedstore::DataOperation> operations);

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

@@ -25,6 +25,9 @@ class FeedStream;
 class LoadStreamTask : public offline_pages::Task {
  public:
   struct Result {
+    Result() = default;
+    explicit Result(LoadStreamStatus a_final_status)
+        : final_status(a_final_status) {}
     // Final status of loading the stream.
     LoadStreamStatus final_status = LoadStreamStatus::kNoStatus;
     // Status of just loading the stream from the persistent store, if that