Revert "[Fuchsia] Update some FIDL APIs to be extensible."

This reverts commit 7da3d9df.

Reason for revert: Looks like my review comments raced with Kevin's LGTM and landing - sorry. :(  Reverting since I'm asking for a FIDL change, so once we push this we can't fix it up without more soft-transitioning.

Original change's description:
> [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}

TBR=wez@chromium.org,kmarshall@chromium.org,fdegans@chromium.org

Change-Id: If592b2fb73941e57af5b253d8e040e18f6559771
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 931831
Reviewed-on: https://chromium-review.googlesource.com/c/1480728Reviewed-by: Wez <wez@chromium.org>
Commit-Queue: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#634443}

fuchsia/engine/browser/frame_impl.cc

@@ -447,30 +447,18 @@ 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.has_headers()) {
+
+  if (params && !params->headers.empty()) {
     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()));
     }
@@ -482,11 +470,9 @@ void FrameImpl::LoadUrl2(std::string url,
 
   params_converted.transition_type = ui::PageTransitionFromInt(
       ui::PAGE_TRANSITION_TYPED | ui::PAGE_TRANSITION_FROM_ADDRESS_BAR);
-  if (params.has_user_activated() && params.user_activated()) {
-    params_converted.was_activated = content::WasActivatedOption::kYes;
-  } else {
-    params_converted.was_activated = content::WasActivatedOption::kNo;
-  }
+  params_converted.was_activated = (params && params->user_activated)
+                                       ? content::WasActivatedOption::kYes
+                                       : content::WasActivatedOption::kNo;
   web_contents_->GetController().LoadURLWithParams(params_converted);
 }
 

fuchsia/engine/browser/frame_impl.h

@@ -107,7 +107,6 @@ 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,36 +87,17 @@ void ContextProviderImpl::SetLaunchCallbackForTests(
 void ContextProviderImpl::Create(
     chromium::web::CreateContextParams params,
     ::fidl::InterfaceRequest<chromium::web::Context> context_request) {
-  chromium::web::CreateContextParams2 convert_params;
+  DCHECK(context_request.is_valid());
 
-  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));
-}
-
-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::CommandLine launch_command = *base::CommandLine::ForCurrentProcess();
 
   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.mutable_service_directory())));
+          std::move(params.service_directory)));
   sandbox_policy.UpdateLaunchOptionsForSandbox(&launch_options);
 
   if (use_shared_tmp_)
@@ -129,36 +110,24 @@ void ContextProviderImpl::Create2(
       {kContextRequestHandleId, context_handle.get()});
 
   // Bind |data_directory| to /data directory, if provided.
-  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.";
+  if (params.data_directory) {
+    if (!IsValidDirectory(params.data_directory.get()))
       return;
-    }
 
     base::FilePath data_path;
-    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()});
+    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()});
   }
 
   // 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);
-  if (status != ZX_OK) {
-    ZX_LOG(FATAL, status) << "zx_job_create";
-    return;
-  }
+  ZX_CHECK(status == ZX_OK, status) << "zx_job_create";
   launch_options.job_handle = job.get();
 
-  ignore_result(launch_.Run(std::move(*base::CommandLine::ForCurrentProcess()),
-                            launch_options));
+  ignore_result(launch_.Run(std::move(launch_command), launch_options));
 
   ignore_result(context_handle.release());
 }

fuchsia/engine/context_provider_impl.h

@@ -39,9 +39,6 @@ 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,22 +136,7 @@ class ContextProviderImplTest : public base::MultiProcessTest {
     EXPECT_EQ(change_observer.captured_event().title, kTitle);
   }
 
-  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() {
+  chromium::web::CreateContextParams BuildCreateContextParams() {
     zx::channel client_channel;
     zx::channel server_channel;
     zx_status_t result =
@@ -214,18 +199,7 @@ class ContextProviderImplTest : public base::MultiProcessTest {
 TEST_F(ContextProviderImplTest, LaunchContext) {
   // Connect to a new context process.
   fidl::InterfacePtr<chromium::web::Context> context;
-  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();
+  chromium::web::CreateContextParams create_params = BuildCreateContextParams();
   provider_ptr_->Create(std::move(create_params), context.NewRequest());
   CheckContextResponsive(&context);
 }
@@ -235,13 +209,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->Create2(BuildCreateContextParams(), context_1.NewRequest());
+  provider_1_ptr->Create(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->Create2(BuildCreateContextParams(), context_2.NewRequest());
+  provider_2_ptr->Create(BuildCreateContextParams(), context_2.NewRequest());
 
   CheckContextResponsive(&context_1);
   CheckContextResponsive(&context_2);
@@ -249,7 +223,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->Create2(BuildCreateContextParams(), context_3.NewRequest());
+  provider_2_ptr->Create(BuildCreateContextParams(), context_3.NewRequest());
   CheckContextResponsive(&context_3);
 }
 
@@ -258,40 +232,7 @@ TEST_F(ContextProviderImplTest, WithProfileDir) {
 
   // Connect to a new context process.
   fidl::InterfacePtr<chromium::web::Context> context;
-  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();
+  chromium::web::CreateContextParams create_params = BuildCreateContextParams();
 
   // Setup data dir.
   EXPECT_TRUE(profile_temp_dir.CreateUniqueTempDir());
@@ -321,17 +262,16 @@ TEST_F(ContextProviderImplTest, FailsDataDirectoryIsFile) {
 
   // Connect to a new context process.
   fidl::InterfacePtr<chromium::web::Context> context;
-  chromium::web::CreateContextParams2 create_params =
-      BuildCreateContextParams();
+  chromium::web::CreateContextParams 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.set_data_directory(zx::channel(
-      base::fuchsia::GetHandleFromFile(std::move(temp_file)).release()));
+  create_params.data_directory.reset(
+      base::fuchsia::GetHandleFromFile(std::move(temp_file)).release());
 
-  provider_ptr_->Create2(std::move(create_params), context.NewRequest());
+  provider_ptr_->Create(std::move(create_params), context.NewRequest());
 
   CheckContextUnresponsive(&context);
 }
@@ -361,7 +301,7 @@ TEST_F(ContextProviderImplTest, CleansUpContextJobs) {
 
   // Create a Context and verify that it is functional.
   chromium::web::ContextPtr context;
-  provider->Create2(BuildCreateContextParams(), context.NewRequest());
+  provider->Create(BuildCreateContextParams(), context.NewRequest());
   CheckContextResponsive(&context);
 
   // Verify that there is at least one job under our default job.

fuchsia/fidl/web/context_provider.fidl

@@ -13,13 +13,9 @@ 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
@@ -31,15 +27,3 @@ 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,11 +12,8 @@ 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();
@@ -28,7 +25,6 @@ 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;
@@ -46,24 +42,6 @@ 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.
-  chromium::web::LoadUrlParams2 params;
-  params.set_user_activated(true);
+  auto params = std::make_unique<chromium::web::LoadUrlParams>();
+  params->user_activated = true;
 
-  navigation_controller->LoadUrl2(url.spec(), std::move(params));
+  navigation_controller->LoadUrl(url.spec(), std::move(params));
 }
 
 WebComponent::WebComponent(

fuchsia/runners/common/web_content_runner.cc

@@ -26,16 +26,16 @@ chromium::web::ContextPtr WebContentRunner::CreateDefaultWebContext() {
       base::fuchsia::ServiceDirectoryClient::ForCurrentProcess()
           ->ConnectToService<chromium::web::ContextProvider>();
 
-  chromium::web::CreateContextParams2 create_params;
+  chromium::web::CreateContextParams create_params;
 
   // Clone /svc to the context.
-  create_params.set_service_directory(
+  create_params.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->Create2(std::move(create_params),
+  web_context_provider->Create(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