[session_shell] Remove StoryController.Start().
Convert remaining places to use StoryController.RequestStart():
* session_shell integration test,
* dev_session_shell,
* modular benchmarks.
Collateral cleanup:
* Convert benchmark config to use fuchsia-pkg URLs.
* Simplify type parameters in requests for environment services.
* Simplify passing FIDL service pointers to watcher implementations.
TESTED=run_modular_benchmarks.sh
MF-121 #comment Remove StoryController.Start().
MF-121 #done
Change-Id: I40f9572acc14bfc5545dbca9246c4bfcd05cb4c1
diff --git a/bin/sessionmgr/dev_session_shell.cc b/bin/sessionmgr/dev_session_shell.cc
index 4f84771..80b1c38 100644
--- a/bin/sessionmgr/dev_session_shell.cc
+++ b/bin/sessionmgr/dev_session_shell.cc
@@ -56,6 +56,7 @@
class DevSessionShellApp : fuchsia::modular::StoryWatcher,
fuchsia::modular::InterruptionListener,
fuchsia::modular::NextListener,
+ fuchsia::modular::SessionShell,
public modular::ViewApp {
public:
explicit DevSessionShellApp(component::StartupContext* const startup_context,
@@ -63,11 +64,9 @@
: ViewApp(startup_context),
settings_(std::move(settings)),
story_watcher_binding_(this) {
- puppet_master_ =
- startup_context
- ->ConnectToEnvironmentService<fuchsia::modular::PuppetMaster>();
- session_shell_context_ = startup_context->ConnectToEnvironmentService<
- fuchsia::modular::SessionShellContext>();
+ startup_context->ConnectToEnvironmentService(puppet_master_.NewRequest());
+ startup_context->ConnectToEnvironmentService(
+ session_shell_context_.NewRequest());
session_shell_context_->GetStoryProvider(story_provider_.NewRequest());
session_shell_context_->GetSuggestionProvider(
suggestion_provider_.NewRequest());
@@ -79,6 +78,9 @@
interruption_listener_bindings_.AddBinding(this));
suggestion_provider_->SubscribeToNext(
next_listener_bindings_.AddBinding(this), 3);
+
+ startup_context->outgoing().AddPublicService(
+ session_shell_bindings_.GetHandler(this));
}
~DevSessionShellApp() override = default;
@@ -182,10 +184,8 @@
story_controller_->Watch(story_watcher_binding_.NewBinding());
FXL_LOG(INFO) << "DevSessionShell Starting story with id: " << story_id;
- fidl::InterfaceHandle<fuchsia::ui::viewsv1token::ViewOwner>
- root_module_view;
- story_controller_->Start(root_module_view.NewRequest());
- view_->ConnectView(std::move(root_module_view));
+
+ story_controller_->RequestStart();
focus_controller_->Set(story_id);
auto visible_stories = fidl::VectorPtr<fidl::StringPtr>::New(0);
visible_stories.push_back(story_id);
@@ -205,6 +205,21 @@
}
}
+ // |SessionShell|
+ void AttachView(fuchsia::modular::ViewIdentifier view_id,
+ fidl::InterfaceHandle<fuchsia::ui::viewsv1token::ViewOwner>
+ view_owner) override {
+ FXL_LOG(INFO) << "DevSessionShell AttachView(): " << view_id.story_id;
+ view_->ConnectView(std::move(view_owner));
+ }
+
+ // |SessionShell|
+ void DetachView(fuchsia::modular::ViewIdentifier view_id,
+ std::function<void()> done) override {
+ FXL_LOG(INFO) << "DevSessionShell DetachView(): " << view_id.story_id;
+ done();
+ }
+
// |fuchsia::modular::StoryWatcher|
void OnStateChange(fuchsia::modular::StoryState state) override {
FXL_LOG(INFO) << "DevSessionShell State " << fidl::ToUnderlying(state);
@@ -244,6 +259,8 @@
const Settings settings_;
+ fidl::BindingSet<fuchsia::modular::SessionShell> session_shell_bindings_;
+
zx::eventpair view_token_;
std::unique_ptr<modular::ViewHost> view_;
diff --git a/bin/sessionmgr/story_runner/story_controller_impl.cc b/bin/sessionmgr/story_runner/story_controller_impl.cc
index 5f16a0c..de21e60 100644
--- a/bin/sessionmgr/story_runner/story_controller_impl.cc
+++ b/bin/sessionmgr/story_runner/story_controller_impl.cc
@@ -1003,12 +1003,10 @@
public:
StartCall(
StoryControllerImpl* const story_controller_impl,
- StoryStorage* const storage,
- fidl::InterfaceRequest<fuchsia::ui::viewsv1token::ViewOwner> request)
+ StoryStorage* const storage)
: Operation("StoryControllerImpl::StartCall", [] {}),
story_controller_impl_(story_controller_impl),
- storage_(storage),
- request_(std::move(request)) {}
+ storage_(storage) {}
private:
void Run() override {
@@ -1021,7 +1019,7 @@
return;
}
- story_controller_impl_->StartStoryShell(std::move(request_));
+ story_controller_impl_->StartStoryShell();
// Start all modules that were not themselves explicitly started by another
// module.
@@ -1046,7 +1044,6 @@
StoryControllerImpl* const story_controller_impl_; // not owned
StoryStorage* const storage_; // not owned
- fidl::InterfaceRequest<fuchsia::ui::viewsv1token::ViewOwner> request_;
OperationQueue operation_queue_;
@@ -1459,14 +1456,8 @@
}));
}
-void StoryControllerImpl::Start(
- fidl::InterfaceRequest<fuchsia::ui::viewsv1token::ViewOwner> request) {
- operation_queue_.Add(new StartCall(this, story_storage_, std::move(request)));
-}
-
void StoryControllerImpl::RequestStart() {
- operation_queue_.Add(
- new StartCall(this, story_storage_, nullptr /* ViewOwner request */));
+ operation_queue_.Add(new StartCall(this, story_storage_));
}
void StoryControllerImpl::Stop(StopCallback done) {
@@ -1560,15 +1551,10 @@
ConnectLinkPath(fidl::MakeOptional(std::move(link_path)), std::move(request));
}
-void StoryControllerImpl::StartStoryShell(
- fidl::InterfaceRequest<fuchsia::ui::viewsv1token::ViewOwner> request) {
- if (!request) {
- // The start call originated in RequestStart() rather than Start().
- fuchsia::ui::viewsv1token::ViewOwnerPtr view_owner;
- request = view_owner.NewRequest();
- story_provider_impl_->AttachView(story_id_, std::move(view_owner));
- needs_detach_view_ = true;
- }
+void StoryControllerImpl::StartStoryShell() {
+ fuchsia::ui::viewsv1token::ViewOwnerPtr view_owner;
+ auto request = view_owner.NewRequest();
+ story_provider_impl_->AttachView(story_id_, std::move(view_owner));
story_shell_app_ =
story_provider_impl_->StartStoryShell(story_id_, std::move(request));
@@ -1581,12 +1567,7 @@
}
void StoryControllerImpl::DetachView(std::function<void()> done) {
- if (needs_detach_view_) {
- story_provider_impl_->DetachView(story_id_, std::move(done));
- needs_detach_view_ = false;
- } else {
- done();
- }
+ story_provider_impl_->DetachView(story_id_, std::move(done));
}
void StoryControllerImpl::SetRuntimeState(
diff --git a/bin/sessionmgr/story_runner/story_controller_impl.h b/bin/sessionmgr/story_runner/story_controller_impl.h
index 3f6972c..6684813 100644
--- a/bin/sessionmgr/story_runner/story_controller_impl.h
+++ b/bin/sessionmgr/story_runner/story_controller_impl.h
@@ -165,11 +165,7 @@
private:
// |StoryController|
void Stop(StopCallback done) override;
-
- // |StoryController|
void GetInfo(GetInfoCallback callback) override;
- void Start(fidl::InterfaceRequest<fuchsia::ui::viewsv1token::ViewOwner>
- request) override;
void RequestStart() override;
void TakeAndLoadSnapshot(
fidl::InterfaceRequest<fuchsia::ui::viewsv1token::ViewOwner> request,
@@ -189,8 +185,7 @@
fidl::InterfaceRequest<fuchsia::modular::Link> request) override;
// Communicates with SessionShell.
- void StartStoryShell(
- fidl::InterfaceRequest<fuchsia::ui::viewsv1token::ViewOwner> request);
+ void StartStoryShell();
void DetachView(std::function<void()> done);
// Called whenever |story_storage_| sees an updated ModuleData from another
@@ -227,13 +222,7 @@
// reading from the |story_observer_| directly.
const fidl::StringPtr story_id_;
- // True once AttachView() was called. Temporarily needed during transition
- // from Start() to RequestStart(), will be removed once Start() is removed.
- // Cf. MF-121.
- bool needs_detach_view_{};
-
StoryProviderImpl* const story_provider_impl_; // Not owned.
-
SessionStorage* const session_storage_; // Not owned.
StoryStorage* const story_storage_; // Not owned.
diff --git a/lib/testing/story_controller_mock.h b/lib/testing/story_controller_mock.h
index 6e8facd..2bd5669 100644
--- a/lib/testing/story_controller_mock.h
+++ b/lib/testing/story_controller_mock.h
@@ -32,16 +32,14 @@
}
// |fuchsia::modular::StoryController|
- void Start(fidl::InterfaceRequest<fuchsia::ui::viewsv1token::ViewOwner>
- request) override {
+ void RequestStart() override {
FXL_NOTIMPLEMENTED();
}
// |fuchsia::modular::StoryController|
- void RequestStart() override { FXL_NOTIMPLEMENTED(); }
-
- // |fuchsia::modular::StoryController|
- void Stop(StopCallback done) override { FXL_NOTIMPLEMENTED(); }
+ void Stop(StopCallback done) override {
+ FXL_NOTIMPLEMENTED();
+ }
// |fuchsia::modular::StoryController|
void TakeAndLoadSnapshot(
diff --git a/public/fidl/fuchsia.modular/story/story_controller.fidl b/public/fidl/fuchsia.modular/story/story_controller.fidl
index e0d345d..15648e2 100644
--- a/public/fidl/fuchsia.modular/story/story_controller.fidl
+++ b/public/fidl/fuchsia.modular/story/story_controller.fidl
@@ -15,17 +15,7 @@
// Gets information associated with the story.
1: GetInfo() -> (StoryInfo info, StoryState state);
- // DEPRECATED in favor of RequestStart().
- //
- // Runs the story controlled by this |StoryController| instance if not yet
- // running or requested to start, else does nothing. |view_owner| is an
- // interface request for the view of the story shell of this story. If the
- // story is already running, the view owner request is dropped. If Stop()
- // requests are pending when this request is issued, the request is queued
- // until the Stop() requests complete.
- 3: Start(request<fuchsia.ui.viewsv1token.ViewOwner> view_owner);
-
- // Requests to runsthe story controlled by this |StoryController| instance.
+ // Requests to run the story controlled by this |StoryController| instance.
// When the story starts, if not yet running, the view of the newly started
// story shell will be passed in a call to SessionShell.AttachView().
14: RequestStart();
diff --git a/tests/benchmarks/story/BUILD.gn b/tests/benchmarks/story/BUILD.gn
index ffabe06..ff63aa5 100644
--- a/tests/benchmarks/story/BUILD.gn
+++ b/tests/benchmarks/story/BUILD.gn
@@ -48,9 +48,11 @@
"//garnet/public/lib/fsl",
"//garnet/public/lib/fxl",
"//peridot/lib/common:names",
+ "//peridot/lib/fidl:single_service_app",
"//peridot/lib/rapidjson",
"//peridot/public/lib/integration_testing/cpp",
- "//peridot/lib/testing:component_base",
+ "//peridot/lib/testing:component_main",
+ "//peridot/lib/testing:session_shell_impl",
"//peridot/public/fidl/fuchsia.modular",
]
diff --git a/tests/benchmarks/story/modular_benchmark_story.tspec b/tests/benchmarks/story/modular_benchmark_story.tspec
index f749fd7..8351e11 100644
--- a/tests/benchmarks/story/modular_benchmark_story.tspec
+++ b/tests/benchmarks/story/modular_benchmark_story.tspec
@@ -1,11 +1,11 @@
{
"test_suite_name": "fuchsia.modular",
"app": "fuchsia-pkg://fuchsia.com/basemgr#meta/basemgr.cmx",
- "args": ["--account_provider=dev_token_manager",
- "--base_shell=dev_base_shell",
- "--session_shell=modular_benchmark_story_session_shell",
+ "args": ["--account_provider=fuchsia-pkg://fuchsia.com/dev_token_manager#meta/dev_token_manager.cmx",
+ "--base_shell=fuchsia-pkg://fuchsia.com/dev_base_shell#meta/dev_base_shell.cmx",
+ "--session_shell=fuchsia-pkg://fuchsia.com/modular_benchmark_story_session_shell#meta/modular_benchmark_story_session_shell.cmx",
"--session_shell_args=--story_count=20",
- "--story_shell=dev_story_shell"],
+ "--story_shell=fuchsia-pkg://fuchsia.com/dev_story_shell#meta/dev_story_shell.cmx"],
"categories": ["benchmark", "modular"],
"duration": 120,
"measure": [
diff --git a/tests/benchmarks/story/modular_benchmark_story_session_shell.cc b/tests/benchmarks/story/modular_benchmark_story_session_shell.cc
index 23b0f2b..5b4db09 100644
--- a/tests/benchmarks/story/modular_benchmark_story_session_shell.cc
+++ b/tests/benchmarks/story/modular_benchmark_story_session_shell.cc
@@ -23,7 +23,8 @@
#include "peridot/lib/common/names.h"
#include "peridot/lib/fidl/single_service_app.h"
-#include "peridot/lib/testing/component_base.h"
+#include "peridot/lib/testing/component_main.h"
+#include "peridot/lib/testing/session_shell_impl.h"
#include "peridot/public/lib/integration_testing/cpp/reporting.h"
#include "peridot/public/lib/integration_testing/cpp/testing.h"
#include "peridot/tests/benchmarks/story/tracing_waiter.h"
@@ -43,7 +44,8 @@
}
module_url = command_line.GetOptionValueWithDefault(
- "module_url", "modular_benchmark_story_module");
+ "module_url",
+ "fuchsia-pkg://fuchsia.com/modular_benchmark_story_module#meta/modular_benchmark_story_module.cmx");
}
int story_count{0};
@@ -60,8 +62,8 @@
// Registers itself as a watcher on the given story. Only one story at a time
// can be watched.
- void Watch(fuchsia::modular::StoryControllerPtr* const story_controller) {
- (*story_controller)->Watch(binding_.NewBinding());
+ void Watch(fuchsia::modular::StoryController* const story_controller) {
+ story_controller->Watch(binding_.NewBinding());
}
// Deregisters itself from the watched story.
@@ -107,8 +109,8 @@
// Registers itself as a watcher on the given link. Only one story at a time
// can be watched.
- void Watch(fuchsia::modular::LinkPtr* const link) {
- (*link)->WatchAll(binding_.NewBinding());
+ void Watch(fuchsia::modular::Link* const link) {
+ link->WatchAll(binding_.NewBinding());
}
// Deregisters itself from the watched story.
@@ -140,12 +142,15 @@
public:
TestApp(component::StartupContext* const startup_context, Settings settings)
: ViewApp(startup_context), settings_(std::move(settings)) {
- session_shell_context_ = startup_context->ConnectToEnvironmentService<
- fuchsia::modular::SessionShellContext>();
+ startup_context->ConnectToEnvironmentService(
+ session_shell_context_.NewRequest());
session_shell_context_->GetStoryProvider(story_provider_.NewRequest());
- puppet_master_ =
- startup_context
- ->ConnectToEnvironmentService<fuchsia::modular::PuppetMaster>();
+
+ startup_context->ConnectToEnvironmentService(puppet_master_.NewRequest());
+
+ startup_context->outgoing().AddPublicService(
+ session_shell_impl_.GetHandler());
+
tracing_waiter_.WaitForTracing([this] { Loop(); });
}
@@ -220,7 +225,7 @@
link_path.link_name = nullptr;
story_controller_->GetLink(std::move(link_path), link_.NewRequest());
- link_watcher_.Watch(&link_);
+ link_watcher_.Watch(link_.get());
link_watcher_.Continue([this](fidl::StringPtr json) {
FXL_LOG(INFO) << "Link() Watch: " << json;
if (json == "") {
@@ -247,10 +252,8 @@
TRACE_ASYNC_END("benchmark", "story/start", 0);
});
- story_watcher_.Watch(&story_controller_);
-
- fidl::InterfaceHandle<fuchsia::ui::viewsv1token::ViewOwner> story_view;
- story_controller_->Start(story_view.NewRequest());
+ story_watcher_.Watch(story_controller_.get());
+ story_controller_->RequestStart();
}
void StoryStop() {
@@ -279,6 +282,7 @@
StoryWatcherImpl story_watcher_;
LinkWatcherImpl link_watcher_;
+ modular::testing::SessionShellImpl session_shell_impl_;
fuchsia::modular::PuppetMasterPtr puppet_master_;
fuchsia::modular::StoryPuppetMasterPtr story_puppet_master_;
diff --git a/tests/session_shell/README.md b/tests/session_shell/README.md
index 92c97ea..2e3f80a 100644
--- a/tests/session_shell/README.md
+++ b/tests/session_shell/README.md
@@ -11,7 +11,7 @@
* Be notified when story state changes.
* Be notified through SessionShell.AttachView() of a new story view when story
- start is requested by RequestStart() rather than Start().
+ start is requested by RequestStart().
* Be notified through SessionShell.DetachView() of a story going away.
diff --git a/tests/session_shell/session_shell_test_session_shell.cc b/tests/session_shell/session_shell_test_session_shell.cc
index 71be6d5..4857cca 100644
--- a/tests/session_shell/session_shell_test_session_shell.cc
+++ b/tests/session_shell/session_shell_test_session_shell.cc
@@ -153,13 +153,6 @@
component_context_.NewRequest());
story_provider_state_watcher_.Watch(story_provider());
- // Until we use RequestStart() for the first time, there must be no calls on
- // the SessionShell service.
- session_shell_impl()->set_on_attach_view(
- [](ViewId) { Fail("AttachView() called without RequestStart()."); });
- session_shell_impl()->set_on_detach_view(
- [](ViewId) { Fail("DetachView() called without RequestStart()."); });
-
TestComponentContext_GetPackageName_Works();
}
@@ -178,8 +171,10 @@
create_view_.Pass();
}
- // Test Case: When we call GetPackageName() on ComponentContext acquired from
- // our environment, it works.
+ // Test Case GetPackageName:
+ //
+ // When we call GetPackageName() on ComponentContext acquired from our
+ // environment, it works.
TestPoint component_context_package_name_{
"ComponentContext.GetPackageName() works"};
@@ -194,7 +189,9 @@
});
}
- // Test Case: The story info of a story that does not exist is null.
+ // Test Case GetStoryInfo Null:
+ //
+ // The story info of a story that does not exist is null.
TestPoint get_story_info_null_{"StoryProvider.GetStoryInfo() is null"};
@@ -305,8 +302,7 @@
TestPoint story1_run_{"Story1 Run"};
void TestStory1_Run() {
// Start and show the new story.
- fidl::InterfaceHandle<fuchsia::ui::viewsv1token::ViewOwner> story_view;
- story_controller_->Start(story_view.NewRequest());
+ story_controller_->RequestStart();
story1_run_.Pass();
TestStory1_Stop();
}
@@ -391,8 +387,7 @@
// Start and show the new story *while* the GetInfo() call above is in
// flight.
- fidl::InterfaceHandle<fuchsia::ui::viewsv1token::ViewOwner> story_view;
- story_controller_->Start(story_view.NewRequest());
+ story_controller_->RequestStart();
story_controller_->GetInfo([this](fuchsia::modular::StoryInfo info,
fuchsia::modular::StoryState state) {
@@ -487,8 +482,7 @@
TestPoint story3_run_{"Story3 Run"};
void TestStory3_Run() {
- fidl::InterfaceHandle<fuchsia::ui::viewsv1token::ViewOwner> story_view;
- story_controller_->Start(story_view.NewRequest());
+ story_controller_->RequestStart();
story_controller_->GetInfo([this](fuchsia::modular::StoryInfo info,
fuchsia::modular::StoryState state) {
