Add CommandBroker::OnTransaction to handle incoming transactions

Add CommandBroker::OnTransaction to handle incoming transactions

To handle incoming transactions:
  Add CommandStream::IncomingCommandHandler::OnTransaction.
  Add CommandBroker::OnTransaction.

To implement test:
  Add CommandBroker::EnterLooper, ExitLooper, PollCommands.
  Add Thread.
  Add TestService.
  Add BinderEndToEndTest.

BUG=563282
Committed: https://crrev.com/bb42dee07a4a8df5dfa5ad33378b877e7b2944f3
Cr-Commit-Position: refs/heads/master@{#369385}

[hashimoto, 2016-01-12 07:44:32]

Description was changed from

==========
Add CommandBroker::OnTransaction to handle incoming transactions

Add CommandStream::IncomingCommandHandler::OnTransaction.
Add CommandBroker::OnTransaction, EnterLooper, ExitLooper, PollCommands.
Add Thread.
Add TestService.

BUG=563282
==========

to

==========
Add CommandBroker::OnTransaction to handle incoming transactions

To handle incoming transactions:
  Add CommandStream::IncomingCommandHandler::OnTransaction.
  Add CommandBroker::OnTransaction.

To implement test:
  Add CommandBroker::EnterLooper, ExitLooper, PollCommands.
  Add Thread.
  Add TestService.
  Add BinderEndToEndTest.

BUG=563282
==========

[hashimoto, 2016-01-12 08:07:56]

Patchset #1 (id:1) has been deleted

[hashimoto, 2016-01-12 08:08:04]

hashimoto@chromium.org changed reviewers:
+ satorux@chromium.org

[satorux1, 2016-01-13 04:33:21]

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/command...
File chromeos/binder/command_broker.cc (right):

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/command...
chromeos/binder/command_broker.cc:62:
DCHECK(!command_stream_.CanProcessIncomingCommand());
so it's not ok to call this function while CanProcessIncomingCommand() is true.
maybe we should document when to call this function in the function comment?

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/command...
chromeos/binder/command_broker.cc:148: tr.target.handle = -1;
maybe document why -1?

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/command...
chromeos/binder/command_broker.cc:157: << "Error on the other end when sending
reply: " << response_type;
in this case, should this return true?

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/command...
File chromeos/binder/command_broker.h (right):

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/command...
chromeos/binder/command_broker.h:35: // Tells the driver that the current thread
entered command handling loop.
Please document the return value here and elsewhere too

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/end_to_...
File chromeos/binder/end_to_end_unittest.cc (right):

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/end_to_...
chromeos/binder/end_to_end_unittest.cc:40: TEST_F(BinderEndToEndTest,
IncrementInt) {
nice!

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/test_se...
File chromeos/binder/test_service.h (right):

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/test_se...
chromeos/binder/test_service.h:16: // Opens binder driver on its own thread and
provides a service.
Maybe document that the service runs in the same process? Would it be difficult
to run it in a separate process?

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/test_se...
chromeos/binder/test_service.h:29: // Starts and waits for the service to start.
please document the return value.

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/test_se...
chromeos/binder/test_service.h:38: void Initialize(bool* result);
function comment is missing.

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/thread.cc
File chromeos/binder/thread.cc (right):

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/thread....
chromeos/binder/thread.cc:27: void Thread::OnFileCanReadWithoutBlocking(int fd)
{
maybe add DCHECK(initialized_) here and elsewhere?

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/thread.h
File chromeos/binder/thread.h (right):

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/thread....
chromeos/binder/thread.h:19: class CHROMEOS_EXPORT Thread : public base::Thread,
This is called 'Thread' inside of the 'binder' namespace, which I felt a bit too
generic. Can it have a more descriptive name? for instance if this is for
implementing serevices, maybe ServiceThread

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/thread....
chromeos/binder/thread.h:29: // Starts this thread.
document the return value?

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/thread....
chromeos/binder/thread.h:37: // base::Thread override:
nit: 'overrides' may be a bit more common?

[hashimoto, 2016-01-13 05:46:46]

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/command...
File chromeos/binder/command_broker.cc (right):

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/command...
chromeos/binder/command_broker.cc:62:
DCHECK(!command_stream_.CanProcessIncomingCommand());
On 2016/01/13 04:33:20, satorux1 wrote:
> so it's not ok to call this function while CanProcessIncomingCommand() is
true.
> maybe we should document when to call this function in the function comment?

I put this DCHECK here to ensure that all previously-fetched commands were
processed in the last PollCommands() call, as leaving fetched commands
unprocessed can result in letting the transaction sender process wait forever.

However, I realized that this DCHECK is unnecessary as there is while
(command_stream_.CanProcessIncomingCommand()) loop below.
So removed this DCHECK.

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/command...
chromeos/binder/command_broker.cc:148: tr.target.handle = -1;
On 2016/01/13 04:33:20, satorux1 wrote:
> maybe document why -1? 

Done.

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/command...
chromeos/binder/command_broker.cc:157: << "Error on the other end when sending
reply: " << response_type;
On 2016/01/13 04:33:20, satorux1 wrote:
> in this case, should this return true?

Added a comment.

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/command...
File chromeos/binder/command_broker.h (right):

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/command...
chromeos/binder/command_broker.h:35: // Tells the driver that the current thread
entered command handling loop.
On 2016/01/13 04:33:20, satorux1 wrote:
> Please document the return value here and elsewhere too

Done.

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/end_to_...
File chromeos/binder/end_to_end_unittest.cc (right):

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/end_to_...
chromeos/binder/end_to_end_unittest.cc:40: TEST_F(BinderEndToEndTest,
IncrementInt) {
On 2016/01/13 04:33:20, satorux1 wrote:
> nice!

Yey, finally!

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/test_se...
File chromeos/binder/test_service.h (right):

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/test_se...
chromeos/binder/test_service.h:16: // Opens binder driver on its own thread and
provides a service.
On 2016/01/13 04:33:20, satorux1 wrote:
> Maybe document that the service runs in the same process? Would it be
difficult
> to run it in a separate process?

I'm not sure if there is any test running in a different process.
As opening two FDs of /dev/binder is enough to let the driver think the test is
running in a separate process, actually there is no need to do that.

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/test_se...
chromeos/binder/test_service.h:29: // Starts and waits for the service to start.
On 2016/01/13 04:33:20, satorux1 wrote:
> please document the return value.

Done.

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/test_se...
chromeos/binder/test_service.h:38: void Initialize(bool* result);
On 2016/01/13 04:33:20, satorux1 wrote:
> function comment is missing.

Done.

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/thread.cc
File chromeos/binder/thread.cc (right):

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/thread....
chromeos/binder/thread.cc:27: void Thread::OnFileCanReadWithoutBlocking(int fd)
{
On 2016/01/13 04:33:20, satorux1 wrote:
> maybe add DCHECK(initialized_) here and elsewhere?

Done.

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/thread.h
File chromeos/binder/thread.h (right):

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/thread....
chromeos/binder/thread.h:19: class CHROMEOS_EXPORT Thread : public base::Thread,
On 2016/01/13 04:33:20, satorux1 wrote:
> This is called 'Thread' inside of the 'binder' namespace, which I felt a bit
too
> generic. Can it have a more descriptive name? for instance if this is for
> implementing serevices, maybe ServiceThread

binder::Thread is a "binder thread" in the same sense as the thread managed by
DBusThreadManager is called the "D-Bus thread".
It's not only for implementing services, but also to make transactions with
services implemented by other processes, by posting a task to this thread from
IO-prohibited threads like the UI thread.

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/thread....
chromeos/binder/thread.h:29: // Starts this thread.
On 2016/01/13 04:33:21, satorux1 wrote:
> document the return value?

Done.

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/thread....
chromeos/binder/thread.h:37: // base::Thread override:
On 2016/01/13 04:33:21, satorux1 wrote:
> nit: 'overrides' may be a bit more common?

Done.

[satorux1, 2016-01-14 05:47:16]

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/test_se...
File chromeos/binder/test_service.h (right):

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/test_se...
chromeos/binder/test_service.h:16: // Opens binder driver on its own thread and
provides a service.
On 2016/01/13 05:46:46, hashimoto wrote:
> On 2016/01/13 04:33:20, satorux1 wrote:
> > Maybe document that the service runs in the same process? Would it be
> difficult
> > to run it in a separate process?
> 
> I'm not sure if there is any test running in a different process.
> As opening two FDs of /dev/binder is enough to let the driver think the test
is
> running in a separate process, actually there is no need to do that.

Then how about documenting that the service runs in the same process and it's
sufficient because opening two FDs of /dev/binder is enough to let the driver
think the test is
running in a separate proces?

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/thread.h
File chromeos/binder/thread.h (right):

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/thread....
chromeos/binder/thread.h:18: // Thread manages binder-related resources and
handles incoming binder commands.
Also document that this opens the binder driver?

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/thread....
chromeos/binder/thread.h:20: public base::MessageLoopForIO::Watcher {
Maybe IpcThread?

[hashimoto, 2016-01-14 06:24:29]

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/test_se...
File chromeos/binder/test_service.h (right):

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/test_se...
chromeos/binder/test_service.h:16: // Opens binder driver on its own thread and
provides a service.
On 2016/01/14 05:47:16, satorux1 wrote:
> On 2016/01/13 05:46:46, hashimoto wrote:
> > On 2016/01/13 04:33:20, satorux1 wrote:
> > > Maybe document that the service runs in the same process? Would it be
> > difficult
> > > to run it in a separate process?
> > 
> > I'm not sure if there is any test running in a different process.
> > As opening two FDs of /dev/binder is enough to let the driver think the test
> is
> > running in a separate process, actually there is no need to do that.
> 
> Then how about documenting that the service runs in the same process and it's
> sufficient because opening two FDs of /dev/binder is enough to let the driver
> think the test is
> running in a separate proces?

Added a note.

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/thread.h
File chromeos/binder/thread.h (right):

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/thread....
chromeos/binder/thread.h:18: // Thread manages binder-related resources and
handles incoming binder commands.
On 2016/01/14 05:47:16, satorux1 wrote:
> Also document that this opens the binder driver?

Done.

https://codereview.chromium.org/1575313002/diff/20001/chromeos/binder/thread....
chromeos/binder/thread.h:20: public base::MessageLoopForIO::Watcher {
On 2016/01/14 05:47:16, satorux1 wrote:
> Maybe IpcThread?

Done.

[satorux1, 2016-01-14 06:34:18]

LGTM with some comments

https://codereview.chromium.org/1575313002/diff/60001/chromeos/binder/end_to_...
File chromeos/binder/end_to_end_unittest.cc (right):

https://codereview.chromium.org/1575313002/diff/60001/chromeos/binder/end_to_...
chromeos/binder/end_to_end_unittest.cc:28: &command_broker_,
test_service_.service_name());
ASSERT_EQ(TYPE_REMOTE, remote_object->GetType()) to confirm that it's a remote
object?

https://codereview.chromium.org/1575313002/diff/60001/chromeos/binder/ipc_thr...
File chromeos/binder/ipc_thread.h (right):

https://codereview.chromium.org/1575313002/diff/60001/chromeos/binder/ipc_thr...
chromeos/binder/ipc_thread.h:19: // handles incoming binder commands.
Is it OK to instantiate this class and call Start() from a non-blocking thread
like UI thread? Would be nice to document.

[hashimoto, 2016-01-14 06:49:38]

https://codereview.chromium.org/1575313002/diff/60001/chromeos/binder/end_to_...
File chromeos/binder/end_to_end_unittest.cc (right):

https://codereview.chromium.org/1575313002/diff/60001/chromeos/binder/end_to_...
chromeos/binder/end_to_end_unittest.cc:28: &command_broker_,
test_service_.service_name());
On 2016/01/14 06:34:18, satorux1 wrote:
> ASSERT_EQ(TYPE_REMOTE, remote_object->GetType()) to confirm that it's a remote
> object?

Done.

https://codereview.chromium.org/1575313002/diff/60001/chromeos/binder/ipc_thr...
File chromeos/binder/ipc_thread.h (right):

https://codereview.chromium.org/1575313002/diff/60001/chromeos/binder/ipc_thr...
chromeos/binder/ipc_thread.h:19: // handles incoming binder commands.
On 2016/01/14 06:34:18, satorux1 wrote:
> Is it OK to instantiate this class and call Start() from a non-blocking thread
> like UI thread? Would be nice to document.

It's OK to do that like all other base::Thread sub-classes.
Do you think this class is doing something tricky when starting thread so it
needs special comments?
If not, I'd like to avoid having redundant comments.

[satorux1, 2016-01-14 07:07:13]

https://codereview.chromium.org/1575313002/diff/60001/chromeos/binder/ipc_thr...
File chromeos/binder/ipc_thread.h (right):

https://codereview.chromium.org/1575313002/diff/60001/chromeos/binder/ipc_thr...
chromeos/binder/ipc_thread.h:19: // handles incoming binder commands.
On 2016/01/14 06:49:38, hashimoto wrote:
> On 2016/01/14 06:34:18, satorux1 wrote:
> > Is it OK to instantiate this class and call Start() from a non-blocking
thread
> > like UI thread? Would be nice to document.
> 
> It's OK to do that like all other base::Thread sub-classes.
> Do you think this class is doing something tricky when starting thread so it
> needs special comments?
> If not, I'd like to avoid having redundant comments.

I felt this class was tricky because it deals with the binder driver.

[hashimoto, 2016-01-14 08:52:03]

https://codereview.chromium.org/1575313002/diff/60001/chromeos/binder/ipc_thr...
File chromeos/binder/ipc_thread.h (right):

https://codereview.chromium.org/1575313002/diff/60001/chromeos/binder/ipc_thr...
chromeos/binder/ipc_thread.h:19: // handles incoming binder commands.
On 2016/01/14 07:07:13, satorux1 wrote:
> On 2016/01/14 06:49:38, hashimoto wrote:
> > On 2016/01/14 06:34:18, satorux1 wrote:
> > > Is it OK to instantiate this class and call Start() from a non-blocking
> thread
> > > like UI thread? Would be nice to document.
> > 
> > It's OK to do that like all other base::Thread sub-classes.
> > Do you think this class is doing something tricky when starting thread so it
> > needs special comments?
> > If not, I'd like to avoid having redundant comments.
> 
> I felt this class was tricky because it deals with the binder driver.

It deals with the binder driver, but all meaningful things happen on the thread
managed by this object.
As this class follows the standard way in which most base::Thread sub-classes
are implemented, I'd rather let the reader reference base/threading/thread.h for
that kind of information.

[hashimoto, 2016-01-14 09:45:37]

The CQ bit was checked by hashimoto@chromium.org

[hashimoto, 2016-01-14 09:45:37]

The patchset sent to the CQ was uploaded after l-g-t-m from satorux@chromium.org
Link to the patchset: https://codereview.chromium.org/1575313002/#ps80001
(title: "Add comment")

[commit-bot: I haz the power, 2016-01-14 09:45:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1575313002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1575313002/80001

[commit-bot: I haz the power, 2016-01-14 10:14:04]

Description was changed from

==========
Add CommandBroker::OnTransaction to handle incoming transactions

To handle incoming transactions:
  Add CommandStream::IncomingCommandHandler::OnTransaction.
  Add CommandBroker::OnTransaction.

To implement test:
  Add CommandBroker::EnterLooper, ExitLooper, PollCommands.
  Add Thread.
  Add TestService.
  Add BinderEndToEndTest.

BUG=563282
==========

to

==========
Add CommandBroker::OnTransaction to handle incoming transactions

To handle incoming transactions:
  Add CommandStream::IncomingCommandHandler::OnTransaction.
  Add CommandBroker::OnTransaction.

To implement test:
  Add CommandBroker::EnterLooper, ExitLooper, PollCommands.
  Add Thread.
  Add TestService.
  Add BinderEndToEndTest.

BUG=563282
==========

[commit-bot: I haz the power, 2016-01-14 10:14:04]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-01-14 10:15:36]

Description was changed from

==========
Add CommandBroker::OnTransaction to handle incoming transactions

To handle incoming transactions:
  Add CommandStream::IncomingCommandHandler::OnTransaction.
  Add CommandBroker::OnTransaction.

To implement test:
  Add CommandBroker::EnterLooper, ExitLooper, PollCommands.
  Add Thread.
  Add TestService.
  Add BinderEndToEndTest.

BUG=563282
==========

to

==========
Add CommandBroker::OnTransaction to handle incoming transactions

To handle incoming transactions:
  Add CommandStream::IncomingCommandHandler::OnTransaction.
  Add CommandBroker::OnTransaction.

To implement test:
  Add CommandBroker::EnterLooper, ExitLooper, PollCommands.
  Add Thread.
  Add TestService.
  Add BinderEndToEndTest.

BUG=563282

Committed: https://crrev.com/bb42dee07a4a8df5dfa5ad33378b877e7b2944f3
Cr-Commit-Position: refs/heads/master@{#369385}
==========

[commit-bot: I haz the power, 2016-01-14 10:15:37]

Patchset 4 (id:??) landed as
https://crrev.com/bb42dee07a4a8df5dfa5ad33378b877e7b2944f3
Cr-Commit-Position: refs/heads/master@{#369385}