Refactor the client code. Move all x11-related code into X11View and create...

Refactor the client code for the X11 version.
Make ChromotingViews responsible for initializing themselves.
Move all x11-related code into X11View.
Create InputCapturer class manage client input capture.

BUG=none
TEST=ran Win host + X11 client


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=52973

[awong, 2010-07-08 01:46:56]

I'm slightly concerned how this fits in with the pepper API.  Can you sketch out
how this will work in reference to the ppapi?

http://codereview.chromium.org/2861047/diff/29003/1028
File remoting/client/client_config.h (right):

http://codereview.chromium.org/2861047/diff/29003/1028#newcode14
remoting/client/client_config.h:14: class ClientConfig {
This seems like it should be a silly struct. Make it a nested struct in
ChromotingClient named Options or something similar?  This is a pattern that
I've seen in things like Thread.

http://codereview.chromium.org/2861047/diff/29003/1030
File remoting/client/client_context.h (right):

http://codereview.chromium.org/2861047/diff/29003/1030#newcode38
remoting/client/client_context.h:38: base::Thread capture_thread_;
This doesn't make sense for pepper.  We only have one thread that sends events
in....

http://codereview.chromium.org/2861047/diff/29003/1042
File remoting/client/x11_input_capturer.h (right):

http://codereview.chromium.org/2861047/diff/29003/1042#newcode14
remoting/client/x11_input_capturer.h:14: class X11InputCapturer : public
InputCapturer {
I'm slightly concerned about abstracting the InputCapturer.  In particular, I'm
not sure it makes 100% sense given that we'll be receiving our input events on
the pepper main thread via the HandleEvent call on the instance.  How do you
envision this working in relation to that setup?

[Alpha Left Google, 2010-07-08 21:15:32]

http://codereview.chromium.org/2861047/diff/29003/1026
File remoting/client/chromoting_client.h (right):

http://codereview.chromium.org/2861047/diff/29003/1026#newcode30
remoting/client/chromoting_client.h:30: base::WaitableEvent* client_done);
Don't think we need client_done any more because the client is running a mssage
loop or context.

http://codereview.chromium.org/2861047/diff/29003/1029
File remoting/client/client_context.cc (right):

http://codereview.chromium.org/2861047/diff/29003/1029#newcode33
remoting/client/client_context.cc:33:
jingle_thread_.message_loop()->PostTask(FROM_HERE,
I believe you don't need to do this.

http://codereview.chromium.org/2861047/diff/29003/1037
File remoting/client/plugin/chromoting_plugin.cc (right):

http://codereview.chromium.org/2861047/diff/29003/1037#newcode81
remoting/client/plugin/chromoting_plugin.cc:81: /*
Commented out code?

http://codereview.chromium.org/2861047/diff/29003/1039
File remoting/client/simple_client.cc (right):

http://codereview.chromium.org/2861047/diff/29003/1039#newcode136
remoting/client/simple_client.cc:136: client_done.Wait();
Don't need to wait since the client is running on a message loop.

context.Stop() will wait until all activities are stopped.

http://codereview.chromium.org/2861047/diff/29003/1039#newcode139
remoting/client/simple_client.cc:139:
context.jingle_thread()->message_loop()->PostTask(FROM_HERE,
Is this really needed?

[garykac, 2010-07-16 22:44:23]

Merged with Albert's recent checkin.

The Win-host/X11-client code was all working fine last week, but it looks like
some of the host stuff has broken recently, so I can't double check after this
merge.

Also, I haven't yet verified that these changes don't mess up the Pepper plugin.
I can't do that until the host is working again.

http://codereview.chromium.org/2861047/diff/29003/1026
File remoting/client/chromoting_client.h (right):

http://codereview.chromium.org/2861047/diff/29003/1026#newcode30
remoting/client/chromoting_client.h:30: base::WaitableEvent* client_done);
On 2010/07/08 21:15:32, Alpha wrote:
> Don't think we need client_done any more because the client is running a
mssage
> loop or context.

Replaced with message_loop callback.

http://codereview.chromium.org/2861047/diff/29003/1028
File remoting/client/client_config.h (right):

http://codereview.chromium.org/2861047/diff/29003/1028#newcode14
remoting/client/client_config.h:14: class ClientConfig {
On 2010/07/08 01:46:56, awong wrote:
> This seems like it should be a silly struct. Make it a nested struct in
> ChromotingClient named Options or something similar?  This is a pattern that
> I've seen in things like Thread.

Having a separate class parallels HostConfig, and I believe that it's likely
that this will grow as we add more client auth.

http://codereview.chromium.org/2861047/diff/29003/1029
File remoting/client/client_context.cc (right):

http://codereview.chromium.org/2861047/diff/29003/1029#newcode33
remoting/client/client_context.cc:33:
jingle_thread_.message_loop()->PostTask(FROM_HERE,
On 2010/07/08 21:15:32, Alpha wrote:
> I believe you don't need to do this.

Done.

http://codereview.chromium.org/2861047/diff/29003/1030
File remoting/client/client_context.h (right):

http://codereview.chromium.org/2861047/diff/29003/1030#newcode38
remoting/client/client_context.h:38: base::Thread capture_thread_;
On 2010/07/08 01:46:56, awong wrote:
> This doesn't make sense for pepper.  We only have one thread that sends events
> in....

True. Input processing shouldn't be time-consuming so it can remain on the main
Pepper thread.

http://codereview.chromium.org/2861047/diff/29003/1037
File remoting/client/plugin/chromoting_plugin.cc (right):

http://codereview.chromium.org/2861047/diff/29003/1037#newcode81
remoting/client/plugin/chromoting_plugin.cc:81: /*
On 2010/07/08 21:15:32, Alpha wrote:
> Commented out code?

Done.

http://codereview.chromium.org/2861047/diff/29003/1039
File remoting/client/simple_client.cc (right):

http://codereview.chromium.org/2861047/diff/29003/1039#newcode136
remoting/client/simple_client.cc:136: client_done.Wait();
On 2010/07/08 21:15:32, Alpha wrote:
> Don't need to wait since the client is running on a message loop.
> 
> context.Stop() will wait until all activities are stopped.

Removing simple_client.cc

http://codereview.chromium.org/2861047/diff/29003/1039#newcode139
remoting/client/simple_client.cc:139:
context.jingle_thread()->message_loop()->PostTask(FROM_HERE,
On 2010/07/08 21:15:32, Alpha wrote:
> Is this really needed?

Removing simple_client.cc

http://codereview.chromium.org/2861047/diff/29003/1042
File remoting/client/x11_input_capturer.h (right):

http://codereview.chromium.org/2861047/diff/29003/1042#newcode14
remoting/client/x11_input_capturer.h:14: class X11InputCapturer : public
InputCapturer {
On 2010/07/08 01:46:56, awong wrote:
> I'm slightly concerned about abstracting the InputCapturer.  In particular,
I'm
> not sure it makes 100% sense given that we'll be receiving our input events on
> the pepper main thread via the HandleEvent call on the instance.  How do you
> envision this working in relation to that setup?

Even if this is on the main Pepper thread, I think encapsulating this into a
class makes sense.

For X11, the X11 input handler needs the X11 view, and having a subclass allows
us to hide that.

[garykac, 2010-07-16 22:57:50]

OK. Try again...

[awong, 2010-07-17 01:27:19]

Looks okay as a first pass.  There's some cleanup we need to do to the codebase
as a whole, but I think let's take that as pass-2.

http://codereview.chromium.org/2861047/diff/55034/110004
File remoting/client/chromoting_client.cc (right):

http://codereview.chromium.org/2861047/diff/55034/110004#newcode38
remoting/client/chromoting_client.cc:38: void ChromotingClient::Start() {
Can we add a TODO to merge all the various Do* methods into something similar to
what I did in PepperView?

http://codereview.chromium.org/2861047/diff/55034/110004#newcode39
remoting/client/chromoting_client.cc:39: context_->Start();
Feels to me like the context should be started/stopped external to this class...

http://codereview.chromium.org/2861047/diff/55034/110004#newcode47
remoting/client/chromoting_client.cc:47: connection_->Disconnect();
Why is that End() instead of Stop()?

http://codereview.chromium.org/2861047/diff/55034/110005
File remoting/client/chromoting_client.h (right):

http://codereview.chromium.org/2861047/diff/55034/110005#newcode24
remoting/client/chromoting_client.h:24: ChromotingClient(ClientConfig* config,
Please add either class or function level comment specifying ownership of the
objects passed around.

http://codereview.chromium.org/2861047/diff/55034/110014
File remoting/client/input_capturer.h (right):

http://codereview.chromium.org/2861047/diff/55034/110014#newcode13
remoting/client/input_capturer.h:13: class InputCapturer {
Let's call this something other than Capturer.  The host already has a Capturer.

Maybe InputHandler? InputProcessor? InputDispatcher? UserEventStream?  Just not
Capturer...

http://codereview.chromium.org/2861047/diff/55034/110014#newcode25
remoting/client/input_capturer.h:25: ClientContext* context_;
If this is going to be an interface, let's make it a pure one, and not use it to
share data members.  Move ClientContext* and ChromotingView* down into each
derived class?

Sure, it's some code duplication, but inheritance to share variables sucks.

http://codereview.chromium.org/2861047/diff/55034/110016
File remoting/client/jingle_host_connection.h (right):

http://codereview.chromium.org/2861047/diff/55034/110016#newcode34
remoting/client/jingle_host_connection.h:34: class JingleThread;
Alphabetical order.

http://codereview.chromium.org/2861047/diff/55034/110017
File remoting/client/plugin/chromoting_plugin.cc (right):

http://codereview.chromium.org/2861047/diff/55034/110017#newcode88
remoting/client/plugin/chromoting_plugin.cc:88: context_.get(),
Yeah, let's call Start() on the context externally.

http://codereview.chromium.org/2861047/diff/55034/110021
File remoting/client/plugin/pepper_input_capturer.h (right):

http://codereview.chromium.org/2861047/diff/55034/110021#newcode24
remoting/client/plugin/pepper_input_capturer.h:24: //void DoProcessX11Events();
Remove these comments?

http://codereview.chromium.org/2861047/diff/55034/110031
File remoting/jingle_glue/jingle_test_client.cc (right):

http://codereview.chromium.org/2861047/diff/55034/110031#newcode25
remoting/jingle_glue/jingle_test_client.cc:25: using remoting::JingleThread;
Do we need this?

[garykac, 2010-07-17 02:37:30]

http://codereview.chromium.org/2861047/diff/55034/110004
File remoting/client/chromoting_client.cc (right):

http://codereview.chromium.org/2861047/diff/55034/110004#newcode39
remoting/client/chromoting_client.cc:39: context_->Start();
On 2010/07/17 01:27:19, awong wrote:
> Feels to me like the context should be started/stopped external to this
class...

Done.

http://codereview.chromium.org/2861047/diff/55034/110004#newcode47
remoting/client/chromoting_client.cc:47: connection_->Disconnect();
On 2010/07/17 01:27:19, awong wrote:
> Why is that End() instead of Stop()?

Done.

http://codereview.chromium.org/2861047/diff/55034/110005
File remoting/client/chromoting_client.h (right):

http://codereview.chromium.org/2861047/diff/55034/110005#newcode24
remoting/client/chromoting_client.h:24: ChromotingClient(ClientConfig* config,
On 2010/07/17 01:27:19, awong wrote:
> Please add either class or function level comment specifying ownership of the
> objects passed around.

Done.

http://codereview.chromium.org/2861047/diff/55034/110014
File remoting/client/input_capturer.h (right):

http://codereview.chromium.org/2861047/diff/55034/110014#newcode13
remoting/client/input_capturer.h:13: class InputCapturer {
On 2010/07/17 01:27:19, awong wrote:
> Let's call this something other than Capturer.  The host already has a
Capturer.
> 
> Maybe InputHandler? InputProcessor? InputDispatcher? UserEventStream?  Just
not
> Capturer...

Done.

http://codereview.chromium.org/2861047/diff/55034/110014#newcode25
remoting/client/input_capturer.h:25: ClientContext* context_;
On 2010/07/17 01:27:19, awong wrote:
> If this is going to be an interface, let's make it a pure one, and not use it
to
> share data members.  Move ClientContext* and ChromotingView* down into each
> derived class?
> 
> Sure, it's some code duplication, but inheritance to share variables sucks.

Done.

http://codereview.chromium.org/2861047/diff/55034/110016
File remoting/client/jingle_host_connection.h (right):

http://codereview.chromium.org/2861047/diff/55034/110016#newcode34
remoting/client/jingle_host_connection.h:34: class JingleThread;
On 2010/07/17 01:27:19, awong wrote:
> Alphabetical order.

Done.

http://codereview.chromium.org/2861047/diff/55034/110017
File remoting/client/plugin/chromoting_plugin.cc (right):

http://codereview.chromium.org/2861047/diff/55034/110017#newcode88
remoting/client/plugin/chromoting_plugin.cc:88: context_.get(),
On 2010/07/17 01:27:19, awong wrote:
> Yeah, let's call Start() on the context externally.

Done.

http://codereview.chromium.org/2861047/diff/55034/110021
File remoting/client/plugin/pepper_input_capturer.h (right):

http://codereview.chromium.org/2861047/diff/55034/110021#newcode24
remoting/client/plugin/pepper_input_capturer.h:24: //void DoProcessX11Events();
On 2010/07/17 01:27:19, awong wrote:
> Remove these comments?

Done.

http://codereview.chromium.org/2861047/diff/55034/110031
File remoting/jingle_glue/jingle_test_client.cc (right):

http://codereview.chromium.org/2861047/diff/55034/110031#newcode25
remoting/jingle_glue/jingle_test_client.cc:25: using remoting::JingleThread;
On 2010/07/17 01:27:19, awong wrote:
> Do we need this?

Done.

[awong, 2010-07-19 21:50:10]

lgtm

http://codereview.chromium.org/2861047/diff/47004/140002
File remoting/client/chromoting_client.cc (right):

http://codereview.chromium.org/2861047/diff/47004/140002#newcode51
remoting/client/chromoting_client.cc:51: 
remove extra newline.