Clean up EDK layering

As a precursor to moving SDK thunks around and rewiring them, this
fixes some of the weird layering within //mojo/edk.

Namely this introduces a new //mojo/edk target to subsume the role
of //mojo/edk/system (aliased for now to avoid churn), and splits out
a source_set (//mojo/edk:core) for the bulk of the EDK impl with no
linkage dependencies on //mojo/public.

A follow-up patch (https://crrev.com/c/963548) will get rid of the
alias targets.

Bug: 822034
Change-Id: I6b45ce6e8bb9b3a5392ae8e5cdfb61bbc4761769
Reviewed-on: https://chromium-review.googlesource.com/963766
Commit-Queue: Ken Rockot <rockot@chromium.org>
Reviewed-by: Jay Civelli <jcivelli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543528}

mojo/edk/BUILD.gn

@@ -0,0 +1,113 @@
+# Copyright 2018 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import("//build/config/nacl/config.gni")
+
+# Targets should depend on this if directly referencing the |mojo::edk|
+# namespace.
+component("edk") {
+  output_name = "mojo_edk"
+
+  public = [
+    "embedder/embedder.h",
+    "embedder/incoming_broker_client_invitation.h",
+    "embedder/outgoing_broker_client_invitation.h",
+    "embedder/peer_connection.h",
+  ]
+
+  sources = [
+    "embedder/embedder.cc",
+    "embedder/incoming_broker_client_invitation.cc",
+    "embedder/outgoing_broker_client_invitation.cc",
+    "embedder/peer_connection.cc",
+  ]
+
+  defines = [ "MOJO_SYSTEM_IMPL_IMPLEMENTATION" ]
+
+  deps = []
+  if (!is_nacl) {
+    deps += [ "//crypto" ]
+  }
+
+  public_deps = [
+    ":core",
+    "//mojo/public/cpp/system",
+  ]
+}
+
+# Bits of the EDK library which do not depend on public API linkage. It is
+# not allowed for this target or any of its transitive dependencies to depend
+# on anything under //mojo/public beyond strict C type definitions.
+source_set("core") {
+  visibility = [ ":edk" ]
+
+  public = [
+    "embedder/configuration.h",
+    "embedder/connection_params.h",
+    "embedder/embedder_internal.h",
+    "embedder/entrypoints.h",
+    "embedder/named_platform_channel_pair.h",
+    "embedder/named_platform_handle.h",
+    "embedder/named_platform_handle_utils.h",
+    "embedder/platform_channel_pair.h",
+    "embedder/platform_handle.h",
+    "embedder/platform_handle_utils.h",
+    "embedder/process_error_callback.h",
+    "embedder/scoped_ipc_support.h",
+    "embedder/scoped_platform_handle.h",
+    "embedder/transport_protocol.h",
+  ]
+
+  sources = [
+    "embedder/connection_params.cc",
+    "embedder/entrypoints.cc",
+    "embedder/named_platform_channel_pair_win.cc",
+    "embedder/named_platform_handle_utils_win.cc",
+    "embedder/platform_channel_pair.cc",
+    "embedder/platform_channel_pair_win.cc",
+    "embedder/platform_handle.cc",
+    "embedder/platform_handle_utils_win.cc",
+    "embedder/platform_shared_buffer.cc",
+    "embedder/scoped_ipc_support.cc",
+  ]
+
+  if (is_fuchsia) {
+    sources += [
+      "embedder/named_platform_handle_utils_fuchsia.cc",
+      "embedder/platform_channel_pair_fuchsia.cc",
+      "embedder/platform_handle_utils_fuchsia.cc",
+    ]
+  } else if (is_posix) {
+    public += [ "embedder/platform_channel_utils_posix.h" ]
+
+    sources += [
+      "embedder/platform_channel_pair_posix.cc",
+      "embedder/platform_channel_utils_posix.cc",
+      "embedder/platform_handle_utils_posix.cc",
+    ]
+
+    if (!is_nacl) {
+      sources += [ "embedder/named_platform_handle_utils_posix.cc" ]
+    }
+  }
+
+  if (is_nacl && !is_nacl_nonsfi) {
+    sources -= [ "embedder/platform_channel_utils_posix.cc" ]
+  }
+
+  defines = [ "MOJO_SYSTEM_IMPL_IMPLEMENTATION" ]
+
+  public_deps = [
+    "//base",
+    "//mojo/edk/system:system_impl",
+    "//mojo/public/c/system:headers",
+  ]
+
+  deps = []
+  if (is_android) {
+    deps += [ "//third_party/ashmem" ]
+  }
+
+  allow_circular_includes_from = [ "//mojo/edk/system:system_impl" ]
+}

mojo/edk/embedder/BUILD.gn

@@ -2,142 +2,6 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
-import("//build/config/nacl/config.gni")
-
-source_set("headers") {
-  sources = [
-    "configuration.h",
-    "connection_params.h",
-    "embedder.h",
-    "embedder_internal.h",
-    "incoming_broker_client_invitation.h",
-    "named_platform_channel_pair.h",
-    "named_platform_handle.h",
-    "named_platform_handle_utils.h",
-    "outgoing_broker_client_invitation.h",
-    "peer_connection.h",
-    "platform_channel_pair.h",
-    "platform_handle.h",
-    "platform_handle_utils.h",
-    "scoped_ipc_support.h",
-    "scoped_platform_handle.h",
-    "transport_protocol.h",
-  ]
-
-  public_deps = [
-    "//base",
-    "//mojo/public/cpp/system",
-  ]
-}
-
-source_set("embedder") {
-  # This isn't really a standalone target; it must be linked into the
-  # mojo_system_impl component.
-  visibility = [
-    "//mojo/edk/system",
-    "//components/nacl:nacl",
-  ]
-
-  sources = [
-    "configuration.h",
-    "connection_params.cc",
-    "embedder.cc",
-    "entrypoints.cc",
-    "entrypoints.h",
-    "incoming_broker_client_invitation.cc",
-    "outgoing_broker_client_invitation.cc",
-    "peer_connection.cc",
-    "scoped_ipc_support.cc",
-
-    # Test-only code:
-    # TODO(vtl): It's a little unfortunate that these end up in the same
-    # component as non-test-only code. In the static build, this code should
-    # hopefully be dead-stripped.
-    "test_embedder.cc",
-    "test_embedder.h",
-  ]
-
-  defines = [ "MOJO_SYSTEM_IMPL_IMPLEMENTATION" ]
-
-  deps = [
-    "//mojo/edk/system/ports",
-  ]
-
-  public_deps = [
-    ":headers",
-    ":platform",
-    "//base",
-    "//mojo/public/cpp/system",
-  ]
-
-  if (!is_nacl) {
-    deps += [ "//crypto" ]
-  }
-}
-
-source_set("platform") {
-  # This isn't really a standalone target; it must be linked into the
-  # mojo_system_impl component.
-  visibility = [
-    ":embedder",
-    "//mojo/edk/system",
-  ]
-
-  sources = [
-    "named_platform_channel_pair.h",
-    "named_platform_channel_pair_win.cc",
-    "named_platform_handle.h",
-    "named_platform_handle_utils.h",
-    "named_platform_handle_utils_win.cc",
-    "platform_channel_pair.cc",
-    "platform_channel_pair.h",
-    "platform_channel_pair_win.cc",
-    "platform_handle.cc",
-    "platform_handle.h",
-    "platform_handle_utils.h",
-    "platform_handle_utils_win.cc",
-    "platform_shared_buffer.cc",
-    "platform_shared_buffer.h",
-    "scoped_platform_handle.h",
-  ]
-
-  if (is_fuchsia) {
-    sources += [
-      "named_platform_handle_utils_fuchsia.cc",
-      "platform_channel_pair_fuchsia.cc",
-      "platform_handle_utils_fuchsia.cc",
-    ]
-  } else if (is_posix) {
-    sources += [
-      "platform_channel_pair_posix.cc",
-      "platform_channel_utils_posix.cc",
-      "platform_channel_utils_posix.h",
-      "platform_handle_utils_posix.cc",
-    ]
-    if (!is_nacl) {
-      sources += [ "named_platform_handle_utils_posix.cc" ]
-    }
-  }
-
-  if (is_nacl && !is_nacl_nonsfi) {
-    sources -= [ "platform_channel_utils_posix.cc" ]
-  }
-
-  defines = [ "MOJO_SYSTEM_IMPL_IMPLEMENTATION" ]
-
-  public_deps = [
-    "//mojo/public/cpp/system",
-  ]
-
-  deps = [
-    "//base",
-  ]
-
-  if (is_android) {
-    deps += [ "//third_party/ashmem" ]
-  }
-}
-
 source_set("embedder_unittests") {
   testonly = true
 
@@ -162,3 +26,11 @@ source_set("embedder_unittests") {
     "//testing/gtest",
   ]
 }
+
+# Temporary alias until dependents can be updated. This will be imminently
+# deleted. There is no compelling reason to have a headers target for the EDK.
+group("headers") {
+  public_deps = [
+    "//mojo/edk",
+  ]
+}

mojo/edk/embedder/README.md

@@ -8,10 +8,9 @@ This document is a subset of the [Mojo documentation](/mojo).
 The Mojo EDK is a (binary-unstable) API which enables a process to use Mojo both
 internally and for IPC to other Mojo-embedding processes.
 
-Using any of the API surface in `//mojo/edk/embedder` requires (somewhat
-confusingly) a direct dependency on the GN `//mojo/edk/system` target. Despite
-this fact, you should never reference any of the headers in `mojo/edk/system`
-directly, as everything there is considered to be an internal detail of the EDK.
+Using any of the API surface in `//mojo/edk/embedder` requires a direct
+dependency on the GN `//mojo/edk` target. Headers in `mojo/edk/system` are
+reserved for internal use by the EDK only.
 
 **NOTE:** Unless you are introducing a new binary entry point into the system
 (*e.g.,* a new executable with a new `main()` definition), you probably don't

mojo/edk/embedder/incoming_broker_client_invitation.cc

@@ -38,7 +38,8 @@ IncomingBrokerClientInvitation::AcceptFromCommandLine(
 
 ScopedMessagePipeHandle IncomingBrokerClientInvitation::ExtractMessagePipe(
     const std::string& name) {
-  return internal::g_core->ExtractMessagePipeFromInvitation(name);
+  return ScopedMessagePipeHandle(MessagePipeHandle(
+      internal::g_core->ExtractMessagePipeFromInvitation(name)));
 }
 
 IncomingBrokerClientInvitation::IncomingBrokerClientInvitation(

mojo/edk/embedder/outgoing_broker_client_invitation.cc

@@ -28,8 +28,8 @@ ScopedMessagePipeHandle OutgoingBrokerClientInvitation::AttachMessagePipe(
     const std::string& name) {
   DCHECK(!sent_);
   ports::PortRef port;
-  ScopedMessagePipeHandle pipe =
-      internal::g_core->CreatePartialMessagePipe(&port);
+  ScopedMessagePipeHandle pipe = ScopedMessagePipeHandle(
+      MessagePipeHandle(internal::g_core->CreatePartialMessagePipe(&port)));
   attached_ports_.emplace_back(name, port);
   return pipe;
 }
@@ -42,8 +42,8 @@ OutgoingBrokerClientInvitation::ExtractInProcessMessagePipe(
   // a single entry.
   for (auto it = attached_ports_.begin(); it != attached_ports_.end(); ++it) {
     if (it->first == name) {
-      ScopedMessagePipeHandle pipe =
-          internal::g_core->CreatePartialMessagePipe(it->second);
+      ScopedMessagePipeHandle pipe = ScopedMessagePipeHandle(MessagePipeHandle(
+          internal::g_core->CreatePartialMessagePipe(it->second)));
       attached_ports_.erase(it);
       return pipe;
     }

mojo/edk/embedder/peer_connection.cc

@@ -22,7 +22,8 @@ ScopedMessagePipeHandle PeerConnection::Connect(ConnectionParams params) {
   is_connected_ = true;
 
   ports::PortRef peer_port;
-  auto pipe = internal::g_core->CreatePartialMessagePipe(&peer_port);
+  auto pipe = ScopedMessagePipeHandle(MessagePipeHandle(
+      internal::g_core->CreatePartialMessagePipe(&peer_port)));
   connection_id_ =
       internal::g_core->ConnectToPeer(std::move(params), peer_port);
   return pipe;

mojo/edk/embedder/process_error_callback.h

@@ -0,0 +1,21 @@
+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef MOJO_EDK_EMBEDDER_PROCESS_ERROR_CALLBACK_H_
+#define MOJO_EDK_EMBEDDER_PROCESS_ERROR_CALLBACK_H_
+
+#include <string>
+
+#include "base/callback.h"
+
+namespace mojo {
+namespace edk {
+
+using ProcessErrorCallback =
+    base::RepeatingCallback<void(const std::string& error)>;
+
+}  // namespace edk
+}  // namespace mojo
+
+#endif  // MOJO_EDK_EMBEDDER_PROCESS_ERROR_CALLBACK_H_

mojo/edk/embedder/scoped_ipc_support.cc

@@ -8,7 +8,6 @@
 #include "base/bind_helpers.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/threading/thread_restrictions.h"
-#include "mojo/edk/embedder/embedder.h"
 #include "mojo/edk/embedder/embedder_internal.h"
 #include "mojo/edk/system/core.h"
 

mojo/edk/system/BUILD.gn

@@ -10,8 +10,18 @@ if (is_android) {
   import("//build/config/android/rules.gni")
 }
 
-component("system") {
-  output_name = "mojo_system_impl"
+# Temporary alias to avoid having to update the world with a new target
+# structure just yet. This will be imminently deleted.
+group("system") {
+  public_deps = [
+    "//mojo/edk",
+  ]
+}
+
+source_set("system_impl") {
+  # All sources in this target should really just be private sources in
+  # "//mojo/edk:core". Make sure that's the only target than can see this one.
+  visibility = [ "//mojo/edk:core" ]
 
   sources = [
     "atomic_flag.h",
@@ -89,11 +99,8 @@ component("system") {
   defines = [ "MOJO_SYSTEM_IMPL_IMPLEMENTATION" ]
 
   public_deps = [
-    "//mojo/edk/embedder",
-    "//mojo/edk/embedder:platform",
     "//mojo/edk/system/ports",
-    "//mojo/public/c/system",
-    "//mojo/public/cpp/system",
+    "//mojo/public/c/system:headers",
   ]
 
   deps = [
@@ -114,8 +121,6 @@ component("system") {
   if (is_android || target_os == "chromeos") {
     defines += [ "MOJO_EDK_LEGACY_PROTOCOL" ]
   }
-
-  allow_circular_includes_from = [ "//mojo/edk/embedder" ]
 }
 
 source_set("test_utils") {