[modular][storage] Don't store ModuleManifest in ModuleData.
We stored ModuleManifest in ModuleData so that we could eventually send
it to the StoryShell. Instead of doing that, we can look it up right
before sending a Module's info to the StoryShell and thus remove a
number of unnecessarily locations depending on ModuleFacetReader.
Now, we always use the ModuleFacetReader to get our ModuleManifest.
TEST=existing
Change-Id: If7043b9663571241e28da7f8efd2a4f365a1f7a1
diff --git a/bin/sessionmgr/puppet_master/command_runners/add_mod_command_runner.cc b/bin/sessionmgr/puppet_master/command_runners/add_mod_command_runner.cc
index dbee8e3..55bc3b2 100644
--- a/bin/sessionmgr/puppet_master/command_runners/add_mod_command_runner.cc
+++ b/bin/sessionmgr/puppet_master/command_runners/add_mod_command_runner.cc
@@ -11,14 +11,10 @@
AddModCommandRunner::AddModCommandRunner(
fuchsia::modular::ModuleResolver* const module_resolver,
- fuchsia::modular::EntityResolver* const entity_resolver,
- modular::ModuleFacetReader* const module_facet_reader)
- : module_resolver_(module_resolver),
- entity_resolver_(entity_resolver),
- module_facet_reader_(module_facet_reader) {
+ fuchsia::modular::EntityResolver* const entity_resolver)
+ : module_resolver_(module_resolver), entity_resolver_(entity_resolver) {
FXL_DCHECK(module_resolver_);
FXL_DCHECK(entity_resolver_);
- FXL_DCHECK(module_facet_reader_);
}
AddModCommandRunner::~AddModCommandRunner() = default;
@@ -32,8 +28,7 @@
auto& add_mod = command.add_mod();
AddAddModOperation(
&operation_queue_, story_storage, module_resolver_, entity_resolver_,
- module_facet_reader_, std::move(add_mod.mod_name),
- std::move(add_mod.intent),
+ std::move(add_mod.mod_name), std::move(add_mod.intent),
std::make_unique<fuchsia::modular::SurfaceRelation>(
std::move(add_mod.surface_relation)),
std::move(add_mod.surface_parent_mod_name),
diff --git a/bin/sessionmgr/puppet_master/command_runners/add_mod_command_runner.h b/bin/sessionmgr/puppet_master/command_runners/add_mod_command_runner.h
index 8b32f1d..f55e7b1 100644
--- a/bin/sessionmgr/puppet_master/command_runners/add_mod_command_runner.h
+++ b/bin/sessionmgr/puppet_master/command_runners/add_mod_command_runner.h
@@ -9,7 +9,6 @@
#include <lib/async/cpp/operation.h>
#include "peridot/bin/sessionmgr/puppet_master/command_runners/command_runner.h"
-#include "peridot/lib/module_manifest/module_facet_reader.h"
namespace modular {
@@ -19,11 +18,8 @@
// * ModuleResolver: used to resolve an intent into a module.
// * EntityResolver: used to resolve an intent parameter's type, which is
// supplied to the module resolver for use in resolution.
- // * ModuleFacetReader: used to read the module facet from the component
- // manifest.
AddModCommandRunner(fuchsia::modular::ModuleResolver* const module_resolver,
- fuchsia::modular::EntityResolver* const entity_resolver,
- modular::ModuleFacetReader* const module_facet_reader);
+ fuchsia::modular::EntityResolver* const entity_resolver);
~AddModCommandRunner() override;
void Execute(
@@ -35,7 +31,6 @@
OperationQueue operation_queue_;
fuchsia::modular::ModuleResolver* const module_resolver_; // Not owned.
fuchsia::modular::EntityResolver* const entity_resolver_; // Not owned.
- modular::ModuleFacetReader* const module_facet_reader_; // Not owned.
};
} // namespace modular
diff --git a/bin/sessionmgr/puppet_master/command_runners/add_mod_command_runner_unittest.cc b/bin/sessionmgr/puppet_master/command_runners/add_mod_command_runner_unittest.cc
index aa187c4..cef88ef 100644
--- a/bin/sessionmgr/puppet_master/command_runners/add_mod_command_runner_unittest.cc
+++ b/bin/sessionmgr/puppet_master/command_runners/add_mod_command_runner_unittest.cc
@@ -14,7 +14,6 @@
#include "gtest/gtest.h"
#include "peridot/lib/ledger_client/page_id.h"
#include "peridot/lib/testing/entity_resolver_fake.h"
-#include "peridot/lib/testing/module_facet_reader_fake.h"
#include "peridot/lib/testing/module_resolver_fake.h"
#include "peridot/lib/testing/test_with_session_storage.h"
@@ -39,8 +38,7 @@
story_storage_ = GetStoryStorage(session_storage_.get(), story_id_);
fake_module_resolver_->Connect(module_resolver_.NewRequest());
fake_entity_resolver_->Connect(entity_resolver_.NewRequest());
- runner_ = MakeRunner(module_resolver_.get(), entity_resolver_.get(),
- &fake_module_facet_reader_);
+ runner_ = MakeRunner(module_resolver_.get(), entity_resolver_.get());
}
protected:
@@ -125,10 +123,9 @@
std::unique_ptr<AddModCommandRunner> MakeRunner(
fuchsia::modular::ModuleResolver* const module_resolver,
- fuchsia::modular::EntityResolver* const entity_resolver,
- modular::ModuleFacetReader* const module_facet_reader) {
- return std::make_unique<AddModCommandRunner>(
- module_resolver, entity_resolver, module_facet_reader);
+ fuchsia::modular::EntityResolver* const entity_resolver) {
+ return std::make_unique<AddModCommandRunner>(module_resolver,
+ entity_resolver);
}
fuchsia::modular::StoryCommand MakeAddModCommand(
@@ -234,7 +231,6 @@
std::make_unique<ModuleResolverFake>();
std::unique_ptr<EntityResolverFake> fake_entity_resolver_ =
std::make_unique<EntityResolverFake>();
- ModuleFacetReaderFake fake_module_facet_reader_;
};
TEST_F(AddModCommandRunnerTest, ExecuteIntentWithIntentHandler) {
@@ -284,7 +280,6 @@
module_data->module_source);
EXPECT_EQ(0.5, module_data->surface_relation->emphasis);
EXPECT_TRUE(AreIntentsEqual(intent, *module_data->intent));
- EXPECT_EQ(*manifest, *module_data->module_manifest);
EXPECT_EQ(0u, module_data->parameter_map.entries.size());
done = true;
});
@@ -342,7 +337,6 @@
module_data->module_source);
EXPECT_EQ(0.5, module_data->surface_relation->emphasis);
EXPECT_TRUE(AreIntentsEqual(intent, *module_data->intent));
- EXPECT_EQ(*manifest, *module_data->module_manifest);
EXPECT_EQ(0u, module_data->parameter_map.entries.size());
done = true;
});
@@ -418,7 +412,6 @@
module_data->module_source);
EXPECT_EQ(0.5, module_data->surface_relation->emphasis);
EXPECT_TRUE(AreIntentsEqual(intent, *module_data->intent));
- EXPECT_EQ(*manifest, *module_data->module_manifest);
EXPECT_EQ(3u, module_data->parameter_map.entries.size());
// Verify parameters
@@ -662,37 +655,5 @@
RunLoopUntil([&] { return done; });
}
-TEST_F(AddModCommandRunnerTest, ReadModFacetIfNoResolverResponse) {
- constexpr char kModUrl[] = "mod_url";
-
- // Don't supply dummy answers to fake module resolver; this should trigger the
- // module facet reader. Verify that it does.
- fake_module_resolver_->SetStatus(
- fuchsia::modular::FindModulesStatus::UNKNOWN_HANDLER);
-
- bool mod_facet_reader_done = false;
- fake_module_facet_reader_.SetGetModuleManifestSink(
- [&](const std::string& mod_url,
- ModuleFacetReader::GetModuleManifestCallback cb) {
- EXPECT_EQ(kModUrl, mod_url);
- mod_facet_reader_done = true;
- cb({});
- });
-
- // Add a mod to begin with.
- bool done = false;
- runner_->Execute(
- story_id_, story_storage_.get(),
- MakeAddModCommand("mod_name", "parent_mod_name", 0.5,
- CreateEmptyIntent("intent_action", kModUrl)),
- [&](fuchsia::modular::ExecuteResult result) {
- ASSERT_EQ(fuchsia::modular::ExecuteStatus::OK, result.status)
- << result.error_message;
- done = true;
- });
-
- RunLoopUntil([&] { return mod_facet_reader_done && done; });
-}
-
} // namespace
} // namespace modular
diff --git a/bin/sessionmgr/puppet_master/command_runners/operation_calls/add_mod_call.cc b/bin/sessionmgr/puppet_master/command_runners/operation_calls/add_mod_call.cc
index 41eca48..1460cc1 100644
--- a/bin/sessionmgr/puppet_master/command_runners/operation_calls/add_mod_call.cc
+++ b/bin/sessionmgr/puppet_master/command_runners/operation_calls/add_mod_call.cc
@@ -14,7 +14,6 @@
#include "peridot/bin/sessionmgr/puppet_master/command_runners/operation_calls/get_types_from_entity_call.h"
#include "peridot/bin/sessionmgr/puppet_master/command_runners/operation_calls/initialize_chain_call.h"
#include "peridot/lib/fidl/clone.h"
-#include "peridot/lib/module_manifest/module_facet_reader.h"
namespace modular {
@@ -26,7 +25,6 @@
AddModCall(StoryStorage* const story_storage,
fuchsia::modular::ModuleResolver* const module_resolver,
fuchsia::modular::EntityResolver* const entity_resolver,
- modular::ModuleFacetReader* const module_facet_reader,
std::vector<std::string> mod_name,
fuchsia::modular::Intent intent,
fuchsia::modular::SurfaceRelationPtr surface_relation,
@@ -36,7 +34,6 @@
story_storage_(story_storage),
module_resolver_(module_resolver),
entity_resolver_(entity_resolver),
- module_facet_reader_(module_facet_reader),
mod_name_(std::move(mod_name)),
intent_(std::move(intent)),
surface_relation_(std::move(surface_relation)),
@@ -211,36 +208,16 @@
out_module_data_.intent =
std::make_unique<fuchsia::modular::Intent>(std::move(intent_));
- auto write_to_storage_cont = [this, flow]() {
- // Operation stays alive until flow goes out of scope.
- fuchsia::modular::ModuleData module_data;
- out_module_data_.Clone(&module_data);
- story_storage_->WriteModuleData(std::move(module_data))
- ->Then([this, flow] {});
- };
-
- // If the resolver gave us a module manifest (which is described in the
- // module facet), we don't need to try and read it.
- if (candidate_module_.manifest) {
- fidl::Clone(candidate_module_.manifest,
- &out_module_data_.module_manifest);
- write_to_storage_cont();
- return;
- }
-
- module_facet_reader_->GetModuleManifest(
- out_module_data_.module_url,
- [this, flow,
- write_to_storage_cont](fuchsia::modular::ModuleManifestPtr manifest) {
- out_module_data_.module_manifest = std::move(manifest);
- write_to_storage_cont();
- });
+ // Operation stays alive until flow goes out of scope.
+ fuchsia::modular::ModuleData module_data;
+ out_module_data_.Clone(&module_data);
+ story_storage_->WriteModuleData(std::move(module_data))->Then([this, flow] {
+ });
}
StoryStorage* const story_storage_; // Not owned.
fuchsia::modular::ModuleResolver* const module_resolver_; // Not owned.
fuchsia::modular::EntityResolver* const entity_resolver_; // Not owned.
- modular::ModuleFacetReader* const module_facet_reader_; // Not owned.
std::vector<std::string> mod_name_;
fuchsia::modular::Intent intent_;
fuchsia::modular::SurfaceRelationPtr surface_relation_;
@@ -265,7 +242,6 @@
OperationContainer* const container, StoryStorage* const story_storage,
fuchsia::modular::ModuleResolver* const module_resolver,
fuchsia::modular::EntityResolver* const entity_resolver,
- modular::ModuleFacetReader* const module_facet_reader,
std::vector<std::string> mod_name, fuchsia::modular::Intent intent,
fuchsia::modular::SurfaceRelationPtr surface_relation,
std::vector<std::string> surface_parent_mod_name,
@@ -274,8 +250,8 @@
fuchsia::modular::ModuleData)>
done) {
container->Add(new AddModCall(
- story_storage, module_resolver, entity_resolver, module_facet_reader,
- std::move(mod_name), std::move(intent), std::move(surface_relation),
+ story_storage, module_resolver, entity_resolver, std::move(mod_name),
+ std::move(intent), std::move(surface_relation),
std::move(surface_parent_mod_name), module_source, std::move(done)));
}
diff --git a/bin/sessionmgr/puppet_master/command_runners/operation_calls/add_mod_call.h b/bin/sessionmgr/puppet_master/command_runners/operation_calls/add_mod_call.h
index 64dbceb..75d4459 100644
--- a/bin/sessionmgr/puppet_master/command_runners/operation_calls/add_mod_call.h
+++ b/bin/sessionmgr/puppet_master/command_runners/operation_calls/add_mod_call.h
@@ -9,7 +9,6 @@
#include <lib/async/cpp/operation.h>
#include "peridot/bin/sessionmgr/storage/story_storage.h"
-#include "peridot/lib/module_manifest/module_facet_reader.h"
namespace modular {
@@ -17,7 +16,6 @@
OperationContainer* container, StoryStorage* story_storage,
fuchsia::modular::ModuleResolver* module_resolver,
fuchsia::modular::EntityResolver* entity_resolver,
- modular::ModuleFacetReader* module_facet_reader,
std::vector<std::string> mod_name, fuchsia::modular::Intent intent,
fuchsia::modular::SurfaceRelationPtr surface_relation,
std::vector<std::string> surface_parent_mod_name,
diff --git a/bin/sessionmgr/puppet_master/make_production_impl.cc b/bin/sessionmgr/puppet_master/make_production_impl.cc
index 0e49652..0bcd27f 100644
--- a/bin/sessionmgr/puppet_master/make_production_impl.cc
+++ b/bin/sessionmgr/puppet_master/make_production_impl.cc
@@ -27,7 +27,6 @@
fuchsia::modular::FocusProviderPtr focus_provider,
fuchsia::modular::ModuleResolver* const module_resolver,
fuchsia::modular::EntityResolver* const entity_resolver,
- modular::ModuleFacetReader* const module_facet_reader,
// TODO(miguelfrde): we shouldn't create this dependency here. Instead
// an interface similar to StoryStorage should be created for Runtime
// use cases.
@@ -40,8 +39,7 @@
new SetFocusStateCommandRunner(std::move(focus_provider)));
command_runners.emplace(
fuchsia::modular::StoryCommand::Tag::kAddMod,
- new AddModCommandRunner(module_resolver, entity_resolver,
- module_facet_reader));
+ new AddModCommandRunner(module_resolver, entity_resolver));
command_runners.emplace(fuchsia::modular::StoryCommand::Tag::kFocusMod,
new FocusModCommandRunner(std::move(module_focuser)));
command_runners.emplace(fuchsia::modular::StoryCommand::Tag::kRemoveMod,
diff --git a/bin/sessionmgr/puppet_master/make_production_impl.h b/bin/sessionmgr/puppet_master/make_production_impl.h
index 8b79a4d..be9055b 100644
--- a/bin/sessionmgr/puppet_master/make_production_impl.h
+++ b/bin/sessionmgr/puppet_master/make_production_impl.h
@@ -8,7 +8,6 @@
#include <memory>
#include "peridot/bin/sessionmgr/puppet_master/dispatch_story_command_executor.h"
-#include "peridot/lib/module_manifest/module_facet_reader.h"
namespace modular {
@@ -20,7 +19,6 @@
fuchsia::modular::FocusProviderPtr focus_provider,
fuchsia::modular::ModuleResolver* module_resolver,
fuchsia::modular::EntityResolver* entity_resolver,
- modular::ModuleFacetReader* module_facet_reader,
fit::function<void(std::string, std::vector<std::string>)>
module_focuser);
diff --git a/bin/sessionmgr/sessionmgr_impl.cc b/bin/sessionmgr/sessionmgr_impl.cc
index e22870b..1a9470a 100644
--- a/bin/sessionmgr/sessionmgr_impl.cc
+++ b/bin/sessionmgr/sessionmgr_impl.cc
@@ -602,7 +602,6 @@
story_command_executor_ = MakeProductionStoryCommandExecutor(
session_storage_.get(), std::move(focus_provider_puppet_master),
module_resolver_service_.get(), entity_provider_runner_.get(),
- static_cast<modular::ModuleFacetReader*>(module_facet_reader_.get()),
std::move(module_focuser));
story_provider_impl_->Connect(
std::move(story_provider_puppet_master_request));
diff --git a/bin/sessionmgr/storage/story_storage_xdr.cc b/bin/sessionmgr/storage/story_storage_xdr.cc
index 238cb99..3129040 100644
--- a/bin/sessionmgr/storage/story_storage_xdr.cc
+++ b/bin/sessionmgr/storage/story_storage_xdr.cc
@@ -131,7 +131,6 @@
// In previous versions we did not have these fields.
data->parameter_map.entries.resize(0);
- data->module_manifest.reset();
}
void XdrModuleData_v2(XdrContext* const xdr,
@@ -145,9 +144,6 @@
// NOTE: the JSON field naming doesn't match the FIDL struct naming because
// the field name in FIDL was changed.
xdr->Field("chain_data", &data->parameter_map, XdrModuleParameterMap);
-
- // In previous versions we did not have these fields.
- data->module_manifest.reset();
}
void XdrModuleData_v3(XdrContext* const xdr,
@@ -161,7 +157,6 @@
// NOTE: the JSON field naming doesn't match the FIDL struct naming because
// the field name in FIDL was changed.
xdr->Field("chain_data", &data->parameter_map, XdrModuleParameterMap);
- xdr->Field("module_manifest", &data->module_manifest, XdrModuleManifest_v1);
}
void XdrModuleData_v4(XdrContext* const xdr,
@@ -178,7 +173,6 @@
// NOTE: the JSON field naming doesn't match the FIDL struct naming because
// the field name in FIDL was changed.
xdr->Field("chain_data", &data->parameter_map, XdrModuleParameterMap);
- xdr->Field("module_manifest", &data->module_manifest, XdrModuleManifest_v2);
}
void XdrModuleData_v5(XdrContext* const xdr,
@@ -195,7 +189,6 @@
// NOTE: the JSON field naming doesn't match the FIDL struct naming because
// the field name in FIDL was changed.
xdr->Field("chain_data", &data->parameter_map, XdrModuleParameterMap);
- xdr->Field("module_manifest", &data->module_manifest, XdrModuleManifest_v2);
}
XdrFilterType<fuchsia::modular::ModuleData> XdrModuleData[] = {
diff --git a/bin/sessionmgr/story_runner/story_controller_impl.cc b/bin/sessionmgr/story_runner/story_controller_impl.cc
index 0cda4a3..f7ac3af 100644
--- a/bin/sessionmgr/story_runner/story_controller_impl.cc
+++ b/bin/sessionmgr/story_runner/story_controller_impl.cc
@@ -389,10 +389,18 @@
operation_queue_.Add(new LaunchModuleCall(
story_controller_impl_, fidl::Clone(module_data_),
std::move(module_controller_request_), view_owner_.NewRequest(),
- [this, flow] { Cont(flow); }));
+ [this, flow] { LoadModuleManifest(flow); }));
}
- void Cont(FlowToken flow) {
+ void LoadModuleManifest(FlowToken flow) {
+ story_controller_impl_->story_provider_impl_->module_facet_reader()->GetModuleManifest(
+ module_data_.module_url, [this, flow] (fuchsia::modular::ModuleManifestPtr manifest) {
+ module_manifest_ = std::move(manifest);
+ MaybeConnectViewToStoryShell(flow);
+ });
+ }
+
+ void MaybeConnectViewToStoryShell(FlowToken flow) {
// If this is called during Stop(), story_shell_ might already have been
// reset. TODO(mesch): Then the whole operation should fail.
if (!story_controller_impl_->story_shell_) {
@@ -403,26 +411,26 @@
// anchor is already known to story shell.
if (module_data_.module_path.size() == 1) {
- ConnectView(flow, "");
+ ConnectViewToStoryShell(flow, "");
return;
}
auto* const running_mod_info =
story_controller_impl_->FindRunningModInfo(module_data_.module_path);
- FXL_CHECK(running_mod_info); // Was just created.
+ FXL_CHECK(running_mod_info); // This was just created in LaunchModuleCall.
auto* const anchor = story_controller_impl_->FindAnchor(running_mod_info);
if (anchor) {
const auto anchor_surface_id =
ModulePathToSurfaceID(anchor->module_data->module_path);
if (story_controller_impl_->connected_views_.count(anchor_surface_id)) {
- ConnectView(flow, anchor_surface_id);
+ ConnectViewToStoryShell(flow, anchor_surface_id);
return;
}
}
auto manifest_clone = fuchsia::modular::ModuleManifest::New();
- fidl::Clone(module_data_.module_manifest, &manifest_clone);
+ fidl::Clone(module_manifest_, &manifest_clone);
auto surface_relation_clone = fuchsia::modular::SurfaceRelation::New();
module_data_.surface_relation->Clone(surface_relation_clone.get());
story_controller_impl_->pending_views_.emplace(
@@ -432,7 +440,8 @@
module_data_.module_source, std::move(view_owner_)});
}
- void ConnectView(FlowToken flow, fidl::StringPtr anchor_surface_id) {
+ void ConnectViewToStoryShell(FlowToken flow,
+ fidl::StringPtr anchor_surface_id) {
const auto surface_id = ModulePathToSurfaceID(module_data_.module_path);
fuchsia::modular::ViewConnection view_connection;
@@ -442,7 +451,7 @@
fuchsia::modular::SurfaceInfo surface_info;
surface_info.parent_id = anchor_surface_id;
surface_info.surface_relation = std::move(module_data_.surface_relation);
- surface_info.module_manifest = std::move(module_data_.module_manifest);
+ surface_info.module_manifest = std::move(module_manifest_);
surface_info.module_source = std::move(module_data_.module_source);
story_controller_impl_->story_shell_->AddSurface(std::move(view_connection),
std::move(surface_info));
@@ -460,6 +469,8 @@
fuchsia::modular::ModuleControllerPtr module_controller_;
fuchsia::ui::viewsv1token::ViewOwnerPtr view_owner_;
+ fuchsia::modular::ModuleManifestPtr module_manifest_;
+
OperationQueue operation_queue_;
FXL_DISALLOW_COPY_AND_ASSIGN(LaunchModuleInShellCall);
@@ -814,7 +825,6 @@
&operation_queue_, story_controller_impl_->story_storage_,
story_controller_impl_->story_provider_impl_->module_resolver(),
story_controller_impl_->story_provider_impl_->entity_resolver(),
- story_controller_impl_->story_provider_impl_->module_facet_reader(),
std::vector<std::string>({module_name_}), std::move(*intent_),
std::move(surface_relation_), std::move(requesting_module_path_),
module_source_,
diff --git a/public/fidl/fuchsia.modular/module/module_data.fidl b/public/fidl/fuchsia.modular/module/module_data.fidl
index ffa5020..31b36ba 100644
--- a/public/fidl/fuchsia.modular/module/module_data.fidl
+++ b/public/fidl/fuchsia.modular/module/module_data.fidl
@@ -40,12 +40,6 @@
/// required once the framework is cleaned up enough to guarantee this
/// statement.
Intent? intent;
-
- /// The manifest data of the module, resolved from the Intent above. If the
- /// Module instance was not added with an Intent, or with an Intent that
- /// contained a module URL directly, then there is no manifest and this field
- /// is null.
- ModuleManifest? module_manifest;
};
enum ModuleSource {
