Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1424503002:
arc-bridge: Add IPC message definitions (Closed)
|
Created:
5 years, 2 months ago by Luis Héctor Chávez Modified:
5 years, 1 month ago Reviewers:
elijahtaylor1, lhc(google), dcheng, jln (very slow on Chromium), hidehiko, Shuhei Takahashi, satorux1 CC:
chromium-reviews, darin-cc_chromium.org, jam, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/a/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc-bridge: Add IPC message definitions
This code will be shared between Chrome and ARC, and will
allow them to communicate with each other.
BUG=b:24339743
Committed: https://crrev.com/67d14d4249ec9be027df91655d301872e75d493a
Cr-Commit-Position: refs/heads/master@{#357996}
Patch Set 1 #
Total comments: 20
Patch Set 2 : Renamed libarcbridge=>bridge/common and addressed feedback #
Total comments: 36
Patch Set 3 : Addressed feedback #Patch Set 4 : Added OWNERS and moved the .gyp changes here #Patch Set 5 : Left only IPC message definitions #
Total comments: 2
Patch Set 6 : Updated a comment #
Total comments: 8
Patch Set 7 : Removed unused imports #Patch Set 8 : Fixed the build by adding the ipc dependency #Patch Set 9 : Added IPC-specific OWNERS file #
Total comments: 6
Dependent Patchsets: Messages
Total messages: 42 (11 generated)
lhchavez@chromium.org changed reviewers: + jln@chromium.org, satorux@chromium.org
PTAL This is required by https://codereview.chromium.org/1412863004/
satorux@chromium.org changed reviewers: + hidehiko@chromium.org
Took a quick look, and added nit-picky comments per Chrome's coding practices. +hidehiko who seems to have some experience with Chrome's IPC code. https://codereview.chromium.org/1424503002/diff/1/chromeos/arc/libarcbridge/a... File chromeos/arc/libarcbridge/arc_bridge.cc (right): https://codereview.chromium.org/1424503002/diff/1/chromeos/arc/libarcbridge/a... chromeos/arc/libarcbridge/arc_bridge.cc:48: bool BridgeInstanceEndpoint::OnMessageReceived(const IPC::Message& message) { should this function run on the io_thread_? Then you can add an assertion here. DCHECK(io_thread_.task_runner()->RunsTasksOnCurrentThread()); would probably work. https://codereview.chromium.org/1424503002/diff/1/chromeos/arc/libarcbridge/a... chromeos/arc/libarcbridge/arc_bridge.cc:70: base::FilePath path(socket_path); Please use base::FilePath::FromUTF8Unsafe() for converting a string into a path here. FilePath can be 16-bit characters on Windows, which does not matter here, but I think it's still good to follow Chrome's common practice. https://codereview.chromium.org/1424503002/diff/1/chromeos/arc/libarcbridge/a... chromeos/arc/libarcbridge/arc_bridge.cc:79: if (chmod(socket_path, 0777) != 0) HANDLE_EINTR? https://codereview.chromium.org/1424503002/diff/1/chromeos/arc/libarcbridge/a... File chromeos/arc/libarcbridge/arc_bridge.h (right): https://codereview.chromium.org/1424503002/diff/1/chromeos/arc/libarcbridge/a... chromeos/arc/libarcbridge/arc_bridge.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. Can we drop 'lib' from the directory name? Directory names starting with 'lib' are unusual in Chrome (ex. we have 'base' instead of 'libbase'). Your next patch will introduce 'chromeos/arc/bridge'. Can we put the stuff in this patch in that directory? If you want to separate two sets of stuff, maybe create sub directories chromeos/arc/bridge' but that may be overkill. https://codereview.chromium.org/1424503002/diff/1/chromeos/arc/libarcbridge/a... chromeos/arc/libarcbridge/arc_bridge.h:3: // found in the LICENSE file. Because the concept of ARC Bridge is not obvious, could you write some description about it here? https://codereview.chromium.org/1424503002/diff/1/chromeos/arc/libarcbridge/a... chromeos/arc/libarcbridge/arc_bridge.h:21: bool Connect(const IPC::ChannelHandle& handle, IPC::Channel::Mode mode); function comment is missing. Please fix other places http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm... https://codereview.chromium.org/1424503002/diff/1/chromeos/arc/libarcbridge/a... chromeos/arc/libarcbridge/arc_bridge.h:27: base::Thread io_thread_; "IO thread" in Chrome usually means BrowserThread::IO. How about naming it like ipc_thread_? https://codereview.chromium.org/1424503002/diff/1/chromeos/arc/libarcbridge/a... chromeos/arc/libarcbridge/arc_bridge.h:38: bool SocketConnect(const char* socket_path); How about const std::string&? I guess NULL is not valid here.
https://codereview.chromium.org/1424503002/diff/1/chromeos/arc/libarcbridge/a... File chromeos/arc/libarcbridge/arc_bridge.cc (right): https://codereview.chromium.org/1424503002/diff/1/chromeos/arc/libarcbridge/a... chromeos/arc/libarcbridge/arc_bridge.cc:48: bool BridgeInstanceEndpoint::OnMessageReceived(const IPC::Message& message) { On 2015/10/23 05:16:43, satorux1 wrote: > should this function run on the io_thread_? Then you can add an assertion here. > > DCHECK(io_thread_.task_runner()->RunsTasksOnCurrentThread()); > > would probably work. No, this is supposed to be processed in the original MessageLoop/Thread. None of the other implementations of IPC have that assertion FWIW. https://codereview.chromium.org/1424503002/diff/1/chromeos/arc/libarcbridge/a... chromeos/arc/libarcbridge/arc_bridge.cc:70: base::FilePath path(socket_path); On 2015/10/23 05:16:43, satorux1 wrote: > Please use base::FilePath::FromUTF8Unsafe() for converting a string into a path > here. FilePath can be 16-bit characters on Windows, which does not matter here, > but I think it's still good to follow Chrome's common practice. Done. https://codereview.chromium.org/1424503002/diff/1/chromeos/arc/libarcbridge/a... chromeos/arc/libarcbridge/arc_bridge.cc:79: if (chmod(socket_path, 0777) != 0) On 2015/10/23 05:16:43, satorux1 wrote: > HANDLE_EINTR? Done. https://codereview.chromium.org/1424503002/diff/1/chromeos/arc/libarcbridge/a... File chromeos/arc/libarcbridge/arc_bridge.h (right): https://codereview.chromium.org/1424503002/diff/1/chromeos/arc/libarcbridge/a... chromeos/arc/libarcbridge/arc_bridge.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/10/23 05:16:43, satorux1 wrote: > Can we drop 'lib' from the directory name? Directory names starting with 'lib' > are unusual in Chrome (ex. we have 'base' instead of 'libbase'). > > Your next patch will introduce 'chromeos/arc/bridge'. Can we put the stuff in > this patch in that directory? If you want to separate two sets of stuff, maybe > create sub directories chromeos/arc/bridge' but that may be overkill. The reason for the separation is that I need to have this be partially isolated since I will import it in the ARC instance and compile it there. I was able to make it a subdirectory. https://codereview.chromium.org/1424503002/diff/1/chromeos/arc/libarcbridge/a... chromeos/arc/libarcbridge/arc_bridge.h:3: // found in the LICENSE file. On 2015/10/23 05:16:43, satorux1 wrote: > Because the concept of ARC Bridge is not obvious, could you write some > description about it here? Done. https://codereview.chromium.org/1424503002/diff/1/chromeos/arc/libarcbridge/a... chromeos/arc/libarcbridge/arc_bridge.h:21: bool Connect(const IPC::ChannelHandle& handle, IPC::Channel::Mode mode); On 2015/10/23 05:16:43, satorux1 wrote: > function comment is missing. Please fix other places > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm... Done. https://codereview.chromium.org/1424503002/diff/1/chromeos/arc/libarcbridge/a... chromeos/arc/libarcbridge/arc_bridge.h:27: base::Thread io_thread_; On 2015/10/23 05:16:43, satorux1 wrote: > "IO thread" in Chrome usually means BrowserThread::IO. How about naming it like > ipc_thread_? Done. https://codereview.chromium.org/1424503002/diff/1/chromeos/arc/libarcbridge/a... chromeos/arc/libarcbridge/arc_bridge.h:38: bool SocketConnect(const char* socket_path); On 2015/10/23 05:16:43, satorux1 wrote: > How about const std::string&? I guess NULL is not valid here. Done.
Thank you for moving stuff into a sub directory. Please update the patch description accordingly. https://codereview.chromium.org/1424503002/diff/1/chromeos/arc/libarcbridge/a... File chromeos/arc/libarcbridge/arc_bridge.cc (right): https://codereview.chromium.org/1424503002/diff/1/chromeos/arc/libarcbridge/a... chromeos/arc/libarcbridge/arc_bridge.cc:48: bool BridgeInstanceEndpoint::OnMessageReceived(const IPC::Message& message) { On 2015/10/23 17:09:24, Luis Héctor Chávez wrote: > On 2015/10/23 05:16:43, satorux1 wrote: > > should this function run on the io_thread_? Then you can add an assertion > here. > > > > DCHECK(io_thread_.task_runner()->RunsTasksOnCurrentThread()); > > > > would probably work. > > No, this is supposed to be processed in the original MessageLoop/Thread. None of > the other implementations of IPC have that assertion FWIW. I see. In that case, how about adding ThreadChecker instance as a member variable and use it here (and elsewhere), to ensure that this function is called in the original thread? https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... File chromeos/arc/bridge/common/arc_bridge.h (right): https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_bridge.h:39: // Instance side of the ARC bridge. I felt that relationship with 'instance side' and 'host side' was not clear. Could you document it here? https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_bridge.h:51: bool OnMessageReceived(const IPC::Message& message) override; please add function comments to these functions. it's a bit tedious but Chrome follows this style rather thoroughly. :) http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm... Every function declaration should have comments immediately preceding it that describe what the function does and how to use it. https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_bridge.h:52: void OnPing(); What's ping? Is this a sample function or does it have a real use case? Please document. https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_bridge.h:53: virtual void OnRegisterInputDevice(const std::string& name, In particular, please document how sub classes are supposed to implement this. https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_bridge.h:66: bool SocketConnect(const std::string& socket_path); ditto. function comment is missing. https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... File chromeos/arc/bridge/common/arc_instance_messages.h (right): https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_instance_messages.h:22: // file descriptor provided. how about adding examples of 'name' and 'device_type' values to illustrate how these will look like.
satorux@chromium.org changed reviewers: + nya@chromium.org
BTW I gave some pretty general feedback so far, but I think it's better for this code to be reviewed by someone who's gonna actually use it. Adding nya@ in this regard. :)
Generic question. No build rule yet? BTW, how about removing abstract messages (like ArcInstanceMsg_RegisterInputDevice) from this CL, and introduce them back in a separate CL with concrete implementation. The separation of "introducing a channel-pair established", and "implementing each message" would more readable, IMHO. WDYT? https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... File chromeos/arc/bridge/common/arc_bridge.cc (right): https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_bridge.cc:5: #include "arc_bridge.h" nit: IIUC, probably we should use #include "chromeos/arc/bridge/common/arc_bridge.h" https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_bridge.cc:9: #include <base/bind_helpers.h> nit: #include "base/bind_helpers.h" Ditto for below. https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_bridge.cc:13: #include "arc_host_messages.h" ditto. And sort in lexicographical order (with mixing base/... ipc/...). https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_bridge.cc:71: if (!base::CreateDirectory(path.DirName())) Clarification: Which thread will this be invoked on? File operation may not be allowed. (Note: on the IO thread, it is not allowed conceptually (despite of its name...)). https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... File chromeos/arc/bridge/common/arc_bridge.h (right): https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_bridge.h:20: class BridgeEndpoint : public IPC::Listener { What's the usage of BridgeEndpoint? If this is just a utility to implement Bridge{Host,Instance}Endpoint classes, it's better not to inherit, instead of composition. Or, in this case, I think you can define some utility functions in .cc file, I guess.
https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... File chromeos/arc/bridge/common/arc_bridge.cc (right): https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_bridge.cc:71: if (!base::CreateDirectory(path.DirName())) On 2015/10/26 09:15:00, hidehiko wrote: > Clarification: Which thread will this be invoked on? File operation may not be > allowed. (Note: on the IO thread, it is not allowed conceptually (despite of its > name...)). And one more question: Is this really needed for our purpose? I remember init scripts create the base directory. https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_bridge.cc:77: // TODO(lhchavez): Tighten the security around the socket by tying it to the Maybe better to file a bug so we don't remember to remove this? https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... File chromeos/arc/bridge/common/arc_bridge.h (right): https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_bridge.h:20: class BridgeEndpoint : public IPC::Listener { On 2015/10/26 09:15:00, hidehiko wrote: > What's the usage of BridgeEndpoint? > > If this is just a utility to implement Bridge{Host,Instance}Endpoint classes, > it's better not to inherit, instead of composition. > Or, in this case, I think you can define some utility functions in .cc file, I > guess. +1 If you still want to use a base class, please rename it to BridgeEndpointBase, and protect the constructor. https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_bridge.h:26: // This function is mainly for testing, since callers can call the Do you mean "This function is mainly *exposed* for testing"? https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_bridge.h:61: class BridgeHostEndpoint : public BridgeEndpoint { nit: To follow IPC naming convention, maybe want to rename this to BridgeInstanceHostEndpoint. https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... File chromeos/arc/bridge/common/arc_host_messages.h (right): https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_host_messages.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. Why did you decide to have two message definition files? As you see in ipc_message_start.h, it is a rare case. https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_host_messages.h:14: IPC_MESSAGE_CONTROL0(ArcHostMsg_Pong) Typically this should be named as ArcInstanceHostMsg_*. I looked for chromium codebase but could not find an exceptional naming like this.
hidehiko@ build rule will come in the next change. The reason why I'm splitting it out this way is to cleanly separate changes to this code from the rest of the codebase, since this is a shared implementation with ARC and must be kept in sync. https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... File chromeos/arc/bridge/common/arc_bridge.cc (right): https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_bridge.cc:5: #include "arc_bridge.h" On 2015/10/26 09:15:00, hidehiko wrote: > nit: IIUC, probably we should use > #include "chromeos/arc/bridge/common/arc_bridge.h" This code should be able to be built from within ARC, so the path would not match there. Ideally this should behave more as a third_party/ directory, but I decided against putting this code there since we are the only users of this code. Once an actual shared repository exists, I can move it to third_party. https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_bridge.cc:9: #include <base/bind_helpers.h> On 2015/10/26 09:15:00, hidehiko wrote: > nit: #include "base/bind_helpers.h" > Ditto for below. Done. https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_bridge.cc:13: #include "arc_host_messages.h" On 2015/10/26 09:15:00, hidehiko wrote: > ditto. And sort in lexicographical order (with mixing base/... ipc/...). Since we can't use the full path, I'm a bit wary of mixing the includes. https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_bridge.cc:71: if (!base::CreateDirectory(path.DirName())) On 2015/10/26 11:13:40, Shuhei Takahashi wrote: > On 2015/10/26 09:15:00, hidehiko wrote: > > Clarification: Which thread will this be invoked on? File operation may not be > > allowed. (Note: on the IO thread, it is not allowed conceptually (despite of > its > > name...)). This is run on the browser main thread. I'll add a comment. > > And one more question: Is this really needed for our purpose? I remember init > scripts create the base directory. The scripts do, but we need to stop using the global socket and part of the init scripts. Part of the initialization needs to be per-profile and only after user opt-in. To expedite the code review, I'll remove this and note that the caller must ensure the directory exists. https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... File chromeos/arc/bridge/common/arc_bridge.h (right): https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_bridge.h:20: class BridgeEndpoint : public IPC::Listener { On 2015/10/26 11:13:40, Shuhei Takahashi wrote: > On 2015/10/26 09:15:00, hidehiko wrote: > > What's the usage of BridgeEndpoint? > > > > If this is just a utility to implement Bridge{Host,Instance}Endpoint classes, > > it's better not to inherit, instead of composition. > > Or, in this case, I think you can define some utility functions in .cc file, I > > guess. > > +1 > > If you still want to use a base class, please rename it to BridgeEndpointBase, > and protect the constructor. Having it as a base class avoids repeating the Connect method for testing. https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_bridge.h:26: // This function is mainly for testing, since callers can call the On 2015/10/26 11:13:40, Shuhei Takahashi wrote: > Do you mean "This function is mainly *exposed* for testing"? Done https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_bridge.h:39: // Instance side of the ARC bridge. On 2015/10/26 01:58:52, satorux1 wrote: > I felt that relationship with 'instance side' and 'host side' was not clear. > Could you document it here? Done. https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_bridge.h:51: bool OnMessageReceived(const IPC::Message& message) override; On 2015/10/26 01:58:52, satorux1 wrote: > please add function comments to these functions. it's a bit tedious but Chrome > follows this style rather thoroughly. :) > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm... > > Every function declaration should have comments immediately preceding it that > describe what the function does and how to use it. Done. https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_bridge.h:52: void OnPing(); On 2015/10/26 01:58:52, satorux1 wrote: > What's ping? Is this a sample function or does it have a real use case? Please > document. It was added at some point to avoid a compilation error due to a variable created in the message processing part was not used since there were no other messages defined. Now we have one, so it's not needed. https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_bridge.h:53: virtual void OnRegisterInputDevice(const std::string& name, On 2015/10/26 01:58:52, satorux1 wrote: > In particular, please document how sub classes are supposed to implement this. Done. https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_bridge.h:61: class BridgeHostEndpoint : public BridgeEndpoint { On 2015/10/26 11:13:40, Shuhei Takahashi wrote: > nit: To follow IPC naming convention, maybe want to rename this to > BridgeInstanceHostEndpoint. Oh I was not aware of this convention. Renamed to ArcInstanceBridgeHostBase since it still is not a concrete class but has some logic. https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_bridge.h:66: bool SocketConnect(const std::string& socket_path); On 2015/10/26 01:58:52, satorux1 wrote: > ditto. function comment is missing. Done. https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... File chromeos/arc/bridge/common/arc_host_messages.h (right): https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_host_messages.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/10/26 11:13:40, Shuhei Takahashi wrote: > Why did you decide to have two message definition files? As you see in > ipc_message_start.h, it is a rare case. To make it possible to add messages to the code without having the generated message IDs for other messages change (by only appending to these files). https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_host_messages.h:14: IPC_MESSAGE_CONTROL0(ArcHostMsg_Pong) On 2015/10/26 11:13:40, Shuhei Takahashi wrote: > Typically this should be named as ArcInstanceHostMsg_*. I looked for chromium > codebase but could not find an exceptional naming like this. Done https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... File chromeos/arc/bridge/common/arc_instance_messages.h (right): https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_instance_messages.h:22: // file descriptor provided. On 2015/10/26 01:58:53, satorux1 wrote: > how about adding examples of 'name' and 'device_type' values to illustrate how > these will look like. Done.
https://codereview.chromium.org/1424503002/diff/1/chromeos/arc/libarcbridge/a... File chromeos/arc/libarcbridge/arc_bridge.cc (right): https://codereview.chromium.org/1424503002/diff/1/chromeos/arc/libarcbridge/a... chromeos/arc/libarcbridge/arc_bridge.cc:48: bool BridgeInstanceEndpoint::OnMessageReceived(const IPC::Message& message) { On 2015/10/26 01:58:52, satorux1 wrote: > On 2015/10/23 17:09:24, Luis Héctor Chávez wrote: > > On 2015/10/23 05:16:43, satorux1 wrote: > > > should this function run on the io_thread_? Then you can add an assertion > > here. > > > > > > DCHECK(io_thread_.task_runner()->RunsTasksOnCurrentThread()); > > > > > > would probably work. > > > > No, this is supposed to be processed in the original MessageLoop/Thread. None > of > > the other implementations of IPC have that assertion FWIW. > > I see. In that case, how about adding ThreadChecker instance as a member > variable and use it here (and elsewhere), to ensure that this function is called > in the original thread? maybe missed this comment? https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... File chromeos/arc/bridge/common/arc_bridge.cc (right): https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_bridge.cc:5: #include "arc_bridge.h" On 2015/10/26 21:03:27, Luis Héctor Chávez wrote: > On 2015/10/26 09:15:00, hidehiko wrote: > > nit: IIUC, probably we should use > > #include "chromeos/arc/bridge/common/arc_bridge.h" > > This code should be able to be built from within ARC, so the path would not > match there. Ideally this should behave more as a third_party/ directory, but I > decided against putting this code there since we are the only users of this > code. Once an actual shared repository exists, I can move it to third_party. The relative paths look worrisome to me. tools/git/move_source_file.py assumes include paths are from the top level directory. There may be other tools that has the same assumption. Would it be difficult to get this code compile-able within ARC while using verbose paths like "chromeos/arc/bridge/common/arc_bridge.h"? Also, the "This code should be able to be built from within ARC" requirement is unusual for code in Chrome. Maybe nice to add a README.md file in the directory and explain this?
https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... File chromeos/arc/bridge/common/arc_bridge.cc (right): https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_bridge.cc:5: #include "arc_bridge.h" On 2015/10/27 07:10:16, satorux1 wrote: > On 2015/10/26 21:03:27, Luis Héctor Chávez wrote: > > On 2015/10/26 09:15:00, hidehiko wrote: > > > nit: IIUC, probably we should use > > > #include "chromeos/arc/bridge/common/arc_bridge.h" > > > > This code should be able to be built from within ARC, so the path would not > > match there. Ideally this should behave more as a third_party/ directory, but > I > > decided against putting this code there since we are the only users of this > > code. Once an actual shared repository exists, I can move it to third_party. > > > The relative paths look worrisome to me. tools/git/move_source_file.py assumes > include paths are from the top level directory. There may be other tools that > has the same assumption. > > Would it be difficult to get this code compile-able within ARC while using > verbose paths like "chromeos/arc/bridge/common/arc_bridge.h"? > > > Also, the "This code should be able to be built from within ARC" requirement is > unusual for code in Chrome. Maybe nice to add a README.md file in the directory > and explain this? Maybe this is another reason to avoid having ArcBridgeEndpoint implementations in libarcbridge and provide IPC message definitions only instead.
lhchavez@google.com changed reviewers: + lhchavez@google.com
https://codereview.chromium.org/1424503002/diff/1/chromeos/arc/libarcbridge/a... File chromeos/arc/libarcbridge/arc_bridge.cc (right): https://codereview.chromium.org/1424503002/diff/1/chromeos/arc/libarcbridge/a... chromeos/arc/libarcbridge/arc_bridge.cc:48: bool BridgeInstanceEndpoint::OnMessageReceived(const IPC::Message& message) { On 2015/10/27 07:10:16, satorux1 wrote: > On 2015/10/26 01:58:52, satorux1 wrote: > > On 2015/10/23 17:09:24, Luis Héctor Chávez wrote: > > > On 2015/10/23 05:16:43, satorux1 wrote: > > > > should this function run on the io_thread_? Then you can add an assertion > > > here. > > > > > > > > DCHECK(io_thread_.task_runner()->RunsTasksOnCurrentThread()); > > > > > > > > would probably work. > > > > > > No, this is supposed to be processed in the original MessageLoop/Thread. > None > > of > > > the other implementations of IPC have that assertion FWIW. > > > > I see. In that case, how about adding ThreadChecker instance as a member > > variable and use it here (and elsewhere), to ensure that this function is > called > > in the original thread? > > maybe missed this comment? Forgot to reply, but I did add the DCHECKs that the methods were being called in their original thread. https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... File chromeos/arc/bridge/common/arc_bridge.cc (right): https://codereview.chromium.org/1424503002/diff/20001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_bridge.cc:5: #include "arc_bridge.h" On 2015/10/27 07:14:25, Shuhei Takahashi wrote: > On 2015/10/27 07:10:16, satorux1 wrote: > > On 2015/10/26 21:03:27, Luis Héctor Chávez wrote: > > > On 2015/10/26 09:15:00, hidehiko wrote: > > > > nit: IIUC, probably we should use > > > > #include "chromeos/arc/bridge/common/arc_bridge.h" > > > > > > This code should be able to be built from within ARC, so the path would not > > > match there. Ideally this should behave more as a third_party/ directory, > but > > I > > > decided against putting this code there since we are the only users of this > > > code. Once an actual shared repository exists, I can move it to third_party. > > > > > > The relative paths look worrisome to me. tools/git/move_source_file.py assumes > > include paths are from the top level directory. There may be other tools that > > has the same assumption. > > > > Would it be difficult to get this code compile-able within ARC while using > > verbose paths like "chromeos/arc/bridge/common/arc_bridge.h"? > > > > > > Also, the "This code should be able to be built from within ARC" requirement > is > > unusual for code in Chrome. Maybe nice to add a README.md file in the > directory > > and explain this? > > Maybe this is another reason to avoid having ArcBridgeEndpoint implementations > in libarcbridge and provide IPC message definitions only instead. Yes, it would be very difficult to compile this code within ARC with the verbose paths. I guess there's no choice but to only have the IPC messages here.
https://codereview.chromium.org/1424503002/diff/1/chromeos/arc/libarcbridge/a... File chromeos/arc/libarcbridge/arc_bridge.cc (right): https://codereview.chromium.org/1424503002/diff/1/chromeos/arc/libarcbridge/a... chromeos/arc/libarcbridge/arc_bridge.cc:48: bool BridgeInstanceEndpoint::OnMessageReceived(const IPC::Message& message) { On 2015/10/27 08:00:23, lhc(google) wrote: > On 2015/10/27 07:10:16, satorux1 wrote: > > On 2015/10/26 01:58:52, satorux1 wrote: > > > On 2015/10/23 17:09:24, Luis Héctor Chávez wrote: > > > > On 2015/10/23 05:16:43, satorux1 wrote: > > > > > should this function run on the io_thread_? Then you can add an > assertion > > > > here. > > > > > > > > > > DCHECK(io_thread_.task_runner()->RunsTasksOnCurrentThread()); > > > > > > > > > > would probably work. > > > > > > > > No, this is supposed to be processed in the original MessageLoop/Thread. > > None > > > of > > > > the other implementations of IPC have that assertion FWIW. > > > > > > I see. In that case, how about adding ThreadChecker instance as a member > > > variable and use it here (and elsewhere), to ensure that this function is > > called > > > in the original thread? > > > > maybe missed this comment? > > Forgot to reply, but I did add the DCHECKs that the methods were being called in > their original thread. oops. I didn't noticed about the DCHECKs. thank you for adding them.
The patch description says "libarcbridge: Add libarcbridge" but this does not seem to match what this patch does. Could you update?
https://codereview.chromium.org/1424503002/diff/80001/chromeos/arc/bridge/com... File chromeos/arc/bridge/common/arc_instance_messages.h (right): https://codereview.chromium.org/1424503002/diff/80001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_instance_messages.h:21: // The virtual device will be reading 'struct input_event's from |fd|. Might be nice to document whether or not Chrome should close 'fd' or not, and when to close.
https://codereview.chromium.org/1424503002/diff/80001/chromeos/arc/bridge/com... File chromeos/arc/bridge/common/arc_instance_messages.h (right): https://codereview.chromium.org/1424503002/diff/80001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_instance_messages.h:21: // The virtual device will be reading 'struct input_event's from |fd|. On 2015/10/28 05:17:20, satorux1 wrote: > Might be nice to document whether or not Chrome should close 'fd' or not, and > when to close. Done.
Description was changed from ========== libarcbridge: Add libarcbridge This will allow communication between Chrome and an ARC instance. BUG=b:24339743 ========== to ========== arc-bridge: Add IPC message definitions This code will be shared between Chrome and ARC, and will allow them to communicate with each other. BUG=b:24339743 ==========
Sorry for delay. https://codereview.chromium.org/1424503002/diff/100001/chromeos/arc/bridge/co... File chromeos/arc/bridge/common/arc_host_messages.h (right): https://codereview.chromium.org/1424503002/diff/100001/chromeos/arc/bridge/co... chromeos/arc/bridge/common/arc_host_messages.h:8: #include <stdint.h> nit: Looks unused? https://codereview.chromium.org/1424503002/diff/100001/chromeos/arc/bridge/co... File chromeos/arc/bridge/common/arc_instance_messages.h (right): https://codereview.chromium.org/1424503002/diff/100001/chromeos/arc/bridge/co... chromeos/arc/bridge/common/arc_instance_messages.h:8: #include <stdint.h> ditto. https://codereview.chromium.org/1424503002/diff/100001/chromeos/arc/bridge/co... chromeos/arc/bridge/common/arc_instance_messages.h:11: #include <vector> Ditto. https://codereview.chromium.org/1424503002/diff/100001/chromeos/arc/bridge/co... File chromeos/arc/bridge/common/arc_message_generator.h (right): https://codereview.chromium.org/1424503002/diff/100001/chromeos/arc/bridge/co... chromeos/arc/bridge/common/arc_message_generator.h:7: #include "arc_host_messages.h" Ok, so then, could you add brief comments why this is relative to the chromeos/arc/bridge/common unlike other code in Chrome?
https://codereview.chromium.org/1424503002/diff/100001/chromeos/arc/bridge/co... File chromeos/arc/bridge/common/arc_host_messages.h (right): https://codereview.chromium.org/1424503002/diff/100001/chromeos/arc/bridge/co... chromeos/arc/bridge/common/arc_host_messages.h:8: #include <stdint.h> On 2015/10/28 16:47:31, hidehiko wrote: > nit: Looks unused? Done. https://codereview.chromium.org/1424503002/diff/100001/chromeos/arc/bridge/co... File chromeos/arc/bridge/common/arc_instance_messages.h (right): https://codereview.chromium.org/1424503002/diff/100001/chromeos/arc/bridge/co... chromeos/arc/bridge/common/arc_instance_messages.h:8: #include <stdint.h> On 2015/10/28 16:47:31, hidehiko wrote: > ditto. Done. https://codereview.chromium.org/1424503002/diff/100001/chromeos/arc/bridge/co... chromeos/arc/bridge/common/arc_instance_messages.h:11: #include <vector> On 2015/10/28 16:47:31, hidehiko wrote: > Ditto. Done. https://codereview.chromium.org/1424503002/diff/100001/chromeos/arc/bridge/co... File chromeos/arc/bridge/common/arc_message_generator.h (right): https://codereview.chromium.org/1424503002/diff/100001/chromeos/arc/bridge/co... chromeos/arc/bridge/common/arc_message_generator.h:7: #include "arc_host_messages.h" On 2015/10/28 16:47:31, hidehiko wrote: > Ok, so then, could you add brief comments why this is relative to the > chromeos/arc/bridge/common unlike other code in Chrome? Done.
LGTM, but please wait for other reviews' replies, too.
LGTM Thanks for this patch!
lhchavez@chromium.org changed reviewers: + elijahtaylor@chromium.org
LGTM. ipc/ needs OWNERS approval. jln@ says (slow on Chromium), so maybe ask someone else if he's busy?
On 2015/10/29 05:15:56, satorux1 wrote: > LGTM. ipc/ needs OWNERS approval. jln@ says (slow on Chromium), so maybe ask > someone else if he's busy? FYI: I forgot to mention that, AFAIK, IPC change will need security review, and probably OWNERS may need to be updated. cf) https://code.google.com/p/chromium/codesearch#chromium/src/components/nacl/co... I think you can ask to another reviewer from the list, if necessary.
lhchavez@chromium.org changed required reviewers: + jln@chromium.org
ping?
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
https://codereview.chromium.org/1424503002/diff/160001/chromeos/arc/bridge/co... File chromeos/arc/bridge/common/arc_instance_messages.h (right): https://codereview.chromium.org/1424503002/diff/160001/chromeos/arc/bridge/co... chromeos/arc/bridge/common/arc_instance_messages.h:24: base::FileDescriptor /* fd */) I know this is the only way to send FDs across IPC today, but would you be open to using ScopedFD once I land ParamTraits for that? base::FileDescriptor is a class that I'd really like to get rid of. https://codereview.chromium.org/1424503002/diff/160001/chromeos/arc/bridge/co... File chromeos/arc/bridge/common/arc_message_generator.h (right): https://codereview.chromium.org/1424503002/diff/160001/chromeos/arc/bridge/co... chromeos/arc/bridge/common/arc_message_generator.h:8: #include "arc_host_messages.h" Does this mean *everything* in here is going to use relative include paths?
https://codereview.chromium.org/1424503002/diff/160001/chromeos/arc/bridge/co... File chromeos/arc/bridge/common/arc_instance_messages.h (right): https://codereview.chromium.org/1424503002/diff/160001/chromeos/arc/bridge/co... chromeos/arc/bridge/common/arc_instance_messages.h:24: base::FileDescriptor /* fd */) On 2015/11/04 00:06:26, dcheng wrote: > I know this is the only way to send FDs across IPC today, but would you be open > to using ScopedFD once I land ParamTraits for that? > > base::FileDescriptor is a class that I'd really like to get rid of. Totally. Just let me know once that is landed and I'll make the change. https://codereview.chromium.org/1424503002/diff/160001/chromeos/arc/bridge/co... File chromeos/arc/bridge/common/arc_message_generator.h (right): https://codereview.chromium.org/1424503002/diff/160001/chromeos/arc/bridge/co... chromeos/arc/bridge/common/arc_message_generator.h:8: #include "arc_host_messages.h" On 2015/11/04 00:06:26, dcheng wrote: > Does this mean *everything* in here is going to use relative include paths? I managed to reduce the relative path usage to just this file and the message_generator.cc. The rest of the files should use normal includes.
https://codereview.chromium.org/1424503002/diff/160001/chromeos/arc/bridge/co... File chromeos/arc/bridge/common/arc_instance_messages.h (right): https://codereview.chromium.org/1424503002/diff/160001/chromeos/arc/bridge/co... chromeos/arc/bridge/common/arc_instance_messages.h:24: base::FileDescriptor /* fd */) On 2015/11/04 03:00:28, lhc(google) wrote: > On 2015/11/04 00:06:26, dcheng wrote: > > I know this is the only way to send FDs across IPC today, but would you be > open > > to using ScopedFD once I land ParamTraits for that? > > > > base::FileDescriptor is a class that I'd really like to get rid of. > > Totally. Just let me know once that is landed and I'll make the change. Rather, I want to land this as soon as possible since there are a few changes that the whole team depends on that require this. Once that's done, I'm willing to come back and change it with ScopedFD.
lgtm however, please explicitly add an ipc reviewer for the followup change that actually sends/handles the new IPCs. That's really the core of the security IPC review, and none of that was present here. https://codereview.chromium.org/1424503002/diff/160001/chromeos/arc/bridge/co... File chromeos/arc/bridge/common/arc_instance_messages.h (right): https://codereview.chromium.org/1424503002/diff/160001/chromeos/arc/bridge/co... chromeos/arc/bridge/common/arc_instance_messages.h:24: base::FileDescriptor /* fd */) On 2015/11/04 at 16:40:58, Luis Héctor Chávez wrote: > On 2015/11/04 03:00:28, lhc(google) wrote: > > On 2015/11/04 00:06:26, dcheng wrote: > > > I know this is the only way to send FDs across IPC today, but would you be > > open > > > to using ScopedFD once I land ParamTraits for that? > > > > > > base::FileDescriptor is a class that I'd really like to get rid of. > > > > Totally. Just let me know once that is landed and I'll make the change. > > Rather, I want to land this as soon as possible since there are a few changes that the whole team depends on that require this. Once that's done, I'm willing to come back and change it with ScopedFD. That's fine.
lhchavez@google.com changed required reviewers: - jln@chromium.org
will do. thanks!
The CQ bit was checked by lhchavez@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from hidehiko@chromium.org, nya@chromium.org, satorux@chromium.org Link to the patchset: https://codereview.chromium.org/1424503002/#ps160001 (title: "Added IPC-specific OWNERS file")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1424503002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1424503002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/67d14d4249ec9be027df91655d301872e75d493a Cr-Commit-Position: refs/heads/master@{#357996} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
