Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1578423002:
Added PlatformHandle thunks. (Closed)
|
Created:
4 years, 11 months ago by Forrest Reiling Modified:
4 years, 9 months ago CC:
mojo-reviews_chromium.org, gregsimon, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org Base URL:
https://github.com/domokit/mojo.git@master Target Ref:
refs/heads/master Project:
mojo Visibility:
Public. |
DescriptionAdded PlatformHandle thunks.
Added thunks for exchanging PlatformHandles (file descriptors) between mojo services as MojoHandles. Basically a port of https://codereview.chromium.org/1113393002 plus some testing code. This is intended to be used to allow cross-process sharing of GPU buffers
Change-Id: I5c77f0ed0381c0b03ce0280314f7fbef3551d895
BUG=
R=viettrungluu@chromium.org
Committed: https://chromium.googlesource.com/external/mojo/+/74b0cac9bdd351dc67c6fc99a7a8b64e85de2f5d
Patch Set 1 #
Total comments: 24
Patch Set 2 : Made @viettrungluu's suggested changes #Patch Set 3 : made one last change I missed in the first pass #Patch Set 4 : effed up the build changes, didnt catch it cause gn didnt run (I think) #
Total comments: 71
Patch Set 5 : Made @viettrunluu 's second round of suggested changes #Patch Set 6 : Renamed platform_handle_apptests to platform_handle_private_apptests #
Total comments: 28
Patch Set 7 : made vtl's most recent suggestions #
Total comments: 4
Patch Set 8 : made nit changes #Patch Set 9 : rebased #
Messages
Total messages: 20 (3 generated)
freiling@chromium.org changed reviewers: + cstout@chromium.org, vtl@google.com
Deferring to vtl
viettrungluu@chromium.org changed reviewers: + viettrungluu@chromium.org
Pro-tip (for freiling): If you send code reviews to viettrungluu@chromium.org, I'll be 98% more likely to notice it. (I know it's confusing. Sorry.)
https://codereview.chromium.org/1578423002/diff/1/mojo/platform_handle/platfo... File mojo/platform_handle/platform_handle_apptest.cc (right): https://codereview.chromium.org/1578423002/diff/1/mojo/platform_handle/platfo... mojo/platform_handle/platform_handle_apptest.cc:6: #include "mojo/public/cpp/application/application_impl.h" nit: blank line above. https://codereview.chromium.org/1578423002/diff/1/mojo/platform_handle/platfo... File mojo/platform_handle/platform_handle_functions.h (right): https://codereview.chromium.org/1578423002/diff/1/mojo/platform_handle/platfo... mojo/platform_handle/platform_handle_functions.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. This file should also live under //mojo/public/platform/native. Probably just call it "platform_handle_private.h". https://codereview.chromium.org/1578423002/diff/1/mojo/platform_handle/platfo... mojo/platform_handle/platform_handle_functions.h:10: using MojoPlatformHandle = int; // Unix file descriptor This header should be compilable as C, so you should use a typedef. https://codereview.chromium.org/1578423002/diff/1/mojo/platform_handle/platfo... mojo/platform_handle/platform_handle_functions.h:12: extern "C" { And wrap this line in #ifdef __cplusplus (and ditto for the closing brace). https://codereview.chromium.org/1578423002/diff/1/mojo/platform_handle/platfo... mojo/platform_handle/platform_handle_functions.h:15: // true on success. This takes ownership of |platform_handle|, regardless of It so doesn't return "true". https://codereview.chromium.org/1578423002/diff/1/mojo/platform_handle/platfo... mojo/platform_handle/platform_handle_functions.h:20: MojoResult MojoExtractPlatformHandle(MojoHandle wrapper, This could use a comment too. https://codereview.chromium.org/1578423002/diff/1/mojo/platform_handle/platfo... File mojo/platform_handle/platform_handle_private_thunks.cc (right): https://codereview.chromium.org/1578423002/diff/1/mojo/platform_handle/platfo... mojo/platform_handle/platform_handle_private_thunks.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. This file and its .h should live in //mojo/public/platform/native. Also, you may as well make this a .c file. https://codereview.chromium.org/1578423002/diff/1/mojo/platform_handle/platfo... mojo/platform_handle/platform_handle_private_thunks.cc:9: #define THUNK_EXPORT __attribute__((visibility("default"))) And include mojo/public/platform/native/thunk_export.h for this. https://codereview.chromium.org/1578423002/diff/1/mojo/platform_handle/platfo... File mojo/platform_handle/platform_handle_private_thunks.h (right): https://codereview.chromium.org/1578423002/diff/1/mojo/platform_handle/platfo... mojo/platform_handle/platform_handle_private_thunks.h:20: inline MojoPlatformHandlePrivateThunks MojoMakePlatformHandlePrivateThunks() { This inline function should be wrapped in |#ifdef __cplusplus|, like in the various files in //mojo/public/platform/native. https://codereview.chromium.org/1578423002/diff/1/shell/platform_handle_impl.cc File shell/platform_handle_impl.cc (right): https://codereview.chromium.org/1578423002/diff/1/shell/platform_handle_impl.... shell/platform_handle_impl.cc:4: This file could use a file-level comment, especially since there's no obvious .h file for it. https://codereview.chromium.org/1578423002/diff/1/shell/platform_handle_impl.... shell/platform_handle_impl.cc:8: Probably the contents of this file should be in the "shell" namespace. https://codereview.chromium.org/1578423002/diff/1/shell/platform_handle_impl.... shell/platform_handle_impl.cc:11: mojo::platform::PlatformHandle platform_handle_wrapper(platform_handle); I'd also add: using mojo::platform::PlatformHandle; using mojo::platform::ScopedPlatformHandle; to the top of the file (usually, I put them before the namespace, after the includes).
https://codereview.chromium.org/1578423002/diff/1/mojo/platform_handle/platfo... File mojo/platform_handle/platform_handle_apptest.cc (right): https://codereview.chromium.org/1578423002/diff/1/mojo/platform_handle/platfo... mojo/platform_handle/platform_handle_apptest.cc:6: #include "mojo/public/cpp/application/application_impl.h" On 2016/02/02 00:07:36, viettrungluu wrote: > nit: blank line above. after I moved the platform handle stuff, that header is last anyway to pass presubmit checks https://codereview.chromium.org/1578423002/diff/1/mojo/platform_handle/platfo... File mojo/platform_handle/platform_handle_functions.h (right): https://codereview.chromium.org/1578423002/diff/1/mojo/platform_handle/platfo... mojo/platform_handle/platform_handle_functions.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/02/02 00:07:36, viettrungluu wrote: > This file should also live under //mojo/public/platform/native. > > Probably just call it "platform_handle_private.h". Done. https://codereview.chromium.org/1578423002/diff/1/mojo/platform_handle/platfo... mojo/platform_handle/platform_handle_functions.h:10: using MojoPlatformHandle = int; // Unix file descriptor On 2016/02/02 00:07:36, viettrungluu wrote: > This header should be compilable as C, so you should use a typedef. Done. https://codereview.chromium.org/1578423002/diff/1/mojo/platform_handle/platfo... mojo/platform_handle/platform_handle_functions.h:12: extern "C" { On 2016/02/02 00:07:36, viettrungluu wrote: > And wrap this line in #ifdef __cplusplus (and ditto for the closing brace). Done. https://codereview.chromium.org/1578423002/diff/1/mojo/platform_handle/platfo... mojo/platform_handle/platform_handle_functions.h:15: // true on success. This takes ownership of |platform_handle|, regardless of On 2016/02/02 00:07:36, viettrungluu wrote: > It so doesn't return "true". Done. https://codereview.chromium.org/1578423002/diff/1/mojo/platform_handle/platfo... mojo/platform_handle/platform_handle_functions.h:20: MojoResult MojoExtractPlatformHandle(MojoHandle wrapper, On 2016/02/02 00:07:36, viettrungluu wrote: > This could use a comment too. Done. https://codereview.chromium.org/1578423002/diff/1/mojo/platform_handle/platfo... File mojo/platform_handle/platform_handle_private_thunks.cc (right): https://codereview.chromium.org/1578423002/diff/1/mojo/platform_handle/platfo... mojo/platform_handle/platform_handle_private_thunks.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/02/02 00:07:36, viettrungluu wrote: > This file and its .h should live in //mojo/public/platform/native. > > Also, you may as well make this a .c file. Done. https://codereview.chromium.org/1578423002/diff/1/mojo/platform_handle/platfo... mojo/platform_handle/platform_handle_private_thunks.cc:9: #define THUNK_EXPORT __attribute__((visibility("default"))) On 2016/02/02 00:07:36, viettrungluu wrote: > And include mojo/public/platform/native/thunk_export.h for this. Done. https://codereview.chromium.org/1578423002/diff/1/mojo/platform_handle/platfo... File mojo/platform_handle/platform_handle_private_thunks.h (right): https://codereview.chromium.org/1578423002/diff/1/mojo/platform_handle/platfo... mojo/platform_handle/platform_handle_private_thunks.h:20: inline MojoPlatformHandlePrivateThunks MojoMakePlatformHandlePrivateThunks() { On 2016/02/02 00:07:36, viettrungluu wrote: > This inline function should be wrapped in |#ifdef __cplusplus|, like in the > various files in //mojo/public/platform/native. Done. https://codereview.chromium.org/1578423002/diff/1/shell/platform_handle_impl.cc File shell/platform_handle_impl.cc (right): https://codereview.chromium.org/1578423002/diff/1/shell/platform_handle_impl.... shell/platform_handle_impl.cc:4: On 2016/02/02 00:07:36, viettrungluu wrote: > This file could use a file-level comment, especially since there's no obvious .h > file for it. Done. https://codereview.chromium.org/1578423002/diff/1/shell/platform_handle_impl.... shell/platform_handle_impl.cc:8: On 2016/02/02 00:07:36, viettrungluu wrote: > Probably the contents of this file should be in the "shell" namespace. As discussed, doing this is problematic because the header used by the shell to include these functions is the same header as the one used by applications to include the thunks, so these definitions need to be in the same neamespace as the thunks. https://codereview.chromium.org/1578423002/diff/1/shell/platform_handle_impl.... shell/platform_handle_impl.cc:11: mojo::platform::PlatformHandle platform_handle_wrapper(platform_handle); On 2016/02/02 00:07:36, viettrungluu wrote: > I'd also add: > > using mojo::platform::PlatformHandle; > using mojo::platform::ScopedPlatformHandle; > > to the top of the file (usually, I put them before the namespace, after the > includes). Done.
https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... File mojo/public/platform/native/BUILD.gn (right): https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/BUILD.gn:6: import("$mojo_root/mojo/public/mojo_application.gni") Just use a relative path, like the import above? https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/BUILD.gn:139: "$mojo_root/mojo/public/c/system", Use mojo_sdk_deps instead of "$mojo_root". https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/BUILD.gn:142: defines = [ "PLATFORM_HANDLE_IMPLEMENTATION" ] Pretty sure you don't need this. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/BUILD.gn:145: # Only targets that are registering the thunks should depend upon this. "registering" is a confusing term. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/BUILD.gn:146: mojo_sdk_source_set("platform_handle_defs") { Probably call it "api" instead of "defs", like system_impl_private_api above. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/BUILD.gn:151: deps = [ mojo_sdk_deps https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/BUILD.gn:156: mojo_native_application("platform_handle_apptest") { Probably have a "tests" group that depends on this (and depend on that from outside this directory)? https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/BUILD.gn:168: "$mojo_root/mojo/public/cpp/application:standalone", mojo_sdk_deps https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... File mojo/public/platform/native/platform_handle_apptest.cc (right): https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_apptest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. Maybe call this file platform_handle_private_apptest.cc? https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_apptest.cc:9: #include "mojo/public/platform/native/platform_handle_private.h" And include this file first.... https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_apptest.cc:28: MojoResult mojo_result; I think you mostly don't need this. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_apptest.cc:30: int posix_result; " https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_apptest.cc:31: int pipe_fds[2]; Probably initialize these (probably both to -1). https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_apptest.cc:33: uint64_t write_buffer = 0xDEADBEEF; You should include <stdint.h> for uint64_t. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_apptest.cc:37: MOJO_CHECK(posix_result >= 0) << "Failed to open pipe: " << strerror(errno); 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. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_apptest.cc:39: /*passing second FD through wrapper*/ Please use standard C++ comments. Don't forget to punctuate. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_apptest.cc:43: MOJO_CHECK(mojo_result == MOJO_RESULT_OK) << "Failed to wrap platform handle"; I'd just do: EXPECT_EQ(MOJO_RESULT_OK, MojoCreatePlatformHandleWrapper(...)); The text ("Failed to wrap platform handle") is mostly redundant. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_apptest.cc:45: mojo_result = MojoExtractPlatformHandle(wrapper, &unwrapped_handle); " https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_apptest.cc:50: ssize_t bytes_written = I think ssize_t is defined in <sys/types.h>. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_apptest.cc:63: You didn't MojoClose() |wrapper|, and thus you're leaking. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... File mojo/public/platform/native/platform_handle_private.h (right): https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_private.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016? https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_private.h:5: #ifndef MOJO_PLATFORM_HANDLE_PLATFORM_HANDLE_FUNCTIONS_H_ Update this header guard. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_private.h:12: #if defined(__cplusplus) I think we conventionally/mostly use #ifdef when checking __cplusplus. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_private.h:25: // longer close the underlying |platform_handle| Probably mention that |wrapper| nonetheless should still be MojoClose()ed. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_private.h:29: #if defined(__cplusplus) ... https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... File mojo/public/platform/native/platform_handle_private_thunks.c (right): https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_private_thunks.c:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016? https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_private_thunks.c:6: #include "mojo/public/platform/native/thunk_export.h" Add a blank line above. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_private_thunks.c:30: Please delete the blank line at the end of this file. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... File mojo/public/platform/native/platform_handle_private_thunks.h (right): https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_private_thunks.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016? https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_private_thunks.h:20: #if defined(__cplusplus) #ifdef? https://codereview.chromium.org/1578423002/diff/60001/shell/native_applicatio... File shell/native_application_support.cc (right): https://codereview.chromium.org/1578423002/diff/60001/shell/native_applicatio... shell/native_application_support.cc:78: SetThunks(&MojoMakePlatformHandlePrivateThunks, This needs a TODO along the lines of the TODO(ncbray) below.
https://codereview.chromium.org/1578423002/diff/60001/shell/platform_handle_i... File shell/platform_handle_impl.cc (right): https://codereview.chromium.org/1578423002/diff/60001/shell/platform_handle_i... shell/platform_handle_impl.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016
https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... File mojo/public/platform/native/BUILD.gn (right): https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/BUILD.gn:6: import("$mojo_root/mojo/public/mojo_application.gni") On 2016/02/18 01:10:06, viettrungluu wrote: > Just use a relative path, like the import above? Done. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/BUILD.gn:139: "$mojo_root/mojo/public/c/system", On 2016/02/18 01:10:06, viettrungluu wrote: > Use mojo_sdk_deps instead of "$mojo_root". Done. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/BUILD.gn:142: defines = [ "PLATFORM_HANDLE_IMPLEMENTATION" ] On 2016/02/18 01:10:06, viettrungluu wrote: > Pretty sure you don't need this. Done. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/BUILD.gn:145: # Only targets that are registering the thunks should depend upon this. On 2016/02/18 01:10:06, viettrungluu wrote: > "registering" is a confusing term. going with 'calling the thunks' instead https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/BUILD.gn:146: mojo_sdk_source_set("platform_handle_defs") { On 2016/02/18 01:10:06, viettrungluu wrote: > Probably call it "api" instead of "defs", like system_impl_private_api above. Done. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/BUILD.gn:151: deps = [ On 2016/02/18 01:10:06, viettrungluu wrote: > mojo_sdk_deps Done. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/BUILD.gn:156: mojo_native_application("platform_handle_apptest") { On 2016/02/18 01:10:06, viettrungluu wrote: > Probably have a "tests" group that depends on this (and depend on that from > outside this directory)? If I do that do you want me to put the other test targets in this build file in there? Im not really sure what the dependency chain for the rest of the tests in here look like, or through what dependency chain this test is supposed to be built. Right now it is depended on by the //mojo:tests target, but I'm not super clear on if thats right. Is there documentation on the right way to add tests to mojo? https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/BUILD.gn:168: "$mojo_root/mojo/public/cpp/application:standalone", On 2016/02/18 01:10:06, viettrungluu wrote: > mojo_sdk_deps when I do this here it says the variable goes out of scope before it is used. Are you sure this is appropriate for 'mojo_native_application' target? is there a different target AI should be using for the app tests? should the apptests not live here? Again I'm kindof fuzzy on the best way to add mojo tests https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... File mojo/public/platform/native/platform_handle_apptest.cc (right): https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_apptest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/02/18 01:10:07, viettrungluu wrote: > Maybe call this file platform_handle_private_apptest.cc? 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? https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_apptest.cc:9: #include "mojo/public/platform/native/platform_handle_private.h" On 2016/02/18 01:10:07, viettrungluu wrote: > And include this file first.... Done. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_apptest.cc:28: MojoResult mojo_result; On 2016/02/18 01:10:07, viettrungluu wrote: > I think you mostly don't need this. Done. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_apptest.cc:30: int posix_result; On 2016/02/18 01:10:07, viettrungluu wrote: > " Done. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_apptest.cc:31: int pipe_fds[2]; On 2016/02/18 01:10:06, viettrungluu wrote: > Probably initialize these (probably both to -1). Done. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_apptest.cc:33: uint64_t write_buffer = 0xDEADBEEF; On 2016/02/18 01:10:06, viettrungluu wrote: > You should include <stdint.h> for uint64_t. Done. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_apptest.cc:37: MOJO_CHECK(posix_result >= 0) << "Failed to open pipe: " << strerror(errno); On 2016/02/18 01:10:07, viettrungluu wrote: > 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. Done. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_apptest.cc:39: /*passing second FD through wrapper*/ On 2016/02/18 01:10:06, viettrungluu wrote: > Please use standard C++ comments. Don't forget to punctuate. Done. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_apptest.cc:43: MOJO_CHECK(mojo_result == MOJO_RESULT_OK) << "Failed to wrap platform handle"; On 2016/02/18 01:10:07, viettrungluu wrote: > I'd just do: > > EXPECT_EQ(MOJO_RESULT_OK, MojoCreatePlatformHandleWrapper(...)); > > The text ("Failed to wrap platform handle") is mostly redundant. Done. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_apptest.cc:45: mojo_result = MojoExtractPlatformHandle(wrapper, &unwrapped_handle); On 2016/02/18 01:10:07, viettrungluu wrote: > " Done. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_apptest.cc:50: ssize_t bytes_written = On 2016/02/18 01:10:06, viettrungluu wrote: > I think ssize_t is defined in <sys/types.h>. Done. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_apptest.cc:63: On 2016/02/18 01:10:06, viettrungluu wrote: > You didn't MojoClose() |wrapper|, and thus you're leaking. 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. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... File mojo/public/platform/native/platform_handle_private.h (right): https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_private.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/02/18 01:10:07, viettrungluu wrote: > 2016? Done. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_private.h:5: #ifndef MOJO_PLATFORM_HANDLE_PLATFORM_HANDLE_FUNCTIONS_H_ On 2016/02/18 01:10:07, viettrungluu wrote: > Update this header guard. Done. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_private.h:12: #if defined(__cplusplus) On 2016/02/18 01:10:07, viettrungluu wrote: > I think we conventionally/mostly use #ifdef when checking __cplusplus. Wait what? So we use '#if defined()' everywhere except for checking if __cplusplus is defined? Doesn't that strike you as strange/terrible? Not that the choice of directive/macro make a bit of difference to me, but the usage rules seem pretty fuzzy and unclear. are the rules spelled out somewhere? https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_private.h:25: // longer close the underlying |platform_handle| On 2016/02/18 01:10:07, viettrungluu wrote: > Probably mention that |wrapper| nonetheless should still be MojoClose()ed. Along the lines of my other comment on the matter, is this actually true? If this succeeds, then the handle no longer owns the wrapped fd, so that fd should be closed on its own and shouldnt need to be closed though the mojo handle. Does MojoClose() have other side effects that I'm not aware of? Does it actually do something useful when called on a MojoHandle that doesnt own an fd? https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_private.h:29: #if defined(__cplusplus) On 2016/02/18 01:10:07, viettrungluu wrote: > ... Done. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... File mojo/public/platform/native/platform_handle_private_thunks.c (right): https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_private_thunks.c:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/02/18 01:10:07, viettrungluu wrote: > 2016? Done. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_private_thunks.c:6: #include "mojo/public/platform/native/thunk_export.h" On 2016/02/18 01:10:07, viettrungluu wrote: > Add a blank line above. Done. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_private_thunks.c:30: On 2016/02/18 01:10:07, viettrungluu wrote: > Please delete the blank line at the end of this file. Done. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... File mojo/public/platform/native/platform_handle_private_thunks.h (right): https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_private_thunks.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/02/18 01:10:07, viettrungluu wrote: > 2016? Done. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_private_thunks.h:20: #if defined(__cplusplus) On 2016/02/18 01:10:07, viettrungluu wrote: > #ifdef? Done. With dissent because I think this policy is terrible, but whatever lol https://codereview.chromium.org/1578423002/diff/60001/shell/native_applicatio... File shell/native_application_support.cc (right): https://codereview.chromium.org/1578423002/diff/60001/shell/native_applicatio... shell/native_application_support.cc:78: SetThunks(&MojoMakePlatformHandlePrivateThunks, On 2016/02/18 01:10:07, viettrungluu wrote: > This needs a TODO along the lines of the TODO(ncbray) below. Sure. Not gonna lie, I dont really understand what the implications of that TODO are, but presumably after @ncbray figures it out for the SystemImplControlThunks I can just copy their approach. https://codereview.chromium.org/1578423002/diff/60001/shell/platform_handle_i... File shell/platform_handle_impl.cc (right): https://codereview.chromium.org/1578423002/diff/60001/shell/platform_handle_i... shell/platform_handle_impl.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/02/18 01:11:03, viettrungluu wrote: > 2016 Done.
https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... File mojo/public/platform/native/BUILD.gn (right): https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/BUILD.gn:156: mojo_native_application("platform_handle_apptest") { On 2016/02/23 23:48:44, Forrest Reiling wrote: > On 2016/02/18 01:10:06, viettrungluu wrote: > > Probably have a "tests" group that depends on this (and depend on that from > > outside this directory)? > > If I do that do you want me to put the other test targets in this build file in > there? Probably, if you can. > Im not really sure what the dependency chain for the rest of the tests in > here look like, or through what dependency chain this test is supposed to be > built. Right now it is depended on by the //mojo:tests target, but I'm not super > clear on if thats right. That seems fine. It'd be good to avoid having to add individual tests to //mojo/BUILD.gn. > Is there documentation on the right way to add tests to > mojo? Ha. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/BUILD.gn:168: "$mojo_root/mojo/public/cpp/application:standalone", On 2016/02/23 23:48:44, Forrest Reiling wrote: > On 2016/02/18 01:10:06, viettrungluu wrote: > > mojo_sdk_deps > > when I do this here it says the variable goes out of scope before it is used. > Are you sure this is appropriate for 'mojo_native_application' target? is there > a different target AI should be using for the app tests? should the apptests not > live here? Again I'm kindof fuzzy on the best way to add mojo tests Blech. Probably mojo_native_application (in //mojo/public/mojo_application.gni should be made to handle mojo_sdk_deps (see //mojo/public/mojo_sdk.gni). Or, if you're lazy, you can use a relative path instead, I guess.
Second round of changes are up, PTAL https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... File mojo/public/platform/native/BUILD.gn (right): https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/BUILD.gn:156: mojo_native_application("platform_handle_apptest") { On 2016/02/24 18:09:57, viettrungluu wrote: > On 2016/02/23 23:48:44, Forrest Reiling wrote: > > On 2016/02/18 01:10:06, viettrungluu wrote: > > > Probably have a "tests" group that depends on this (and depend on that from > > > outside this directory)? > > > > If I do that do you want me to put the other test targets in this build file > in > > there? > > Probably, if you can. > > > Im not really sure what the dependency chain for the rest of the tests in > > here look like, or through what dependency chain this test is supposed to be > > built. Right now it is depended on by the //mojo:tests target, but I'm not > super > > clear on if thats right. > > That seems fine. It'd be good to avoid having to add individual tests to > //mojo/BUILD.gn. > > > Is there documentation on the right way to add tests to > > mojo? > > Ha. Done. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/BUILD.gn:168: "$mojo_root/mojo/public/cpp/application:standalone", On 2016/02/24 18:09:57, viettrungluu wrote: > On 2016/02/23 23:48:44, Forrest Reiling wrote: > > On 2016/02/18 01:10:06, viettrungluu wrote: > > > mojo_sdk_deps > > > > when I do this here it says the variable goes out of scope before it is used. > > Are you sure this is appropriate for 'mojo_native_application' target? is > there > > a different target AI should be using for the app tests? should the apptests > not > > live here? Again I'm kindof fuzzy on the best way to add mojo tests > > Blech. Probably mojo_native_application (in //mojo/public/mojo_application.gni > should be made to handle mojo_sdk_deps (see //mojo/public/mojo_sdk.gni). > > Or, if you're lazy, you can use a relative path instead, I guess. using relative path https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... File mojo/public/platform/native/platform_handle_apptest.cc (right): https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_apptest.cc:9: #include "mojo/public/platform/native/platform_handle_private.h" On 2016/02/18 01:10:07, viettrungluu wrote: > And include this file first.... 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
https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... File mojo/public/platform/native/platform_handle_apptest.cc (right): https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_apptest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/02/23 23:48:44, Forrest Reiling wrote: > On 2016/02/18 01:10:07, viettrungluu wrote: > > Maybe call this file platform_handle_private_apptest.cc? > > 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? All of the "platform handle" API is private. https://codereview.chromium.org/1578423002/diff/60001/mojo/public/platform/na... mojo/public/platform/native/platform_handle_apptest.cc:9: #include "mojo/public/platform/native/platform_handle_private.h" On 2016/02/24 20:10:42, Forrest Reiling wrote: > On 2016/02/18 01:10:07, viettrungluu wrote: > > And include this file first.... > > 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 That's because you didn't rename this file.
Renamed platform_handle_apptests as per request by @viettrungluu. PTAL
https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... File mojo/public/platform/native/platform_handle_private.h (right): https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... mojo/public/platform/native/platform_handle_private.h:5: #ifndef MOJO_PLATFORM_HANDLE_PLATFORM_HANDLE_PRIVATE_H_ This include guard should be updated. https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... mojo/public/platform/native/platform_handle_private.h:35: #endif // MOJO_PLATFORM_HANDLE_PLATFORM_HANDLE_PRIVATE_H_ Don't forget to update this comment. https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... File mojo/public/platform/native/platform_handle_private_apptest.cc (right): https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... mojo/public/platform/native/platform_handle_private_apptest.cc:21: // Exemplifies ApplicationTestBase's application testing pattern. This comment seems kind of pointless. https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... mojo/public/platform/native/platform_handle_private_apptest.cc:32: MojoPlatformHandle original_handle; (Probably to -1.) https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... mojo/public/platform/native/platform_handle_private_apptest.cc:34: MojoHandle wrapper; You should initialize these (probably to MOJO_HANDLE_INVALID). https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... mojo/public/platform/native/platform_handle_private_apptest.cc:41: ASSERT_GE(pipe(pipe_fds), 0) << strerror(errno); ASSERT_EQ (the specification says that pipe() returns 0 on success) I'd also skip the strerror() (here, and everywhere else). (Then, you can stop including <string.h>. Also <errno.h>, which you didn't bother including in the first place.) https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... mojo/public/platform/native/platform_handle_private_apptest.cc:54: MOJO_CHECK(bytes_written >= 0) EXPECT_GE Really, you want EXPECT_EQ(sizeof(write_buffer), bytes_written); (or maybe ASSERT_EQ), since you rely on this being true below. https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... mojo/public/platform/native/platform_handle_private_apptest.cc:55: << "Failed to write to wrapped/unwrapped handle: " << strerror(errno); These messages are mostly overkill; I'd skip them. https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... mojo/public/platform/native/platform_handle_private_apptest.cc:59: MOJO_CHECK(bytes_read >= 0) << "Failed to read to read from original pipe: " This seems kind of redundant. https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... mojo/public/platform/native/platform_handle_private_apptest.cc:62: MOJO_CHECK(bytes_read == bytes_written) EXPECT_EQ() (and this will cover the above check). The additional output seems mostly like overkill (any error is pretty much entirely self-explanatory). https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... mojo/public/platform/native/platform_handle_private_apptest.cc:66: close(pipe_fds[0]); ASSERT_EQ(0, close(...)); (or maybe EXPECT_EQ) https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... mojo/public/platform/native/platform_handle_private_apptest.cc:68: MojoClose(wrapper); ASSERT_EQ(MOJO_RESULT_OK, MojoClose(wrapper)); https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... File mojo/public/platform/native/platform_handle_private_thunks.h (right): https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... mojo/public/platform/native/platform_handle_private_thunks.h:5: #ifndef MOJO_PLATFORM_HANDLE_PLATFORM_HANDLE_PRIVATE_THUNKS_H_ This include guard should be updated. https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... mojo/public/platform/native/platform_handle_private_thunks.h:33: #endif // MOJO_PLATFORM_HANDLE_PLATFORM_HANDLE_PRIVATE_THUNKS_H_ (And also this comment.)
PTAL https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... File mojo/public/platform/native/platform_handle_private.h (right): https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... mojo/public/platform/native/platform_handle_private.h:5: #ifndef MOJO_PLATFORM_HANDLE_PLATFORM_HANDLE_PRIVATE_H_ On 2016/02/26 00:14:21, viettrungluu wrote: > This include guard should be updated. Done. https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... mojo/public/platform/native/platform_handle_private.h:35: #endif // MOJO_PLATFORM_HANDLE_PLATFORM_HANDLE_PRIVATE_H_ On 2016/02/26 00:14:21, viettrungluu wrote: > Don't forget to update this comment. Done. https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... File mojo/public/platform/native/platform_handle_private_apptest.cc (right): https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... mojo/public/platform/native/platform_handle_private_apptest.cc:21: // Exemplifies ApplicationTestBase's application testing pattern. On 2016/02/26 00:14:21, viettrungluu wrote: > This comment seems kind of pointless. Looks like copy-pasta from the example I used, I'll scrub it https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... mojo/public/platform/native/platform_handle_private_apptest.cc:32: MojoPlatformHandle original_handle; On 2016/02/26 00:14:21, viettrungluu wrote: > (Probably to -1.) Done. https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... mojo/public/platform/native/platform_handle_private_apptest.cc:34: MojoHandle wrapper; On 2016/02/26 00:14:21, viettrungluu wrote: > You should initialize these (probably to MOJO_HANDLE_INVALID). Done. https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... mojo/public/platform/native/platform_handle_private_apptest.cc:41: ASSERT_GE(pipe(pipe_fds), 0) << strerror(errno); On 2016/02/26 00:14:21, viettrungluu wrote: > ASSERT_EQ > > (the specification says that pipe() returns 0 on success) > > I'd also skip the strerror() (here, and everywhere else). > > (Then, you can stop including <string.h>. Also <errno.h>, which you didn't > bother including in the first place.) Done. https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... mojo/public/platform/native/platform_handle_private_apptest.cc:54: MOJO_CHECK(bytes_written >= 0) On 2016/02/26 00:14:21, viettrungluu wrote: > EXPECT_GE > > Really, you want > > EXPECT_EQ(sizeof(write_buffer), bytes_written); > > (or maybe ASSERT_EQ), since you rely on this being true below. Done. https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... mojo/public/platform/native/platform_handle_private_apptest.cc:55: << "Failed to write to wrapped/unwrapped handle: " << strerror(errno); On 2016/02/26 00:14:21, viettrungluu wrote: > These messages are mostly overkill; I'd skip them. Done. https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... mojo/public/platform/native/platform_handle_private_apptest.cc:59: MOJO_CHECK(bytes_read >= 0) << "Failed to read to read from original pipe: " On 2016/02/26 00:14:21, viettrungluu wrote: > This seems kind of redundant. replaced with symmetric check to the one above (EXPECT_EQ(sizeof(read_buffer), bytes_read);) https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... mojo/public/platform/native/platform_handle_private_apptest.cc:62: MOJO_CHECK(bytes_read == bytes_written) On 2016/02/26 00:14:21, viettrungluu wrote: > EXPECT_EQ() (and this will cover the above check). The additional output seems > mostly like overkill (any error is pretty much entirely self-explanatory). Done. https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... mojo/public/platform/native/platform_handle_private_apptest.cc:66: close(pipe_fds[0]); On 2016/02/26 00:14:22, viettrungluu wrote: > ASSERT_EQ(0, close(...)); > > (or maybe EXPECT_EQ) Done. https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... mojo/public/platform/native/platform_handle_private_apptest.cc:68: MojoClose(wrapper); On 2016/02/26 00:14:21, viettrungluu wrote: > ASSERT_EQ(MOJO_RESULT_OK, MojoClose(wrapper)); Done. https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... File mojo/public/platform/native/platform_handle_private_thunks.h (right): https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... mojo/public/platform/native/platform_handle_private_thunks.h:5: #ifndef MOJO_PLATFORM_HANDLE_PLATFORM_HANDLE_PRIVATE_THUNKS_H_ On 2016/02/26 00:14:22, viettrungluu wrote: > This include guard should be updated. Done. https://codereview.chromium.org/1578423002/diff/100001/mojo/public/platform/n... mojo/public/platform/native/platform_handle_private_thunks.h:33: #endif // MOJO_PLATFORM_HANDLE_PLATFORM_HANDLE_PRIVATE_THUNKS_H_ On 2016/02/26 00:14:22, viettrungluu wrote: > (And also this comment.) Done.
lgtm w/nits https://codereview.chromium.org/1578423002/diff/120001/mojo/public/platform/n... File mojo/public/platform/native/platform_handle_private_apptest.cc (right): https://codereview.chromium.org/1578423002/diff/120001/mojo/public/platform/n... mojo/public/platform/native/platform_handle_private_apptest.cc:8: #include <string.h> nit: I don't think you need this include anymore. https://codereview.chromium.org/1578423002/diff/120001/mojo/public/platform/n... mojo/public/platform/native/platform_handle_private_apptest.cc:39: ASSERT_EQ(pipe(pipe_fds), 0); nit: ASSERT_EQ and EXPECT_EQ are both "backwards": It should be ASSERT_EQ(expected_value, real_value);, so this should be ASSERT_EQ(0, pipe(pipe_fds));.
PTAL https://codereview.chromium.org/1578423002/diff/120001/mojo/public/platform/n... File mojo/public/platform/native/platform_handle_private_apptest.cc (right): https://codereview.chromium.org/1578423002/diff/120001/mojo/public/platform/n... mojo/public/platform/native/platform_handle_private_apptest.cc:8: #include <string.h> On 2016/03/02 23:42:55, viettrungluu wrote: > nit: I don't think you need this include anymore. Done. https://codereview.chromium.org/1578423002/diff/120001/mojo/public/platform/n... mojo/public/platform/native/platform_handle_private_apptest.cc:39: ASSERT_EQ(pipe(pipe_fds), 0); On 2016/03/02 23:42:55, viettrungluu wrote: > nit: ASSERT_EQ and EXPECT_EQ are both "backwards": It should be > ASSERT_EQ(expected_value, real_value);, so this should be ASSERT_EQ(0, > pipe(pipe_fds));. ewwww
Description was changed from ========== Added PlatformHandle thunks. Added thunks for exchanging PlatformHandles (file descriptors) between mojo services as MojoHandles. Basically a port of https://codereview.chromium.org/1113393002 plus some testing code. This is intended to be used to allow cross-process sharing of GPU buffers Change-Id: I5c77f0ed0381c0b03ce0280314f7fbef3551d895 BUG= ========== to ========== Added PlatformHandle thunks. Added thunks for exchanging PlatformHandles (file descriptors) between mojo services as MojoHandles. Basically a port of https://codereview.chromium.org/1113393002 plus some testing code. This is intended to be used to allow cross-process sharing of GPU buffers Change-Id: I5c77f0ed0381c0b03ce0280314f7fbef3551d895 BUG= R=viettrungluu@chromium.org Committed: https://chromium.googlesource.com/external/mojo/+/74b0cac9bdd351dc67c6fc99a7a... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) manually as 74b0cac9bdd351dc67c6fc99a7a8b64e85de2f5d (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
