[modular] Delete StoryProvider.DeleteStory().
Also: perform related cleanups now that some code-paths are simplified.
TEST=existing
MF-13 #comment [modular] Delete StoryProvider.DeleteStory().
Change-Id: I044a9bd9292c8a6ad285c6d72000df19e5f6ee83
diff --git a/bin/sessionmgr/story_runner/story_controller_impl.cc b/bin/sessionmgr/story_runner/story_controller_impl.cc
index 5f2f29a..0fbff7e 100644
--- a/bin/sessionmgr/story_runner/story_controller_impl.cc
+++ b/bin/sessionmgr/story_runner/story_controller_impl.cc
@@ -556,8 +556,7 @@
}
StoryControllerImpl* const story_controller_impl_; // not owned
- const bool
- notify_watchers_; // Whether to notify state change; false in DeleteCall.
+ const bool notify_watchers_; // Whether to notify state change to watchers.
FXL_DISALLOW_COPY_AND_ASSIGN(StopCall);
};
@@ -628,34 +627,6 @@
FXL_DISALLOW_COPY_AND_ASSIGN(StopModuleAndStoryIfEmptyCall);
};
-class StoryControllerImpl::DeleteCall : public Operation<> {
- public:
- DeleteCall(StoryControllerImpl* const story_controller_impl,
- std::function<void()> done)
- : Operation("StoryControllerImpl::DeleteCall", [] {}),
- story_controller_impl_(story_controller_impl),
- done_(std::move(done)) {}
-
- private:
- void Run() override {
- // No call to Done(), in order to block all further operations on the queue
- // until the instance is deleted.
- operation_queue_.Add(new StopCall(story_controller_impl_,
- false /* notify watchers */, done_));
- }
-
- StoryControllerImpl* const story_controller_impl_; // not owned
-
- // Not the result call of the Operation, because it's invoked without
- // unblocking the operation queue, to prevent subsequent operations from
- // executing until the instance is deleted, which cancels those operations.
- std::function<void()> done_;
-
- OperationQueue operation_queue_;
-
- FXL_DISALLOW_COPY_AND_ASSIGN(DeleteCall);
-};
-
class StoryControllerImpl::OnModuleDataUpdatedCall : public Operation<> {
public:
OnModuleDataUpdatedCall(StoryControllerImpl* const story_controller_impl,
@@ -1172,11 +1143,7 @@
}
}
-void StoryControllerImpl::StopForDelete(const StopCallback& done) {
- operation_queue_.Add(new DeleteCall(this, done));
-}
-
-void StoryControllerImpl::StopForTeardown(const StopCallback& done) {
+void StoryControllerImpl::StopWithoutNotifying(const StopCallback& done) {
operation_queue_.Add(new StopCall(this, false /* notify watchers */, done));
}
diff --git a/bin/sessionmgr/story_runner/story_controller_impl.h b/bin/sessionmgr/story_runner/story_controller_impl.h
index 3e212a5..1c1c935 100644
--- a/bin/sessionmgr/story_runner/story_controller_impl.h
+++ b/bin/sessionmgr/story_runner/story_controller_impl.h
@@ -64,21 +64,9 @@
// Called by StoryProviderImpl.
bool IsRunning();
- // Called by StoryProviderImpl.
- //
- // A variant of Stop() that stops the story because the story is being
- // deleted. The StoryControllerImpl instance is deleted by StoryProviderImpl
- // and the story data are deleted from the ledger once the done callback is
- // invoked.
- //
- // No further operations invoked after this one are executed. (The Operation
- // accomplishes this by not calling Done() and instead invoking its callback
- // directly from Run(), such that the OperationQueue stays blocked on it until
- // it gets deleted.)
- void StopForDelete(const std::function<void()>& done);
-
- // Called by StoryProviderImpl.
- void StopForTeardown(const std::function<void()>& done);
+ // Called by StoryProviderImpl. A variant of Stop() which does not notify
+ // listeners that we stopped the story.
+ void StopWithoutNotifying(const std::function<void()>& done);
// Called by StoryProviderImpl.
fuchsia::modular::StoryState GetStoryState() const;
@@ -355,7 +343,6 @@
// Operations implemented here.
class AddIntentCall;
class DefocusCall;
- class DeleteCall;
class FocusCall;
class KillModuleCall;
class LaunchModuleCall;
diff --git a/bin/sessionmgr/story_runner/story_provider_impl.cc b/bin/sessionmgr/story_runner/story_provider_impl.cc
index db42e32..a19449b 100644
--- a/bin/sessionmgr/story_runner/story_provider_impl.cc
+++ b/bin/sessionmgr/story_runner/story_provider_impl.cc
@@ -44,45 +44,36 @@
constexpr char kSnapshotLoaderUrl[] = "snapshot";
-class StoryProviderImpl::DeleteStoryCall : public Operation<> {
+class StoryProviderImpl::StopStoryCall : public Operation<> {
public:
using StoryRuntimesMap = std::map<std::string, struct StoryRuntimeContainer>;
- using PendingDeletion = std::pair<std::string, DeleteStoryCall*>;
- DeleteStoryCall(SessionStorage* session_storage, fidl::StringPtr story_id,
- StoryRuntimesMap* const story_runtime_containers,
- MessageQueueManager* const message_queue_manager,
- const bool already_deleted, ResultCall result_call)
+ StopStoryCall(fidl::StringPtr story_id,
+ StoryRuntimesMap* const story_runtime_containers,
+ MessageQueueManager* const message_queue_manager,
+ ResultCall result_call)
: Operation("StoryProviderImpl::DeleteStoryCall", std::move(result_call)),
- session_storage_(session_storage),
story_id_(story_id),
story_runtime_containers_(story_runtime_containers),
- message_queue_manager_(message_queue_manager),
- already_deleted_(already_deleted) {}
+ message_queue_manager_(message_queue_manager) {}
private:
void Run() override {
FlowToken flow{this};
- if (already_deleted_) {
- Teardown(flow);
- } else {
- session_storage_->DeleteStory(story_id_)->WeakThen(
- GetWeakPtr(), [this, flow] { Teardown(flow); });
- }
- }
-
- void Teardown(FlowToken flow) {
auto i = story_runtime_containers_->find(story_id_);
if (i == story_runtime_containers_->end()) {
+ FXL_LOG(WARNING) << "I was told to teardown story " << story_id_
+ << ", but I can't find it.";
return;
}
FXL_DCHECK(i->second.controller_impl != nullptr);
- i->second.controller_impl->StopForDelete([this, flow] { Erase(flow); });
+ i->second.controller_impl->StopWithoutNotifying(
+ [this, flow] { CleanupRuntime(flow); });
}
- void Erase(FlowToken flow) {
+ void CleanupRuntime(FlowToken flow) {
// Here we delete the instance from whose operation a result callback was
// received. Thus we must assume that the callback returns to a method of
// the instance. If we delete the instance right here, |this| would be
@@ -91,23 +82,21 @@
// functions that run as methods of other objects owned by |this| or
// provided to |this|. To avoid such problems, the delete is invoked
// through the run loop.
+ //
+ // TODO(thatguy); Understand the above comment, and rewrite it.
async::PostTask(async_get_default_dispatcher(), [this, flow] {
story_runtime_containers_->erase(story_id_);
message_queue_manager_->DeleteNamespace(
EncodeModuleComponentNamespace(story_id_), [flow] {});
-
- // TODO(mesch): We must delete the story page too. MI4-1002
});
}
private:
- SessionStorage* const session_storage_; // Not owned.
const fidl::StringPtr story_id_;
StoryRuntimesMap* const story_runtime_containers_;
MessageQueueManager* const message_queue_manager_;
- const bool already_deleted_; // True if called from OnChange();
- FXL_DISALLOW_COPY_AND_ASSIGN(DeleteStoryCall);
+ FXL_DISALLOW_COPY_AND_ASSIGN(StopStoryCall);
};
// Loads a StoryRuntimeContainer object so that the given story is ready to be
@@ -196,12 +185,14 @@
// Each callback has a copy of |flow| which only goes out-of-scope
// once the story corresponding to |it| stops.
//
- // TODO(mesch): If a DeleteCall is executing in front of
- // StopForTeardown(), then the StopCall in StopForTeardown() never
- // executes because the fuchsia::modular::StoryController instance is
- // deleted after the DeleteCall finishes. This will then block unless it
- // runs in a timeout.
- it.second.controller_impl->StopForTeardown(
+ // TODO(thatguy): If the StoryControllerImpl is deleted before it can
+ // complete StopWithoutNotifying(), we will never be called back and the
+ // OperationQueue on which we're running will block. Moving over to
+ // fit::promise will allow us to observe cancellation.
+ //
+ // TODO(thatguy): Use StopStoryCall instead of reproducing some of its
+ // logic here.
+ it.second.controller_impl->StopWithoutNotifying(
[this, story_id = it.first, flow] {
// It is okay to erase story_id because story provider binding has
// been closed and this callback cannot be invoked synchronously.
@@ -435,15 +426,6 @@
}
// |fuchsia::modular::StoryProvider|
-void StoryProviderImpl::DeleteStory(fidl::StringPtr story_id,
- DeleteStoryCallback callback) {
- operation_queue_.Add(new DeleteStoryCall(
- session_storage_, story_id, &story_runtime_containers_,
- component_context_info_.message_queue_manager,
- false /* already_deleted */, callback));
-}
-
-// |fuchsia::modular::StoryProvider|
void StoryProviderImpl::GetStoryInfo(fidl::StringPtr story_id,
GetStoryInfoCallback callback) {
auto on_run = Future<>::Create("StoryProviderImpl.GetStoryInfo.on_run");
@@ -611,14 +593,9 @@
}
void StoryProviderImpl::OnStoryStorageDeleted(fidl::StringPtr story_id) {
- // NOTE: DeleteStoryCall is used here, as well as in DeleteStory(). In this
- // case, either another device deleted the story, or we did and the Ledger
- // is now notifying us. In this case, we pass |already_deleted = true| so
- // that we don't ask to delete the story data again.
- operation_queue_.Add(new DeleteStoryCall(
- session_storage_, story_id, &story_runtime_containers_,
- component_context_info_.message_queue_manager, true /* already_deleted */,
- [this, story_id] {
+ operation_queue_.Add(new StopStoryCall(
+ story_id, &story_runtime_containers_,
+ component_context_info_.message_queue_manager, [this, story_id] {
for (const auto& i : watchers_.ptrs()) {
(*i)->OnDelete(story_id);
}
@@ -662,7 +639,7 @@
const fuchsia::modular::internal::StoryData* const story_data,
const fuchsia::modular::StoryState story_state,
const fuchsia::modular::StoryVisibilityState story_visibility_state) {
- if (story_data->story_options.kind_of_proto_story) {
+ if (!story_data || story_data->story_options.kind_of_proto_story) {
return;
}
for (const auto& i : watchers_.ptrs()) {
diff --git a/bin/sessionmgr/story_runner/story_provider_impl.h b/bin/sessionmgr/story_runner/story_provider_impl.h
index 3b4bcdf..ca04c50 100644
--- a/bin/sessionmgr/story_runner/story_provider_impl.h
+++ b/bin/sessionmgr/story_runner/story_provider_impl.h
@@ -131,8 +131,8 @@
fidl::VectorPtr<fuchsia::modular::OngoingActivityType>
ongoing_activities);
- // Called by StoryControllerImpl. Sends request to fuchsia::modular::SessionShell
- // through PresentationProvider.
+ // Called by StoryControllerImpl. Sends request to
+ // fuchsia::modular::SessionShell through PresentationProvider.
void GetPresentation(
fidl::StringPtr story_id,
fidl::InterfaceRequest<fuchsia::ui::policy::Presentation> request);
@@ -177,10 +177,6 @@
private:
// |fuchsia::modular::StoryProvider|
- void DeleteStory(fidl::StringPtr story_id,
- DeleteStoryCallback callback) override;
-
- // |fuchsia::modular::StoryProvider|
void GetController(fidl::StringPtr story_id,
fidl::InterfaceRequest<fuchsia::modular::StoryController>
request) override;
@@ -270,7 +266,7 @@
user_intelligence_provider_; // Not owned.
fuchsia::modular::ModuleResolver* const module_resolver_; // Not owned.
EntityProviderRunner* const entity_provider_runner_; // Not owned.
- modular::ModuleFacetReader* const module_facet_reader_; // Not owned.
+ modular::ModuleFacetReader* const module_facet_reader_; // Not owned.
PresentationProvider* const presentation_provider_; // Not owned.
// When a story gets created, or when it gets focused on this device, we write
@@ -313,9 +309,8 @@
fxl::WeakPtrFactory<StoryProviderImpl> weak_factory_;
// Operations implemented here.
- class CreateStoryCall;
- class DeleteStoryCall;
class LoadStoryRuntimeCall;
+ class StopStoryCall;
class StopAllStoriesCall;
class StopStoryShellCall;
class GetStoryEntityProviderCall;
diff --git a/lib/testing/story_provider_mock.h b/lib/testing/story_provider_mock.h
index c0c694c..eda98f7 100644
--- a/lib/testing/story_provider_mock.h
+++ b/lib/testing/story_provider_mock.h
@@ -67,13 +67,6 @@
}
// |fuchsia::modular::StoryProvider|
- void DeleteStory(fidl::StringPtr story_id,
- DeleteStoryCallback callback) override {
- deleted_story_ = story_id;
- callback();
- }
-
- // |fuchsia::modular::StoryProvider|
void GetStoryInfo(fidl::StringPtr story_id,
GetStoryInfoCallback callback) override {
callback(nullptr);
diff --git a/public/fidl/fuchsia.modular/story/story_provider.fidl b/public/fidl/fuchsia.modular/story/story_provider.fidl
index dc0fea6..33f979a 100644
--- a/public/fidl/fuchsia.modular/story/story_provider.fidl
+++ b/public/fidl/fuchsia.modular/story/story_provider.fidl
@@ -12,22 +12,13 @@
// framework.
[Discoverable]
interface StoryProvider {
- // Deletes an existing story from the list of known stories. Returns when the
- // delete notification is received from the Ledger. If the story to be deleted
- // is running, it is first stopped and its story controller disconnected. If
- // the story ID doesn't exist, it silently does nothing and returns.
- //
- // DEPRECATED: To delete stories, use PuppetMaster.
- 3: DeleteStory(string story_id) -> ();
-
// Returns a list of existing stories. If |watcher| is provided, the client will
// be notified of story changes (new stories, deleted stories, runtime
// state changes).
4: GetStories(StoryProviderWatcher? watcher) -> (vector<StoryInfo> story_infos);
// Requests detailed information about the given story. If the story doesn't
- // exist, returns null. GetStoryInfo() called immediately after DeleteStory()
- // (even before DeleteStory() returns) returns null.
+ // exist, returns null.
5: GetStoryInfo(string story_id) -> (StoryInfo? story_info);
// Obtains a controller for a previously created story identified by its story
@@ -89,25 +80,11 @@
1: OnChange(StoryInfo story_info, StoryState story_state,
StoryVisibilityState story_visibility_state);
- // Called in two different situations:
+ // Called when a story record is permanently deleted. The deletion could
+ // have originated on this or on another device.
//
- // * After the story record of a story is deleted from the current device by
- // an invocation of StoryProvider.DeleteStory() on the current device.
- //
- // * When the information reaches the current device through Ledger
- // synchronization that the story record of a story was deleted from another
- // device by an invocation by StoryProvider.DeleteStory() there.
- //
- // I.e. if the story is deleted on *another* device, it eventually *does*
- // cause an OnDelete() call on *this* device. Cf. OnChange() above.
- //
- // Either way, if the story is running on this device at the time the deletion
- // becomes known to this device on this device, it is also stopped first. This
- // stopping of the story does not cause an OnChange() callback, only the
- // OnDelete() here.
- //
- // The OnDelete() call also happen when the story record is deleted while the
- // story is not running.
+ // If the story is running on this device at the time it is deleted,
+ // OnChange() will not be called first.
2: OnDelete(string story_id);
};
diff --git a/tests/link_data/link_data_test_session_shell.cc b/tests/link_data/link_data_test_session_shell.cc
index ff24f7a..dd5ef22 100644
--- a/tests/link_data/link_data_test_session_shell.cc
+++ b/tests/link_data/link_data_test_session_shell.cc
@@ -237,7 +237,7 @@
TestPoint story2_stop_{"Story2 Stop"};
void TestStory2_Delete() {
- story_provider_->DeleteStory(story_info_.id, [this] {
+ puppet_master_->DeleteStory(story_info_.id, [this] {
story2_stop_.Pass();
session_shell_context_->Logout();
});
