[Fuchsia] Update some FIDL APIs to be extensible.

This creates chromium.web.CreateContextParams2 and
chromium.web.LoadUrlParams2 as FIDL tables, which are extensible. All
the call sites for the previous APIs are also updated. The original
APIs will be removed in a future CL when all out-of-tree callers for
them will have been updated.

Bug: 931831
Change-Id: I15ba10012734daa57cdb6034bbaade1253715e0c
Reviewed-on: https://chromium-review.googlesource.com/c/1476184
Commit-Queue: Fabrice de Gans-Riberi <fdegans@chromium.org>
Reviewed-by: Kevin Marshall <kmarshall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#634435}

fuchsia/engine/browser/frame_impl.cc

@@ -447,18 +447,30 @@ void FrameImpl::MaybeSendNavigationEvent() {
 
 void FrameImpl::LoadUrl(std::string url,
                         std::unique_ptr<chromium::web::LoadUrlParams> params) {
+  chromium::web::LoadUrlParams2 converted_params;
+  if (params) {
+    converted_params.set_type(params->type);
+    converted_params.set_referrer(std::move(params->referrer));
+    converted_params.set_user_activated(params->user_activated);
+    converted_params.set_headers(std::move(params->headers));
+  }
+  LoadUrl2(std::move(url), std::move(converted_params));
+}
+
+void FrameImpl::LoadUrl2(std::string url,
+                         chromium::web::LoadUrlParams2 params) {
   GURL validated_url(url);
   if (!validated_url.is_valid()) {
+    // TODO(crbug.com/934539): Add type epitaph.
     DLOG(WARNING) << "Invalid URL: " << url;
     return;
   }
 
   content::NavigationController::LoadURLParams params_converted(validated_url);
-
-  if (params && !params->headers.empty()) {
+  if (params.has_headers()) {
     std::vector<base::StringPiece> extra_headers;
-    extra_headers.reserve(params->headers.size());
-    for (const auto& header : params->headers) {
+    extra_headers.reserve(params.headers()->size());
+    for (const auto& header : *params.headers()) {
       extra_headers.push_back(base::StringPiece(
           reinterpret_cast<const char*>(header.data()), header.size()));
     }
@@ -470,9 +482,11 @@ void FrameImpl::LoadUrl(std::string url,
 
   params_converted.transition_type = ui::PageTransitionFromInt(
       ui::PAGE_TRANSITION_TYPED | ui::PAGE_TRANSITION_FROM_ADDRESS_BAR);
-  params_converted.was_activated = (params && params->user_activated)
-                                       ? content::WasActivatedOption::kYes
-                                       : content::WasActivatedOption::kNo;
+  if (params.has_user_activated() && params.user_activated()) {
+    params_converted.was_activated = content::WasActivatedOption::kYes;
+  } else {
+    params_converted.was_activated = content::WasActivatedOption::kNo;
+  }
   web_contents_->GetController().LoadURLWithParams(params_converted);
 }
 

fuchsia/engine/browser/frame_impl.h

@@ -107,6 +107,7 @@ class FrameImpl : public chromium::web::Frame,
   // chromium::web::NavigationController implementation.
   void LoadUrl(std::string url,
                std::unique_ptr<chromium::web::LoadUrlParams> params) override;
+  void LoadUrl2(std::string url, chromium::web::LoadUrlParams2 params) override;
   void GoBack() override;
   void GoForward() override;
   void Stop() override;

fuchsia/engine/context_provider_impl.cc

@@ -87,17 +87,36 @@ void ContextProviderImpl::SetLaunchCallbackForTests(
 void ContextProviderImpl::Create(
     chromium::web::CreateContextParams params,
     ::fidl::InterfaceRequest<chromium::web::Context> context_request) {
-  DCHECK(context_request.is_valid());
+  chromium::web::CreateContextParams2 convert_params;
 
-  base::CommandLine launch_command = *base::CommandLine::ForCurrentProcess();
+  convert_params.set_service_directory(std::move(params.service_directory));
+  if (params.data_directory) {
+    convert_params.set_data_directory(std::move(params.data_directory));
+  }
+  Create2(std::move(convert_params), std::move(context_request));
+}
 
-  base::LaunchOptions launch_options;
+void ContextProviderImpl::Create2(
+    chromium::web::CreateContextParams2 params,
+    ::fidl::InterfaceRequest<chromium::web::Context> context_request) {
+  if (!context_request.is_valid()) {
+    // TODO(crbug.com/934539): Add type epitaph.
+    DLOG(WARNING) << "Invalid |context_request|.";
+    return;
+  }
+  if (!params.has_service_directory()) {
+    // TODO(crbug.com/934539): Add type epitaph.
+    DLOG(WARNING)
+        << "Missing argument |service_directory| in CreateContextParams2.";
+    return;
+  }
 
+  base::LaunchOptions launch_options;
   service_manager::SandboxPolicyFuchsia sandbox_policy;
   sandbox_policy.Initialize(service_manager::SANDBOX_TYPE_WEB_CONTEXT);
   sandbox_policy.SetServiceDirectory(
       fidl::InterfaceHandle<::fuchsia::io::Directory>(
-          std::move(params.service_directory)));
+          std::move(*params.mutable_service_directory())));
   sandbox_policy.UpdateLaunchOptionsForSandbox(&launch_options);
 
   if (use_shared_tmp_)
@@ -110,24 +129,36 @@ void ContextProviderImpl::Create(
       {kContextRequestHandleId, context_handle.get()});
 
   // Bind |data_directory| to /data directory, if provided.
-  if (params.data_directory) {
-    if (!IsValidDirectory(params.data_directory.get()))
+  if (params.has_data_directory()) {
+    if (!IsValidDirectory(params.data_directory()->get())) {
+      // TODO(crbug.com/934539): Add type epitaph.
+      DLOG(WARNING)
+          << "Invalid argument |data_directory| in CreateContextParams2.";
       return;
+    }
 
     base::FilePath data_path;
-    CHECK(base::PathService::Get(base::DIR_APP_DATA, &data_path));
-    launch_options.paths_to_transfer.push_back(
-        base::PathToTransfer{data_path, params.data_directory.release()});
+    if (!base::PathService::Get(base::DIR_APP_DATA, &data_path)) {
+      // TODO(crbug.com/934539): Add type epitaph.
+      DLOG(WARNING) << "Failed to get data directory service path.";
+      return;
+    }
+    launch_options.paths_to_transfer.push_back(base::PathToTransfer{
+        data_path, params.mutable_data_directory()->release()});
   }
 
   // Isolate the child Context processes by containing them within their own
   // respective jobs.
   zx::job job;
   zx_status_t status = zx::job::create(*base::GetDefaultJob(), 0, &job);
-  ZX_CHECK(status == ZX_OK, status) << "zx_job_create";
+  if (status != ZX_OK) {
+    ZX_LOG(FATAL, status) << "zx_job_create";
+    return;
+  }
   launch_options.job_handle = job.get();
 
-  ignore_result(launch_.Run(std::move(launch_command), launch_options));
+  ignore_result(launch_.Run(std::move(*base::CommandLine::ForCurrentProcess()),
+                            launch_options));
 
   ignore_result(context_handle.release());
 }

fuchsia/engine/context_provider_impl.h

@@ -39,6 +39,9 @@ class WEB_ENGINE_EXPORT ContextProviderImpl
   void Create(chromium::web::CreateContextParams params,
               ::fidl::InterfaceRequest<chromium::web::Context> context_request)
       override;
+  void Create2(chromium::web::CreateContextParams2 params,
+               ::fidl::InterfaceRequest<chromium::web::Context> context_request)
+      override;
 
  private:
   using LaunchContextProcessCallback = base::RepeatingCallback<base::Process(

fuchsia/engine/context_provider_impl_unittest.cc

@@ -136,7 +136,22 @@ class ContextProviderImplTest : public base::MultiProcessTest {
     EXPECT_EQ(change_observer.captured_event().title, kTitle);
   }
 
-  chromium::web::CreateContextParams BuildCreateContextParams() {
+  chromium::web::CreateContextParams2 BuildCreateContextParams() {
+    zx::channel client_channel;
+    zx::channel server_channel;
+    zx_status_t result =
+        zx::channel::create(0, &client_channel, &server_channel);
+    ZX_CHECK(result == ZX_OK, result) << "zx_channel_create()";
+    result = fdio_service_connect("/svc/.", server_channel.release());
+    ZX_CHECK(result == ZX_OK, result) << "Failed to open /svc";
+
+    chromium::web::CreateContextParams2 output;
+    output.set_service_directory(std::move(client_channel));
+    return output;
+  }
+
+  // TODO(crbug.com/931831): Remove this method once the transition is complete.
+  chromium::web::CreateContextParams BuildDeprecatedCreateContextParams() {
     zx::channel client_channel;
     zx::channel server_channel;
     zx_status_t result =
@@ -199,7 +214,18 @@ class ContextProviderImplTest : public base::MultiProcessTest {
 TEST_F(ContextProviderImplTest, LaunchContext) {
   // Connect to a new context process.
   fidl::InterfacePtr<chromium::web::Context> context;
-  chromium::web::CreateContextParams create_params = BuildCreateContextParams();
+  chromium::web::CreateContextParams2 create_params =
+      BuildCreateContextParams();
+  provider_ptr_->Create2(std::move(create_params), context.NewRequest());
+  CheckContextResponsive(&context);
+}
+
+// TODO(crbug.com/931831): Remove this test once the transition is complete.
+TEST_F(ContextProviderImplTest, DeprecatedLaunchContext) {
+  // Connect to a new context process.
+  fidl::InterfacePtr<chromium::web::Context> context;
+  chromium::web::CreateContextParams create_params =
+      BuildDeprecatedCreateContextParams();
   provider_ptr_->Create(std::move(create_params), context.NewRequest());
   CheckContextResponsive(&context);
 }
@@ -209,13 +235,13 @@ TEST_F(ContextProviderImplTest, MultipleConcurrentClients) {
   chromium::web::ContextProviderPtr provider_1_ptr;
   provider_->Bind(provider_1_ptr.NewRequest());
   chromium::web::ContextPtr context_1;
-  provider_1_ptr->Create(BuildCreateContextParams(), context_1.NewRequest());
+  provider_1_ptr->Create2(BuildCreateContextParams(), context_1.NewRequest());
 
   // Do the same on another Provider connection.
   chromium::web::ContextProviderPtr provider_2_ptr;
   provider_->Bind(provider_2_ptr.NewRequest());
   chromium::web::ContextPtr context_2;
-  provider_2_ptr->Create(BuildCreateContextParams(), context_2.NewRequest());
+  provider_2_ptr->Create2(BuildCreateContextParams(), context_2.NewRequest());
 
   CheckContextResponsive(&context_1);
   CheckContextResponsive(&context_2);
@@ -223,7 +249,7 @@ TEST_F(ContextProviderImplTest, MultipleConcurrentClients) {
   // Ensure that the initial ContextProvider connection is still usable, by
   // creating and verifying another Context from it.
   chromium::web::ContextPtr context_3;
-  provider_2_ptr->Create(BuildCreateContextParams(), context_3.NewRequest());
+  provider_2_ptr->Create2(BuildCreateContextParams(), context_3.NewRequest());
   CheckContextResponsive(&context_3);
 }
 
@@ -232,7 +258,40 @@ TEST_F(ContextProviderImplTest, WithProfileDir) {
 
   // Connect to a new context process.
   fidl::InterfacePtr<chromium::web::Context> context;
-  chromium::web::CreateContextParams create_params = BuildCreateContextParams();
+  chromium::web::CreateContextParams2 create_params =
+      BuildCreateContextParams();
+
+  // Setup data dir.
+  EXPECT_TRUE(profile_temp_dir.CreateUniqueTempDir());
+  ASSERT_EQ(
+      base::WriteFile(profile_temp_dir.GetPath().AppendASCII(kTestDataFileIn),
+                      nullptr, 0),
+      0);
+
+  // Pass a handle data dir to the context.
+  create_params.set_data_directory(
+      zx::channel(base::fuchsia::GetHandleFromFile(
+                      base::File(profile_temp_dir.GetPath(),
+                                 base::File::FLAG_OPEN | base::File::FLAG_READ))
+                      .release()));
+
+  provider_ptr_->Create2(std::move(create_params), context.NewRequest());
+
+  CheckContextResponsive(&context);
+
+  // Verify that the context process can write to the data dir.
+  EXPECT_TRUE(base::PathExists(
+      profile_temp_dir.GetPath().AppendASCII(kTestDataFileOut)));
+}
+
+// TODO(crbug.com/931831): Remove this test once the transition is complete.
+TEST_F(ContextProviderImplTest, DeprecatedWithProfileDir) {
+  base::ScopedTempDir profile_temp_dir;
+
+  // Connect to a new context process.
+  fidl::InterfacePtr<chromium::web::Context> context;
+  chromium::web::CreateContextParams create_params =
+      BuildDeprecatedCreateContextParams();
 
   // Setup data dir.
   EXPECT_TRUE(profile_temp_dir.CreateUniqueTempDir());
@@ -262,16 +321,17 @@ TEST_F(ContextProviderImplTest, FailsDataDirectoryIsFile) {
 
   // Connect to a new context process.
   fidl::InterfacePtr<chromium::web::Context> context;
-  chromium::web::CreateContextParams create_params = BuildCreateContextParams();
+  chromium::web::CreateContextParams2 create_params =
+      BuildCreateContextParams();
 
   // Pass in a handle to a file instead of a directory.
   CHECK(base::CreateTemporaryFile(&temp_file_path));
   base::File temp_file(temp_file_path,
                        base::File::FLAG_OPEN | base::File::FLAG_READ);
-  create_params.data_directory.reset(
-      base::fuchsia::GetHandleFromFile(std::move(temp_file)).release());
+  create_params.set_data_directory(zx::channel(
+      base::fuchsia::GetHandleFromFile(std::move(temp_file)).release()));
 
-  provider_ptr_->Create(std::move(create_params), context.NewRequest());
+  provider_ptr_->Create2(std::move(create_params), context.NewRequest());
 
   CheckContextUnresponsive(&context);
 }
@@ -301,7 +361,7 @@ TEST_F(ContextProviderImplTest, CleansUpContextJobs) {
 
   // Create a Context and verify that it is functional.
   chromium::web::ContextPtr context;
-  provider->Create(BuildCreateContextParams(), context.NewRequest());
+  provider->Create2(BuildCreateContextParams(), context.NewRequest());
   CheckContextResponsive(&context);
 
   // Verify that there is at least one job under our default job.

fuchsia/fidl/web/context_provider.fidl

@@ -13,9 +13,13 @@ protocol ContextProvider {
   ///
   /// context: An interface request which will receive a bound Context
   ///          service.
+  // DEPRECATED Use Create2 instead.
   Create(CreateContextParams params, request<Context> context);
+
+  Create2(CreateContextParams2 params, request<Context> context);
 };
 
+// DEPRECATED Use CreateContextParams2 instead.
 struct CreateContextParams {
   /// Service directory to be used by the context.
   // TODO(https://crbug.com/870057): Document required and optional services
@@ -27,3 +31,15 @@ struct CreateContextParams {
   /// stateless, with all of its data discarded upon Context destruction.
   handle<channel>? data_directory;
 };
+
+table CreateContextParams2 {
+  /// Service directory to be used by the context.
+  // TODO(https://crbug.com/870057): Document required and optional services
+  // that Context needs.
+  1: handle<channel> service_directory;
+
+  /// Handle to the directory that will contain the Context's
+  /// persistent data. If it is left unset, then the created Context will be
+  /// stateless, with all of its data discarded upon Context destruction.
+  2: handle<channel> data_directory;
+};

fuchsia/fidl/web/navigation_controller.fidl

@@ -12,8 +12,11 @@ protocol NavigationController {
   /// |url|:    The address to navigate to.
   /// |params|: Additional parameters that affect how the resource will be
   ///           loaded (e.g. cookies, HTTP headers, etc.)
+  // DEPRECATED Use LoadUrl2 instead.
   LoadUrl(string url, LoadUrlParams? params);
 
+  LoadUrl2(string url, LoadUrlParams2 params);
+
   GoBack();
   GoForward();
   Stop();
@@ -25,6 +28,7 @@ protocol NavigationController {
 };
 
 /// Additional parameters for modifying the behavior of LoadUrl().
+// DEPRECATED Use LoadUrlParams2 instead.
 struct LoadUrlParams {
   /// Provides a hint to the browser UI about how LoadUrl was triggered.
   LoadUrlReason type;
@@ -42,6 +46,24 @@ struct LoadUrlParams {
   vector<bytes> headers;
 };
 
+/// Additional parameters for modifying the behavior of LoadUrl().
+table LoadUrlParams2 {
+  /// Provides a hint to the browser UI about how LoadUrl was triggered.
+  1: LoadUrlReason type;
+
+  /// The URL that linked to the resource being requested.
+  2: string referrer;
+
+  /// Should be set to true to propagate user activation to the frame. User
+  /// activation implies that the user is interacting with the web frame. It
+  /// enables some web features that are not available otherwise. For example
+  /// autoplay will work only when this flag is set to true.
+  3: bool user_activated;
+
+  /// Custom HTTP headers.
+  4: vector<vector<uint8>> headers;
+};
+
 /// Characterizes the origin of a LoadUrl request.
 enum LoadUrlReason {
   /// Navigation was initiated by the user following a link.

fuchsia/runners/common/web_component.cc

@@ -27,10 +27,10 @@ void WebComponent::LoadUrl(const GURL& url) {
   // Set the page activation flag on the initial load, so that features like
   // autoplay work as expected when a WebComponent first loads the specified
   // content.
-  auto params = std::make_unique<chromium::web::LoadUrlParams>();
-  params->user_activated = true;
+  chromium::web::LoadUrlParams2 params;
+  params.set_user_activated(true);
 
-  navigation_controller->LoadUrl(url.spec(), std::move(params));
+  navigation_controller->LoadUrl2(url.spec(), std::move(params));
 }
 
 WebComponent::WebComponent(

fuchsia/runners/common/web_content_runner.cc

@@ -26,17 +26,17 @@ chromium::web::ContextPtr WebContentRunner::CreateDefaultWebContext() {
       base::fuchsia::ServiceDirectoryClient::ForCurrentProcess()
           ->ConnectToService<chromium::web::ContextProvider>();
 
-  chromium::web::CreateContextParams create_params;
+  chromium::web::CreateContextParams2 create_params;
 
   // Clone /svc to the context.
-  create_params.service_directory =
+  create_params.set_service_directory(
       zx::channel(base::fuchsia::GetHandleFromFile(
           base::File(base::FilePath("/svc"),
-                     base::File::FLAG_OPEN | base::File::FLAG_READ)));
+                     base::File::FLAG_OPEN | base::File::FLAG_READ))));
 
   chromium::web::ContextPtr web_context;
-  web_context_provider->Create(std::move(create_params),
-                               web_context.NewRequest());
+  web_context_provider->Create2(std::move(create_params),
+                                web_context.NewRequest());
   web_context.set_error_handler([](zx_status_t status) {
     // If the browser instance died, then exit everything and do not attempt
     // to recover. appmgr will relaunch the runner when it is needed again.