Added PlatformHandle thunks.

mojo/public/platform/native/platform_handle_apptest.cc

+// Copyright 2016 The Chromium Authors. All rights reserved.

[viettrungluu, 2016/02/18 01:10:07]

Maybe call this file platform_handle_private_apptest.cc?

[Forrest Reiling, 2016/02/23 23:48:44]

Wait why? It tests the thunks and the 'api' target at well? What is private
about this test? platform_handle_private.h is the api that apps talk to to use
the thunks. Maybe this is a naming issue with platform_handle_private.h (which
you had me rename from platform_handle_functions.h
https://codereview.chromium.org/1578423002/diff/1/mojo/platform_handle/platfo...
). What exactly does 'private' mean in the context of system thunks?

[viettrungluu, 2016/02/24 21:59:57]

All of the "platform handle" API is private.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "mojo/public/cpp/application/application_impl.h"
+#include "mojo/public/cpp/application/application_test_base.h"
+#include "mojo/public/cpp/environment/logging.h"
+#include "mojo/public/cpp/system/macros.h"
+#include "mojo/public/platform/native/platform_handle_private.h"

[viettrungluu, 2016/02/18 01:10:07]

And include this file first....

[Forrest Reiling, 2016/02/23 23:48:44]

Done.

[Forrest Reiling, 2016/02/24 20:10:42]

So when I do that I dont pass the presubmit checks? is there some special foo I
need here? Im leaving this where it is for now so i dont have to bypass
presubmit hooks

[viettrungluu, 2016/02/24 21:59:57]

That's because you didn't rename this file.

+
+namespace mojo {
+namespace {
+
+// Exemplifies ApplicationTestBase's application testing pattern.
+class PlatformHandleApplicationTest : public test::ApplicationTestBase {
+ public:
+  PlatformHandleApplicationTest() : ApplicationTestBase() {}
+  ~PlatformHandleApplicationTest() override {}
+
+ private:
+  MOJO_DISALLOW_COPY_AND_ASSIGN(PlatformHandleApplicationTest);
+};
+
+TEST_F(PlatformHandleApplicationTest, WrapAndUnwrapFileDescriptor) {
+  MojoPlatformHandle original_handle;
+  MojoPlatformHandle unwrapped_handle;
+  MojoHandle wrapper;
+  MojoResult mojo_result;

[viettrungluu, 2016/02/18 01:10:07]

I think you mostly don't need this.

[Forrest Reiling, 2016/02/23 23:48:44]

Done.

+
+  int posix_result;

[viettrungluu, 2016/02/18 01:10:07]

"

[Forrest Reiling, 2016/02/23 23:48:44]

Done.

+  int pipe_fds[2];

[viettrungluu, 2016/02/18 01:10:06]

Probably initialize these (probably both to -1).

[Forrest Reiling, 2016/02/23 23:48:45]

Done.

+
+  uint64_t write_buffer = 0xDEADBEEF;

[viettrungluu, 2016/02/18 01:10:06]

You should include <stdint.h> for uint64_t.

[Forrest Reiling, 2016/02/23 23:48:44]

Done.

+  uint64_t read_buffer = 0;
+
+  posix_result = pipe(pipe_fds);
+  MOJO_CHECK(posix_result >= 0) << "Failed to open pipe: " << strerror(errno);

[viettrungluu, 2016/02/18 01:10:07]

I'd use ASSERT_GE(), probably as simply

ASSERT_GE(pipe(pipe_fds), 0);

or maybe

ASSERT_GE(pipe(pipe_fds), 0) << errno.

Also, you need to include <string.h> for strerror().

And <errno.h> for errno.

And I guess <unistd.h> for pipe() above.

[Forrest Reiling, 2016/02/23 23:48:44]

Done.

+
+  /*passing second FD through wrapper*/

[viettrungluu, 2016/02/18 01:10:06]

Please use standard C++ comments. Don't forget to punctuate.

[Forrest Reiling, 2016/02/23 23:48:44]

Done.

+  original_handle = pipe_fds[1];
+
+  mojo_result = MojoCreatePlatformHandleWrapper(original_handle, &wrapper);
+  MOJO_CHECK(mojo_result == MOJO_RESULT_OK) << "Failed to wrap platform handle";

[viettrungluu, 2016/02/18 01:10:07]

I'd just do:

EXPECT_EQ(MOJO_RESULT_OK, MojoCreatePlatformHandleWrapper(...));

The text ("Failed to wrap platform handle") is mostly redundant.

[Forrest Reiling, 2016/02/23 23:48:44]

Done.

+
+  mojo_result = MojoExtractPlatformHandle(wrapper, &unwrapped_handle);

[viettrungluu, 2016/02/18 01:10:07]

"

[Forrest Reiling, 2016/02/23 23:48:44]

Done.

+  MOJO_CHECK(mojo_result == MOJO_RESULT_OK)
+      << "Failed to extract platform handle";
+
+  /*write to wrapped/unwrapped fd*/
+  ssize_t bytes_written =

[viettrungluu, 2016/02/18 01:10:06]

I think ssize_t is defined in <sys/types.h>.

[Forrest Reiling, 2016/02/23 23:48:45]

Done.

+      write(unwrapped_handle, &write_buffer, sizeof(write_buffer));
+  MOJO_CHECK(bytes_written >= 0)
+      << "Failed to write to wrapped/unwrapped handle: " << strerror(errno);
+
+  /*read from other end of pipe*/
+  ssize_t bytes_read = read(pipe_fds[0], &read_buffer, sizeof(read_buffer));
+  MOJO_CHECK(bytes_read >= 0) << "Failed to read to read from original pipe: "
+                              << strerror(errno);
+
+  MOJO_CHECK(bytes_read == bytes_written)
+      << "Read a different number of bytes from pipe than were written to pipe";
+  EXPECT_EQ(write_buffer, read_buffer);
+

[viettrungluu, 2016/02/18 01:10:06]

You didn't MojoClose() |wrapper|, and thus you're leaking.

[Forrest Reiling, 2016/02/23 23:48:45]

Wait so MojoExtractPlatformHandle() calls
mojo::embedder::PassWrappedPlatformHandle() on |wrapper|, which causes |wrapper|
to relinquish ownership of the wrapped FD right? Are you saying I still need to
MojoClose |wrapper| even though it doesnt own the fd anymore? Im going to leave
this out for now because it doesn't seem right, but please let me know if I do
in fact need to close the MojoHandle despite it no longer owning the fd.

+  close(pipe_fds[0]);
+  close(unwrapped_handle);
+}
+
+}  // namespace
+}  // namespace mojo