Implement InputStub in the host side for chromoting

Implement input stub in the host side for chromoting

Implement InputStub for the host.

BUG=None
TEST=None

[Alpha Left Google, 2010-11-10 01:43:03]

Sending this out for comments. I'm a little worried about threading so please
tell me if this is not correct. In the worst case I can have ChromotingHost
implement InputStub and do a thread switching there.

[awong, 2010-11-10 19:36:54]

I see what you mean about the threading.

This comes a bit back to the host structuring issue.

I think we should have at least the following generic concepts (these are not
our current names):
  1) network thread
  2) application thread

The stub implementation should run on the application thread.

Currently, we have "main thread" and "jingle thread".  I think we need to get
more defined about what these are.  Jingle thread is obviously the network
thread.  I think the main thread is our "application thread."

http://codereview.chromium.org/4726003/diff/1/remoting/host/chromoting_host.h
File remoting/host/chromoting_host.h (right):

http://codereview.chromium.org/4726003/diff/1/remoting/host/chromoting_host.h...
remoting/host/chromoting_host.h:66: Capturer* capturer, protocol::InputStub*
input_stub);
I feel like the ChromotingHost should actually keep taking EventExecutor, and
that EventExecutor just implements InputStub.  No?

http://codereview.chromium.org/4726003/diff/1/remoting/host/event_executor_wi...
File remoting/host/event_executor_win.cc (right):

http://codereview.chromium.org/4726003/diff/1/remoting/host/event_executor_wi...
remoting/host/event_executor_win.cc:25: void
EventExecutorMac::InjectKeyEvent(const KeyEvent* event, Task* done) {
You should talk to Dave about this.

I would be happiest if this code was not executed on the network thread at all. 
In particular, done->Run() scares me.

http://codereview.chromium.org/4726003/diff/1/remoting/protocol/connection_to...
File remoting/protocol/connection_to_client.h (right):

http://codereview.chromium.org/4726003/diff/1/remoting/protocol/connection_to...
remoting/protocol/connection_to_client.h:23: class InputStub;
alphabetical order

[Sergey Ulanov, 2010-11-10 20:14:21]

Agree with Albert on the threading issue. We should handle any messages on the
network thread. I think dispatcher should dispatch messages on the application
(main) thread instead of doing it on the network thread as soon as it receives
them.

http://codereview.chromium.org/4726003/diff/1/remoting/host/chromoting_host.h
File remoting/host/chromoting_host.h (right):

http://codereview.chromium.org/4726003/diff/1/remoting/host/chromoting_host.h...
remoting/host/chromoting_host.h:66: Capturer* capturer, protocol::InputStub*
input_stub);
On 2010/11/10 19:36:54, awong wrote:
> I feel like the ChromotingHost should actually keep taking EventExecutor, and
> that EventExecutor just implements InputStub.  No?

How EventExecutor would be different from InputStub? I think the type should be
InputStub, but it should be called event_executor, just to make clear its
purpose.

http://codereview.chromium.org/4726003/diff/1/remoting/host/chromoting_host.h...
remoting/host/chromoting_host.h:134: scoped_ptr<protocol::InputStub>
input_stub_;
event_executor_?

http://codereview.chromium.org/4726003/diff/1/remoting/proto/event.proto
File remoting/proto/event.proto (right):

http://codereview.chromium.org/4726003/diff/1/remoting/proto/event.proto#newc...
remoting/proto/event.proto:74: optional int32 mouse_width = 3;
What is mouse width/height?

http://codereview.chromium.org/4726003/diff/1/remoting/protocol/connection_to...
File remoting/protocol/connection_to_client.h (right):

http://codereview.chromium.org/4726003/diff/1/remoting/protocol/connection_to...
remoting/protocol/connection_to_client.h:116: scoped_ptr<HostMessageDispatcher>
dispatcher_;
Does this need to be a pointer?

http://codereview.chromium.org/4726003/diff/1/remoting/protocol/host_message_...
File remoting/protocol/host_message_dispatcher.cc (right):

http://codereview.chromium.org/4726003/diff/1/remoting/protocol/host_message_...
remoting/protocol/host_message_dispatcher.cc:82:
message->mutable_suggest_resolution(), NewDeleteTask(ref_msg));
&message->suggest_resolution()?

[awong, 2010-11-10 20:20:58]

On Wed, Nov 10, 2010 at 12:14 PM, <sergeyu@chromium.org> wrote:

> Agree with Albert on the threading issue. We should handle any messages on
> the
> network thread. I think dispatcher should dispatch messages on the
> application
> (main) thread instead of doing it on the network thread as soon as it
> receives
> them.


I rather have the stub do the threadswitch, and leave the dispatcher to only
be aware of the network thread.



> http://codereview.chromium.org/4726003/diff/1/remoting/host/chromoting_host.h
> File remoting/host/chromoting_host.h (right):
>
>
>
http://codereview.chromium.org/4726003/diff/1/remoting/host/chromoting_host.h...
> remoting/host/chromoting_host.h:66: Capturer* capturer,
> protocol::InputStub* input_stub);
> On 2010/11/10 19:36:54, awong wrote:
>
>> I feel like the ChromotingHost should actually keep taking
>>
> EventExecutor, and
>
>> that EventExecutor just implements InputStub.  No?
>>
>
> How EventExecutor would be different from InputStub? I think the type
> should be InputStub, but it should be called event_executor, just to
> make clear its purpose.


I think I'm being OCD about layering.  Feel free to disregard this if you
want.



>
>
>
http://codereview.chromium.org/4726003/diff/1/remoting/host/chromoting_host.h...
> remoting/host/chromoting_host.h:134: scoped_ptr<protocol::InputStub>
> input_stub_;
> event_executor_?
>
> http://codereview.chromium.org/4726003/diff/1/remoting/proto/event.proto
> File remoting/proto/event.proto (right):
>
>
>
http://codereview.chromium.org/4726003/diff/1/remoting/proto/event.proto#newc...
> remoting/proto/event.proto:74: optional int32 mouse_width = 3;
> What is mouse width/height?
>
>
>
>
http://codereview.chromium.org/4726003/diff/1/remoting/protocol/connection_to...
> File remoting/protocol/connection_to_client.h (right):
>
>
>
http://codereview.chromium.org/4726003/diff/1/remoting/protocol/connection_to...
> remoting/protocol/connection_to_client.h:116:
> scoped_ptr<HostMessageDispatcher> dispatcher_;
> Does this need to be a pointer?
>
>
>
http://codereview.chromium.org/4726003/diff/1/remoting/protocol/host_message_...
> File remoting/protocol/host_message_dispatcher.cc (right):
>
>
>
http://codereview.chromium.org/4726003/diff/1/remoting/protocol/host_message_...
> remoting/protocol/host_message_dispatcher.cc:82:
> message->mutable_suggest_resolution(), NewDeleteTask(ref_msg));
> &message->suggest_resolution()?
>
>
> http://codereview.chromium.org/4726003/
>

[Alpha Left Google, 2010-11-11 02:53:31]

Event executors now run on the capture thread. Also changed from using
MessageLoop to MessageLoopProxy. Will slowly move to that direction for the rest
of threads.

[awong, 2010-11-12 01:40:10]

Looks pretty good

I've got some minor comments.  The biggest of which is MessageLoopProxy...I
still feel like we shouldn't have it...

http://codereview.chromium.org/4726003/diff/22001/remoting/host/chromoting_ho...
File remoting/host/chromoting_host_context.h (right):

http://codereview.chromium.org/4726003/diff/22001/remoting/host/chromoting_ho...
remoting/host/chromoting_host_context.h:34: virtual
scoped_refptr<base::MessageLoopProxy> capture_message_loop_proxy();
The more I think about this, more I don't really see a need to use the proxy. 
If nothing else, it makes all these calls ugly cause you have to say _proxy()
everywhere.  Our destruction is structured well enough that I don't think that
this will save us anything in practicality.

http://codereview.chromium.org/4726003/diff/22001/remoting/host/event_executo...
File remoting/host/event_executor_linux.cc (right):

http://codereview.chromium.org/4726003/diff/22001/remoting/host/event_executo...
remoting/host/event_executor_linux.cc:442:
DISABLE_RUNNABLE_METHOD_REFCOUNT(remoting::EventExecutorLinux);
This needs to go in the header file.

http://codereview.chromium.org/4726003/diff/22001/remoting/host/event_executo...
File remoting/host/event_executor_mac.h (right):

http://codereview.chromium.org/4726003/diff/22001/remoting/host/event_executo...
remoting/host/event_executor_mac.h:10: #include "base/ref_counted.h"
ordering

http://codereview.chromium.org/4726003/diff/22001/remoting/host/simple_host_p...
File remoting/host/simple_host_process.cc (right):

http://codereview.chromium.org/4726003/diff/22001/remoting/host/simple_host_p...
remoting/host/simple_host_process.cc:126: context.Stop();
Don't we want to do an explicit stop in the normal path?

http://codereview.chromium.org/4726003/diff/22001/remoting/protocol/connectio...
File remoting/protocol/connection_to_client.h (right):

http://codereview.chromium.org/4726003/diff/22001/remoting/protocol/connectio...
remoting/protocol/connection_to_client.h:32: public
base::RefCountedThreadSafe<ConnectionToClient> {
I thought this should only be indent 4 spaces?

http://codereview.chromium.org/4726003/diff/22001/remoting/protocol/host_mess...
File remoting/protocol/host_message_dispatcher.cc (right):

http://codereview.chromium.org/4726003/diff/22001/remoting/protocol/host_mess...
remoting/protocol/host_message_dispatcher.cc:82:
message->mutable_suggest_resolution(), NewDeleteTask(ref_msg));
why do we need this to be mutable?

http://codereview.chromium.org/4726003/diff/22001/remoting/protocol/host_mess...
remoting/protocol/host_message_dispatcher.cc:93:
message->mutable_event(i)->mutable_key(), NewDeleteTask(ref_msg));
Why are all these mutable?

[Alpha Left Google, 2010-11-12 02:10:39]

fixed also windows. please take another look.

[Sergey Ulanov, 2010-11-12 19:52:40]

I agree with Albert about MessageLoopProxy usage. If we worry about threads
destroyed unexpectedly then we also shouldn't
use DISABLE_RUNNABLE_METHOD_REFCOUNT left and right as we do know, because
we create/destroy those objects more often than threads.

Can you please also answer my comments for this CL?

On Thu, Nov 11, 2010 at 6:10 PM, <hclam@chromium.org> wrote:

> fixed also windows. please take another look.
>
> http://codereview.chromium.org/4726003/
>

[Sergey Ulanov, 2010-11-12 19:55:28]

On Fri, Nov 12, 2010 at 11:52 AM, Sergey Ulanov <sergeyu@chromium.org>wrote:

> I agree with Albert about MessageLoopProxy usage. If we worry about threads
> destroyed unexpectedly then we also shouldn't
> use DISABLE_RUNNABLE_METHOD_REFCOUNT left and right as we do *know*,
> because we create/destroy those objects more often than threads.

s/know/now/


>
> Can you please also answer my comments for this CL?
>
>
> On Thu, Nov 11, 2010 at 6:10 PM, <hclam@chromium.org> wrote:
>
>> fixed also windows. please take another look.
>>
>> http://codereview.chromium.org/4726003/
>>
>
>

[Alpha Left Google, 2010-11-12 20:07:51]

MessageLoopProxy is the newer version of using MessageLoop which gives us
protection during teardown. We still make strong guarantees about thread
destruction order and that message loop given to InputStub live strictly longer
than the object. However one thing I wasn't sure that made me into using
MessageLoopProxy is the jingle thread and its lifetime. I don't think using it
hurts at all but do give us protection during destruction, it'll be handy if we
want to solve some ugly teardown crashes.

[Alpha Left Google, 2010-11-12 20:08:01]

http://codereview.chromium.org/4726003/diff/1/remoting/host/chromoting_host.h
File remoting/host/chromoting_host.h (right):

http://codereview.chromium.org/4726003/diff/1/remoting/host/chromoting_host.h...
remoting/host/chromoting_host.h:66: Capturer* capturer, protocol::InputStub*
input_stub);
On 2010/11/10 20:14:21, sergeyu wrote:
> On 2010/11/10 19:36:54, awong wrote:
> > I feel like the ChromotingHost should actually keep taking EventExecutor,
and
> > that EventExecutor just implements InputStub.  No?
> 
> How EventExecutor would be different from InputStub? I think the type should
be
> InputStub, but it should be called event_executor, just to make clear its
> purpose.

Getting rid of EventExecutor and shift to use InputStub. Evently we will just
them as InputStub.

http://codereview.chromium.org/4726003/diff/1/remoting/host/chromoting_host.h...
remoting/host/chromoting_host.h:134: scoped_ptr<protocol::InputStub>
input_stub_;
On 2010/11/10 20:14:21, sergeyu wrote:
> event_executor_?

It will be merged with capturer so reference them as input_stub_.

http://codereview.chromium.org/4726003/diff/1/remoting/proto/event.proto
File remoting/proto/event.proto (right):

http://codereview.chromium.org/4726003/diff/1/remoting/proto/event.proto#newc...
remoting/proto/event.proto:74: optional int32 mouse_width = 3;
On 2010/11/10 20:14:21, sergeyu wrote:
> What is mouse width/height?

This is removed in the latest patch.

http://codereview.chromium.org/4726003/diff/1/remoting/protocol/connection_to...
File remoting/protocol/connection_to_client.h (right):

http://codereview.chromium.org/4726003/diff/1/remoting/protocol/connection_to...
remoting/protocol/connection_to_client.h:116: scoped_ptr<HostMessageDispatcher>
dispatcher_;
On 2010/11/10 20:14:21, sergeyu wrote:
> Does this need to be a pointer?

Trying not to include a bunch of internal / protocol headers here.

http://codereview.chromium.org/4726003/diff/1/remoting/protocol/host_message_...
File remoting/protocol/host_message_dispatcher.cc (right):

http://codereview.chromium.org/4726003/diff/1/remoting/protocol/host_message_...
remoting/protocol/host_message_dispatcher.cc:82:
message->mutable_suggest_resolution(), NewDeleteTask(ref_msg));
On 2010/11/10 20:14:21, sergeyu wrote:
> &message->suggest_resolution()?

That shouldn't make a difference since callee see them as const pointer.

[awong, 2010-11-12 20:25:30]

On Fri, Nov 12, 2010 at 12:08 PM, <hclam@chromium.org> wrote:

>
>
> http://codereview.chromium.org/4726003/diff/1/remoting/host/chromoting_host.h
> File remoting/host/chromoting_host.h (right):
>
>
>
http://codereview.chromium.org/4726003/diff/1/remoting/host/chromoting_host.h...
> remoting/host/chromoting_host.h:66: Capturer* capturer,
> protocol::InputStub* input_stub);
> On 2010/11/10 20:14:21, sergeyu wrote:
>
>> On 2010/11/10 19:36:54, awong wrote:
>> > I feel like the ChromotingHost should actually keep taking
>>
> EventExecutor, and
>
>> > that EventExecutor just implements InputStub.  No?
>>
>
>  How EventExecutor would be different from InputStub? I think the type
>>
> should be
>
>> InputStub, but it should be called event_executor, just to make clear
>>
> its
>
>> purpose.
>>
>
> Getting rid of EventExecutor and shift to use InputStub. Evently we will
> just them as InputStub.
>
>
>
>
http://codereview.chromium.org/4726003/diff/1/remoting/host/chromoting_host.h...
> remoting/host/chromoting_host.h:134: scoped_ptr<protocol::InputStub>
> input_stub_;
> On 2010/11/10 20:14:21, sergeyu wrote:
>
>> event_executor_?
>>
>
> It will be merged with capturer so reference them as input_stub_.
>
>
> http://codereview.chromium.org/4726003/diff/1/remoting/proto/event.proto
> File remoting/proto/event.proto (right):
>
>
>
http://codereview.chromium.org/4726003/diff/1/remoting/proto/event.proto#newc...
> remoting/proto/event.proto:74: optional int32 mouse_width = 3;
> On 2010/11/10 20:14:21, sergeyu wrote:
>
>> What is mouse width/height?
>>
>
> This is removed in the latest patch.
>
>
>
http://codereview.chromium.org/4726003/diff/1/remoting/protocol/connection_to...
>
> File remoting/protocol/connection_to_client.h (right):
>
>
>
http://codereview.chromium.org/4726003/diff/1/remoting/protocol/connection_to...
> remoting/protocol/connection_to_client.h:116:
> scoped_ptr<HostMessageDispatcher> dispatcher_;
> On 2010/11/10 20:14:21, sergeyu wrote:
>
>> Does this need to be a pointer?
>>
>
> Trying not to include a bunch of internal / protocol headers here.
>
>
>
http://codereview.chromium.org/4726003/diff/1/remoting/protocol/host_message_...
>
> File remoting/protocol/host_message_dispatcher.cc (right):
>
>
>
http://codereview.chromium.org/4726003/diff/1/remoting/protocol/host_message_...
>
> remoting/protocol/host_message_dispatcher.cc:82:
> message->mutable_suggest_resolution(), NewDeleteTask(ref_msg));
> On 2010/11/10 20:14:21, sergeyu wrote:
>
>> &message->suggest_resolution()?
>>
>
> That shouldn't make a difference since callee see them as const pointer.


It actually modifies the originating proto.  Mutable will add a field if it
wasn't there previously.

If a field is "unset", calling the non-mutable accessor just returns the
default value, but a "has_field()" later call will still return false.

It's not a big deal, but feels slightly cleaner.

-Albert

[Sergey Ulanov, 2010-11-12 20:30:33]

http://codereview.chromium.org/4726003/diff/1/remoting/protocol/host_message_...
File remoting/protocol/host_message_dispatcher.cc (right):

http://codereview.chromium.org/4726003/diff/1/remoting/protocol/host_message_...
remoting/protocol/host_message_dispatcher.cc:82:
message->mutable_suggest_resolution(), NewDeleteTask(ref_msg));
On 2010/11/12 20:08:01, Alpha wrote:
> On 2010/11/10 20:14:21, sergeyu wrote:
> > &message->suggest_resolution()?
> 
> That shouldn't make a difference since callee see them as const pointer.

Yes, but immutable version should be preferred here because you don't need
mutability.

[Sergey Ulanov, 2010-11-12 20:36:36]

On Fri, Nov 12, 2010 at 12:07 PM, <hclam@chromium.org> wrote:

> MessageLoopProxy is the newer version of using MessageLoop which gives us
> protection during teardown. We still make strong guarantees about thread
> destruction order and that message loop given to InputStub live strictly
> longer
> than the object. However one thing I wasn't sure that made me into using
> MessageLoopProxy is the jingle thread and its lifetime.

Jingle thread is not different from any other thread in how we control it's
lifetime, and it doesn't depend on the the JingleClient since long time ago,
so it shouldn't be a problem.


> I don't think using it
> hurts at all but do give us protection during destruction, it'll be handy
> if we
> want to solve some ugly teardown crashes.

IMO, it is important to be consistent, so it is better to use MessageLoop
everywhere, unless we find that we really need MessageLoopProxy. It will be
easy to swap MessageLoop with MessageLoopProxy in future if necessary.


>
>
> http://codereview.chromium.org/4726003/
>

[Alpha Left Google, 2010-11-12 21:23:35]

In fact what I plan to do for next iteration is to change everything in
SeesionManager and ChromotingContext to MessageLoopProxy.

[awong, 2010-11-12 21:29:32]

I don't believe we need MessageLoopProxy in our code, and am really not 100%
comfortable about buying us that simplification at shutdown.  It really
feels like we're asking to allow ourselves to be lazy with managing our
thread lifetimes.  I can't even see a concrete shutdown scenario in our code
where this would be a problem.  My personal preference would be to just keep
using MessageLoop.

That having been said, there's also no concrete cost to MessageLoopProxy
beyond having an uglier name.  So, if you want to keep the Proxy version, I
won't push anymore than this last response. :)

-Albert


On Fri, Nov 12, 2010 at 1:23 PM, <hclam@chromium.org> wrote:

> In fact what I plan to do for next iteration is to change everything in
> SeesionManager and ChromotingContext to MessageLoopProxy.
>
>
> http://codereview.chromium.org/4726003/
>

[Alpha Left Google, 2010-11-12 22:45:03]

Switched back to MessageLoop and not calling mutable_*.

[Alpha Left Google, 2010-11-12 22:56:40]

http://codereview.chromium.org/4726003/diff/22001/remoting/host/simple_host_p...
File remoting/host/simple_host_process.cc (right):

http://codereview.chromium.org/4726003/diff/22001/remoting/host/simple_host_p...
remoting/host/simple_host_process.cc:126: context.Stop();
On 2010/11/12 01:40:10, awong wrote:
> Don't we want to do an explicit stop in the normal path?

Not sure what this means. But this error code path shuts down threads and
return.

http://codereview.chromium.org/4726003/diff/22001/remoting/protocol/connectio...
File remoting/protocol/connection_to_client.h (right):

http://codereview.chromium.org/4726003/diff/22001/remoting/protocol/connectio...
remoting/protocol/connection_to_client.h:32: public
base::RefCountedThreadSafe<ConnectionToClient> {
On 2010/11/12 01:40:10, awong wrote:
> I thought this should only be indent 4 spaces?

Done.

http://codereview.chromium.org/4726003/diff/22001/remoting/protocol/host_mess...
File remoting/protocol/host_message_dispatcher.cc (right):

http://codereview.chromium.org/4726003/diff/22001/remoting/protocol/host_mess...
remoting/protocol/host_message_dispatcher.cc:82:
message->mutable_suggest_resolution(), NewDeleteTask(ref_msg));
On 2010/11/12 01:40:10, awong wrote:
> why do we need this to be mutable?

Done.

http://codereview.chromium.org/4726003/diff/22001/remoting/protocol/host_mess...
remoting/protocol/host_message_dispatcher.cc:82:
message->mutable_suggest_resolution(), NewDeleteTask(ref_msg));
On 2010/11/12 01:40:10, awong wrote:
> why do we need this to be mutable?

Done.

http://codereview.chromium.org/4726003/diff/22001/remoting/protocol/host_mess...
remoting/protocol/host_message_dispatcher.cc:93:
message->mutable_event(i)->mutable_key(), NewDeleteTask(ref_msg));
On 2010/11/12 01:40:10, awong wrote:
> Why are all these mutable?

Done.

[awong, 2010-11-12 23:29:04]

LGTM w/ small nit and if it compiles. :)

http://codereview.chromium.org/4726003/diff/22001/remoting/host/simple_host_p...
File remoting/host/simple_host_process.cc (right):

http://codereview.chromium.org/4726003/diff/22001/remoting/host/simple_host_p...
remoting/host/simple_host_process.cc:126: context.Stop();
On 2010/11/12 22:56:40, Alpha wrote:
> On 2010/11/12 01:40:10, awong wrote:
> > Don't we want to do an explicit stop in the normal path?
> 
> Not sure what this means. But this error code path shuts down threads and
> return.



Sorry...I just misread the code.  Please ignore my comment.

http://codereview.chromium.org/4726003/diff/54001/remoting/host/event_executo...
File remoting/host/event_executor_win.cc (right):

http://codereview.chromium.org/4726003/diff/54001/remoting/host/event_executo...
remoting/host/event_executor_win.cc:26: if
(!message_loop_->BelongsToCurrentThread()) {
Does this compile?

http://codereview.chromium.org/4726003/diff/54001/remoting/host/event_executo...
File remoting/host/event_executor_win.h (right):

http://codereview.chromium.org/4726003/diff/54001/remoting/host/event_executo...
remoting/host/event_executor_win.h:10: #include "base/task.h"
Alphabetical ordering.

[Sergey Ulanov, 2010-11-12 23:43:20]

LGTM. Thanks

[Sergey Ulanov, 2010-11-12 23:43:20]

LGTM. Thanks