Implement a chromoting client using X11

Implement a chromoting client using X11

Using XRender to render the chromoting client. This patch has done several things:
1. Rename chromotocol_pb to remoting
2. Defined ChromotingView as the display area of the remote view
3. Implemented X11Client as the client that uses X11 for display
4. Implemented X11View that uses XRender for drawing
5. Fixed several problems in host capturer and encoder

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

[Alpha Left Google, 2010-06-09 02:52:41]

Sorry for such a big patch. I mixed the renaming of namespace with this patch
which is hard to separate.. :(

There are several important files to look:
- remoting/client/chromoting_view.h
- remoting/client/x11_client.cc
- remoting/client/x11_view.cc
- remoting/client/x11_view.h

X11View implements ChromotingView and should show a ChromotingView is used and
how it interacts with the decoder.

[Sergey Ulanov, 2010-06-09 05:18:57]

http://codereview.chromium.org/2745006/diff/5001/6015
File remoting/client/x11_client.cc (right):

http://codereview.chromium.org/2745006/diff/5001/6015#newcode249
remoting/client/x11_client.cc:249: std::string password_;
Just FYI: I have a pending change for token auth:
http://codereview.chromium.org/2749004/show , this will need to be replaced with
token_auth_.

[awong, 2010-06-09 18:43:52]

This CL is too large (conflates a rename + new functionality). (-_-)

Overall, seems okay, but I have some questions about the X11 factoring, and it's
missing tests.  However, given the length, I'd prefer we checkin with TODOs on
larger issues and fix it in the next CL.

http://codereview.chromium.org/2745006/diff/5001/6001
File media/base/video_frame.cc (right):

http://codereview.chromium.org/2745006/diff/5001/6001#newcode60
media/base/video_frame.cc:60: new VideoFrame(VideoFrame::TYPE_SYSTEM_MEMORY,
format, width, height);
don't you need to set external_memory to true somewhere?

Alternately, do we want to just add another memory type?  Like TYPE_NOTOWNED or
TYPE_EXTERNAL_MEMORY?

http://codereview.chromium.org/2745006/diff/5001/6007
File remoting/chromoting.gyp (right):

http://codereview.chromium.org/2745006/diff/5001/6007#newcode258
remoting/chromoting.gyp:258: },  # end of target 'chromoting_x11_client'
Do we really want all of these end of target comments? Just checking.

http://codereview.chromium.org/2745006/diff/5001/6008
File remoting/client/chromoting_view.h (right):

http://codereview.chromium.org/2745006/diff/5001/6008#newcode9
remoting/client/chromoting_view.h:9: #include
"remoting/base/protocol/chromotocol.pb.h"
Prefer forward declaration to #include.  I thinkyou just need to forward declare
HostMessage right?

http://codereview.chromium.org/2745006/diff/5001/6008#newcode13
remoting/client/chromoting_view.h:13: // ChromotingView defines the behavior an
object that draw the remote desktop
behavior an -> behavior of an

http://codereview.chromium.org/2745006/diff/5001/6008#newcode14
remoting/client/chromoting_view.h:14: // view. It's main function is to choose
the right decoder and render the
"draw the remote desktop view" -> "draw a view of the remote desktop"

http://codereview.chromium.org/2745006/diff/5001/6008#newcode15
remoting/client/chromoting_view.h:15: // update stream on the screen.
on the -> onto the

http://codereview.chromium.org/2745006/diff/5001/6008#newcode22
remoting/client/chromoting_view.h:22: class Client {
Client is a super generic name.  Especially since we're in a directory called
"Client".  Come up with something more specific?

http://codereview.chromium.org/2745006/diff/5001/6009
File remoting/client/decoder.h (right):

http://codereview.chromium.org/2745006/diff/5001/6009#newcode52
remoting/client/decoder.h:52: Task* decode_done,
Stuff the callbacks at the end of the function call. Done callback last. More
standard..

http://codereview.chromium.org/2745006/diff/5001/6010
File remoting/client/decoder_verbatim.cc (right):

http://codereview.chromium.org/2745006/diff/5001/6010#newcode70
remoting/client/decoder_verbatim.cc:70: updated_rects_->clear();
BTW, random thought...should we be using vectors for these things or lists?

http://codereview.chromium.org/2745006/diff/5001/6015
File remoting/client/x11_client.cc (right):

http://codereview.chromium.org/2745006/diff/5001/6015#newcode15
remoting/client/x11_client.cc:15: // Include Xlib at the end because it clashes
with ClientMessage defined in
Should we rename ClientMessage?

http://codereview.chromium.org/2745006/diff/5001/6015#newcode22
remoting/client/x11_client.cc:22: 
remove extra space.

http://codereview.chromium.org/2745006/diff/5001/6015#newcode47
remoting/client/x11_client.cc:47: main_loop_.PostTask(FROM_HERE,
Call this thing message_loop_. It's called that everywhere else in media code.

http://codereview.chromium.org/2745006/diff/5001/6015#newcode184
remoting/client/x11_client.cc:184: view_ = new X11View(display_, window_, this,
width_, height_);
We're starting to have a lot of classes that have circular communication.

X11Client owns and X11View that signals X11Client.  Is there any way to simplify
this?

http://codereview.chromium.org/2745006/diff/5001/6015#newcode192
remoting/client/x11_client.cc:192: DCHECK_EQ(&main_loop_,
MessageLoop::current());
put the expected value first.

http://codereview.chromium.org/2745006/diff/5001/6015#newcode235
remoting/client/x11_client.cc:235: NewRunnableMethod(this,
&X11Client::X11EventHandler), 10);
Pull 10 out into a constant.

http://codereview.chromium.org/2745006/diff/5001/6016
File remoting/client/x11_view.cc (right):

http://codereview.chromium.org/2745006/diff/5001/6016#newcode140
remoting/client/x11_view.cc:140: client_->SchedulePaint();
I don't completely understand this interaction.  Why is client and view
separate?

http://codereview.chromium.org/2745006/diff/5001/6017
File remoting/client/x11_view.h (right):

http://codereview.chromium.org/2745006/diff/5001/6017#newcode45
remoting/client/x11_view.h:45: unsigned long picture_;
Is unsigned long the correct type?

http://codereview.chromium.org/2745006/diff/5001/6025
File remoting/host/client_connection_unittest.cc (right):

http://codereview.chromium.org/2745006/diff/5001/6025#newcode55
remoting/host/client_connection_unittest.cc:55: = new
UpdateStreamPacketHeader();
break after the equal sign, not before.

http://codereview.chromium.org/2745006/diff/5001/6031
File remoting/host/encoder_vp8_unittest.cc (right):

http://codereview.chromium.org/2745006/diff/5001/6031#newcode51
remoting/host/encoder_vp8_unittest.cc:51: = new UpdateStreamPacketHeader();
break after = sign.

[garykac, 2010-06-09 18:51:30]

Missing tests for new code.

http://codereview.chromium.org/2745006/diff/5001/6002
File media/base/video_frame.h (right):

http://codereview.chromium.org/2745006/diff/5001/6002#newcode142
media/base/video_frame.h:142: // True of memory referenced by |data_| is
provided externally and shouldn't
True *if*

http://codereview.chromium.org/2745006/diff/5001/6007
File remoting/chromoting.gyp (right):

http://codereview.chromium.org/2745006/diff/5001/6007#newcode194
remoting/chromoting.gyp:194: 'client/client_util.cc',
New file?
I don't see it in this CL.  Also, it would need unittests.

http://codereview.chromium.org/2745006/diff/5001/6008
File remoting/client/chromoting_view.h (right):

http://codereview.chromium.org/2745006/diff/5001/6008#newcode13
remoting/client/chromoting_view.h:13: // ChromotingView defines the behavior an
object that draw the remote desktop
... the behavior *of* an object that *draws* the ...

http://codereview.chromium.org/2745006/diff/5001/6008#newcode14
remoting/client/chromoting_view.h:14: // view. It's main function is to choose
the right decoder and render the
*Its*

http://codereview.chromium.org/2745006/diff/5001/6009
File remoting/client/decoder.h (right):

http://codereview.chromium.org/2745006/diff/5001/6009#newcode18
remoting/client/decoder.h:18: typedef std::vector<gfx::Rect> UpdatedRects;
does it make sense to share this with the DirtyRects definition used in the
encoder/differ?

http://codereview.chromium.org/2745006/diff/5001/6013
File remoting/client/host_connection.h (right):

http://codereview.chromium.org/2745006/diff/5001/6013#newcode73
remoting/client/host_connection.h:73: Lock lock_;
Perhaps jingle_lock_ is a more descriptive name.

[awong, 2010-06-09 18:57:50]

Check the * associations also in your diffs.

http://codereview.chromium.org/2745006/diff/5001/6017
File remoting/client/x11_view.h (right):

http://codereview.chromium.org/2745006/diff/5001/6017#newcode30
remoting/client/x11_view.h:30: virtual void HandleBeginUpdateStream(HostMessage
*msg);
* is in the wrong spot.

[scherkus (not reviewing), 2010-06-09 19:05:14]

general comment about function naming...

In this CL I saw a few different variants for callbacks/tasks/interfaces:

FooTask
HandleFoo
FooHandler
OnFoo
ScheduleFoo

You guys should decide on something a bit more consistent (interfaces use OnFoo,
tasks/callbacks use DoFoo, etc...)

http://codereview.chromium.org/2745006/diff/5001/6001
File media/base/video_frame.cc (right):

http://codereview.chromium.org/2745006/diff/5001/6001#newcode60
media/base/video_frame.cc:60: new VideoFrame(VideoFrame::TYPE_SYSTEM_MEMORY,
format, width, height);
On 2010/06/09 18:43:53, awong wrote:
> don't you need to set external_memory to true somewhere?
> 
> Alternately, do we want to just add another memory type?  Like TYPE_NOTOWNED
or
> TYPE_EXTERNAL_MEMORY?

Alpha and I chatted about this and I argued that type dictates how the user
interacts with this object.  The user doesn't care how its allocated, just that
TYPE_SYSTEM_MEMORY is true and therefore the object acts the same way.  Even if
we don't have code today that checks for TYPE_SYSTEM_MEMORY it would be
unfortunate to have to check for both TYPE_SYSTEM_MEMORY and TYPE_NOTOWNED in
order to know that the data pointer is system memory, etc..

http://codereview.chromium.org/2745006/diff/5001/6002
File media/base/video_frame.h (right):

http://codereview.chromium.org/2745006/diff/5001/6002#newcode58
media/base/video_frame.h:58: static void CreateFrameExternal(Format format,
unit test please

this ideally should have been done as a separate CL :)

http://codereview.chromium.org/2745006/diff/5001/6007
File remoting/chromoting.gyp (right):

http://codereview.chromium.org/2745006/diff/5001/6007#newcode258
remoting/chromoting.gyp:258: },  # end of target 'chromoting_x11_client'
On 2010/06/09 18:43:53, awong wrote:
> Do we really want all of these end of target comments? Just checking.

I agree its a bit of noise but to each their own!

http://codereview.chromium.org/2745006/diff/5001/6008
File remoting/client/chromoting_view.h (right):

http://codereview.chromium.org/2745006/diff/5001/6008#newcode16
remoting/client/chromoting_view.h:16: class ChromotingView : public
base::RefCountedThreadSafe<ChromotingView> {
sanity check: does this need to be ref counted?

http://codereview.chromium.org/2745006/diff/5001/6008#newcode38
remoting/client/chromoting_view.h:38: virtual void
HandleBeginUpdateStream(HostMessage *msg) = 0;
based on your .cc file pointer goes with type x3

http://codereview.chromium.org/2745006/diff/5001/6013
File remoting/client/host_connection.h (right):

http://codereview.chromium.org/2745006/diff/5001/6013#newcode73
remoting/client/host_connection.h:73: Lock lock_;
you added this lock but its not being used in .cc

http://codereview.chromium.org/2745006/diff/5001/6015
File remoting/client/x11_client.cc (right):

http://codereview.chromium.org/2745006/diff/5001/6015#newcode39
remoting/client/x11_client.cc:39: ~X11Client() {
virtual

http://codereview.chromium.org/2745006/diff/5001/6015#newcode221
remoting/client/x11_client.cc:221: // TODO(hclam): Implement.
add NOTIMPLEMENTED

http://codereview.chromium.org/2745006/diff/5001/6015#newcode223
remoting/client/x11_client.cc:223: // TODO(hclam): Implement.
add NOTIMPLEMENTED

http://codereview.chromium.org/2745006/diff/5001/6017
File remoting/client/x11_view.h (right):

http://codereview.chromium.org/2745006/diff/5001/6017#newcode16
remoting/client/x11_view.h:16: #include <X11/Xlib.h>
please try to avoid including massive system headers in .h files

if you look at chrome x11/gtk headers you'll notice they forward declare what
they need:

http://src.chromium.org/viewvc/chrome/trunk/src/app/x11_util.h

[Alpha Left Google, 2010-06-09 19:53:18]

Fixed several things in the latest upload:
1. ChromotingView doesn't need a view now, it signals for repaint itself
2. Added unit test for VideoFrame
3. Rename methods to be more coherent.

Now all tasks posted within the same class is DoXXX
All event handlers are OnXXX
Done tasks given to external object are XXXDone

There's nothing called XXXTask now.

As for unit test, i will add unit test for the following classes:
1. DecoderVerbatim
2. HostConnection
3. CapturerFake

Don't think we need to test the X11 part.

http://codereview.chromium.org/2745006/diff/5001/6001
File media/base/video_frame.cc (right):

http://codereview.chromium.org/2745006/diff/5001/6001#newcode60
media/base/video_frame.cc:60: new VideoFrame(VideoFrame::TYPE_SYSTEM_MEMORY,
format, width, height);
On 2010/06/09 18:43:53, awong wrote:
> don't you need to set external_memory to true somewhere?
> 
> Alternately, do we want to just add another memory type?  Like TYPE_NOTOWNED
or
> TYPE_EXTERNAL_MEMORY?

If we create a new type user may have to do a switch sometime in the future and
may break stuff.

http://codereview.chromium.org/2745006/diff/5001/6001#newcode60
media/base/video_frame.cc:60: new VideoFrame(VideoFrame::TYPE_SYSTEM_MEMORY,
format, width, height);
A unit test is added.

http://codereview.chromium.org/2745006/diff/5001/6007
File remoting/chromoting.gyp (right):

http://codereview.chromium.org/2745006/diff/5001/6007#newcode194
remoting/chromoting.gyp:194: 'client/client_util.cc',
On 2010/06/09 18:51:30, garykac wrote:
> New file?
> I don't see it in this CL.  Also, it would need unittests.

Added the file, that is extracted from simple_client.cc just to get user input
for testing purpose.

http://codereview.chromium.org/2745006/diff/5001/6007#newcode258
remoting/chromoting.gyp:258: },  # end of target 'chromoting_x11_client'
On 2010/06/09 19:05:15, scherkus wrote:
> On 2010/06/09 18:43:53, awong wrote:
> > Do we really want all of these end of target comments? Just checking.
> 
> I agree its a bit of noise but to each their own!
> 

I actually think this is nice as gyp can be hard to read.

http://codereview.chromium.org/2745006/diff/5001/6008
File remoting/client/chromoting_view.h (right):

http://codereview.chromium.org/2745006/diff/5001/6008#newcode9
remoting/client/chromoting_view.h:9: #include
"remoting/base/protocol/chromotocol.pb.h"
On 2010/06/09 18:43:53, awong wrote:
> Prefer forward declaration to #include.  I thinkyou just need to forward
declare
> HostMessage right?

Done.

http://codereview.chromium.org/2745006/diff/5001/6008#newcode13
remoting/client/chromoting_view.h:13: // ChromotingView defines the behavior an
object that draw the remote desktop
On 2010/06/09 18:43:53, awong wrote:
> behavior an -> behavior of an

Done.

http://codereview.chromium.org/2745006/diff/5001/6008#newcode14
remoting/client/chromoting_view.h:14: // view. It's main function is to choose
the right decoder and render the
On 2010/06/09 18:43:53, awong wrote:
> "draw the remote desktop view" -> "draw a view of the remote desktop"

Done.

http://codereview.chromium.org/2745006/diff/5001/6008#newcode15
remoting/client/chromoting_view.h:15: // update stream on the screen.
On 2010/06/09 18:43:53, awong wrote:
> on the -> onto the

Done.

http://codereview.chromium.org/2745006/diff/5001/6008#newcode16
remoting/client/chromoting_view.h:16: class ChromotingView : public
base::RefCountedThreadSafe<ChromotingView> {
On 2010/06/09 19:05:15, scherkus wrote:
> sanity check: does this need to be ref counted?

We create tasks to the decoder to do asynchronous decode so this is needed.

http://codereview.chromium.org/2745006/diff/5001/6008#newcode22
remoting/client/chromoting_view.h:22: class Client {
On 2010/06/09 18:43:53, awong wrote:
> Client is a super generic name.  Especially since we're in a directory called
> "Client".  Come up with something more specific?

ChromotingViewClient?

http://codereview.chromium.org/2745006/diff/5001/6008#newcode38
remoting/client/chromoting_view.h:38: virtual void
HandleBeginUpdateStream(HostMessage *msg) = 0;
On 2010/06/09 19:05:15, scherkus wrote:
> based on your .cc file pointer goes with type x3

Done.

http://codereview.chromium.org/2745006/diff/5001/6009
File remoting/client/decoder.h (right):

http://codereview.chromium.org/2745006/diff/5001/6009#newcode18
remoting/client/decoder.h:18: typedef std::vector<gfx::Rect> UpdatedRects;
On 2010/06/09 18:51:30, garykac wrote:
> does it make sense to share this with the DirtyRects definition used in the
> encoder/differ?

They are in a difference though. Maybe refactor later to pull them into base/.

http://codereview.chromium.org/2745006/diff/5001/6009#newcode52
remoting/client/decoder.h:52: Task* decode_done,
On 2010/06/09 18:43:53, awong wrote:
> Stuff the callbacks at the end of the function call. Done callback last. More
> standard..

Done.

http://codereview.chromium.org/2745006/diff/5001/6010
File remoting/client/decoder_verbatim.cc (right):

http://codereview.chromium.org/2745006/diff/5001/6010#newcode70
remoting/client/decoder_verbatim.cc:70: updated_rects_->clear();
On 2010/06/09 18:43:53, awong wrote:
> BTW, random thought...should we be using vectors for these things or lists?

I don't think they make much difference in runtime performance though. Just not
a fan of list because of more weight (forward pointers).

http://codereview.chromium.org/2745006/diff/5001/6013
File remoting/client/host_connection.h (right):

http://codereview.chromium.org/2745006/diff/5001/6013#newcode73
remoting/client/host_connection.h:73: Lock lock_;
On 2010/06/09 19:05:15, scherkus wrote:
> you added this lock but its not being used in .cc

Right, removed.

http://codereview.chromium.org/2745006/diff/5001/6015
File remoting/client/x11_client.cc (right):

http://codereview.chromium.org/2745006/diff/5001/6015#newcode15
remoting/client/x11_client.cc:15: // Include Xlib at the end because it clashes
with ClientMessage defined in
On 2010/06/09 18:43:53, awong wrote:
> Should we rename ClientMessage?

Don't think we need to rename the protobuf because of this test app.

http://codereview.chromium.org/2745006/diff/5001/6015#newcode22
remoting/client/x11_client.cc:22: 
On 2010/06/09 18:43:53, awong wrote:
> remove extra space.

Done.

http://codereview.chromium.org/2745006/diff/5001/6015#newcode22
remoting/client/x11_client.cc:22: 
On 2010/06/09 18:43:53, awong wrote:
> remove extra space.

Done.

http://codereview.chromium.org/2745006/diff/5001/6015#newcode39
remoting/client/x11_client.cc:39: ~X11Client() {
On 2010/06/09 19:05:15, scherkus wrote:
> virtual

Done.

http://codereview.chromium.org/2745006/diff/5001/6015#newcode47
remoting/client/x11_client.cc:47: main_loop_.PostTask(FROM_HERE,
On 2010/06/09 18:43:53, awong wrote:
> Call this thing message_loop_. It's called that everywhere else in media code.

Done.

http://codereview.chromium.org/2745006/diff/5001/6015#newcode184
remoting/client/x11_client.cc:184: view_ = new X11View(display_, window_, this,
width_, height_);
On 2010/06/09 18:43:53, awong wrote:
> We're starting to have a lot of classes that have circular communication.
> 
> X11Client owns and X11View that signals X11Client.  Is there any way to
simplify
> this?

In this particular case we don't necessary need the circular communication as
X11View can signal an repaint event itself. Not sure about pepper but I'll do
that first.

http://codereview.chromium.org/2745006/diff/5001/6015#newcode192
remoting/client/x11_client.cc:192: DCHECK_EQ(&main_loop_,
MessageLoop::current());
On 2010/06/09 18:43:53, awong wrote:
> put the expected value first.

Actually main_loop_ is the expected value.

http://codereview.chromium.org/2745006/diff/5001/6015#newcode221
remoting/client/x11_client.cc:221: // TODO(hclam): Implement.
On 2010/06/09 19:05:15, scherkus wrote:
> add NOTIMPLEMENTED

Done.

http://codereview.chromium.org/2745006/diff/5001/6015#newcode223
remoting/client/x11_client.cc:223: // TODO(hclam): Implement.
On 2010/06/09 19:05:15, scherkus wrote:
> add NOTIMPLEMENTED

Done.

http://codereview.chromium.org/2745006/diff/5001/6015#newcode235
remoting/client/x11_client.cc:235: NewRunnableMethod(this,
&X11Client::X11EventHandler), 10);
On 2010/06/09 18:43:53, awong wrote:
> Pull 10 out into a constant.

Done.

http://codereview.chromium.org/2745006/diff/5001/6015#newcode249
remoting/client/x11_client.cc:249: std::string password_;
On 2010/06/09 05:18:58, sergeyu wrote:
> Just FYI: I have a pending change for token auth:
> http://codereview.chromium.org/2749004/show , this will need to be replaced
with
> token_auth_.
> 

Done.

http://codereview.chromium.org/2745006/diff/5001/6016
File remoting/client/x11_view.cc (right):

http://codereview.chromium.org/2745006/diff/5001/6016#newcode140
remoting/client/x11_view.cc:140: client_->SchedulePaint();
On 2010/06/09 18:43:53, awong wrote:
> I don't completely understand this interaction.  Why is client and view
> separate?

Merged together now.

http://codereview.chromium.org/2745006/diff/5001/6017
File remoting/client/x11_view.h (right):

http://codereview.chromium.org/2745006/diff/5001/6017#newcode16
remoting/client/x11_view.h:16: #include <X11/Xlib.h>
On 2010/06/09 19:05:15, scherkus wrote:
> please try to avoid including massive system headers in .h files
> 
> if you look at chrome x11/gtk headers you'll notice they forward declare what
> they need:
> 
> http://src.chromium.org/viewvc/chrome/trunk/src/app/x11_util.h

Done.

http://codereview.chromium.org/2745006/diff/5001/6017#newcode30
remoting/client/x11_view.h:30: virtual void HandleBeginUpdateStream(HostMessage
*msg);
On 2010/06/09 18:57:52, awong wrote:
> * is in the wrong spot.

Done.

http://codereview.chromium.org/2745006/diff/5001/6017#newcode45
remoting/client/x11_view.h:45: unsigned long picture_;
On 2010/06/09 18:43:53, awong wrote:
> Is unsigned long the correct type?

This is how I used it in other place but I can change it to int.

http://codereview.chromium.org/2745006/diff/5001/6025
File remoting/host/client_connection_unittest.cc (right):

http://codereview.chromium.org/2745006/diff/5001/6025#newcode55
remoting/host/client_connection_unittest.cc:55: = new
UpdateStreamPacketHeader();
On 2010/06/09 18:43:53, awong wrote:
> break after the equal sign, not before.

Done.

http://codereview.chromium.org/2745006/diff/5001/6031
File remoting/host/encoder_vp8_unittest.cc (right):

http://codereview.chromium.org/2745006/diff/5001/6031#newcode51
remoting/host/encoder_vp8_unittest.cc:51: = new UpdateStreamPacketHeader();
On 2010/06/09 18:43:53, awong wrote:
> break after = sign.

Done.

[awong, 2010-06-09 20:36:11]

LGTM

http://codereview.chromium.org/2745006/diff/5001/6008
File remoting/client/chromoting_view.h (right):

http://codereview.chromium.org/2745006/diff/5001/6008#newcode16
remoting/client/chromoting_view.h:16: class ChromotingView : public
base::RefCountedThreadSafe<ChromotingView> {
On 2010/06/09 19:53:18, Alpha wrote:
> On 2010/06/09 19:05:15, scherkus wrote:
> > sanity check: does this need to be ref counted?
> 
> We create tasks to the decoder to do asynchronous decode so this is needed.
> 

Grrr...we need to add an alternative to NewRunnableMethod into task.h.  Maybe
NewRunnableMethodNoRef or something.  Adding refcounts to use message loops you
know will outlive an object is just annoying.

http://codereview.chromium.org/2745006/diff/5001/6009
File remoting/client/decoder.h (right):

http://codereview.chromium.org/2745006/diff/5001/6009#newcode18
remoting/client/decoder.h:18: typedef std::vector<gfx::Rect> UpdatedRects;
On 2010/06/09 19:53:18, Alpha wrote:
> On 2010/06/09 18:51:30, garykac wrote:
> > does it make sense to share this with the DirtyRects definition used in the
> > encoder/differ?
> 
> They are in a difference though. Maybe refactor later to pull them into base/.

What's the difference?  Why not just call the thing RectList?

Add a TODO?

http://codereview.chromium.org/2745006/diff/5001/6010
File remoting/client/decoder_verbatim.cc (right):

http://codereview.chromium.org/2745006/diff/5001/6010#newcode70
remoting/client/decoder_verbatim.cc:70: updated_rects_->clear();
On 2010/06/09 19:53:18, Alpha wrote:
> On 2010/06/09 18:43:53, awong wrote:
> > BTW, random thought...should we be using vectors for these things or lists?
> 
> I don't think they make much difference in runtime performance though. Just
not
> a fan of list because of more weight (forward pointers).

On the otherhand, vectors double on a resize so they're not necessary more
efficient...and I don't think clear resizes the vector downwards.  So for a
long-lived vectors, we basically end up with max memory usage...

Anyways, just something to think about.  I doubt it matters much.

http://codereview.chromium.org/2745006/diff/5001/6015
File remoting/client/x11_client.cc (right):

http://codereview.chromium.org/2745006/diff/5001/6015#newcode192
remoting/client/x11_client.cc:192: DCHECK_EQ(&main_loop_,
MessageLoop::current());
On 2010/06/09 19:53:18, Alpha wrote:
> On 2010/06/09 18:43:53, awong wrote:
> > put the expected value first.
> 
> Actually main_loop_ is the expected value.

Good point...hmm...lots of the code in media has it backwards then.  Oh well.
whatever.

http://codereview.chromium.org/2745006/diff/23001/24018#newcode221
remoting/client/x11_client.cc:221: // TODO(hclam): Implement.
static const int please.

[Alpha Left Google, 2010-06-09 21:19:17]

http://codereview.chromium.org/2745006/diff/5001/6009
File remoting/client/decoder.h (right):

http://codereview.chromium.org/2745006/diff/5001/6009#newcode18
remoting/client/decoder.h:18: typedef std::vector<gfx::Rect> UpdatedRects;
On 2010/06/09 20:36:12, awong wrote:
> On 2010/06/09 19:53:18, Alpha wrote:
> > On 2010/06/09 18:51:30, garykac wrote:
> > > does it make sense to share this with the DirtyRects definition used in
the
> > > encoder/differ?
> > 
> > They are in a difference though. Maybe refactor later to pull them into
base/.
> 
> What's the difference?  Why not just call the thing RectList?
> 
> Add a TODO?

I meant different folder. Will add a TODO to refactor.

http://codereview.chromium.org/2745006/diff/5001/6010
File remoting/client/decoder_verbatim.cc (right):

http://codereview.chromium.org/2745006/diff/5001/6010#newcode70
remoting/client/decoder_verbatim.cc:70: updated_rects_->clear();
On 2010/06/09 20:36:12, awong wrote:
> On 2010/06/09 19:53:18, Alpha wrote:
> > On 2010/06/09 18:43:53, awong wrote:
> > > BTW, random thought...should we be using vectors for these things or
lists?
> > 
> > I don't think they make much difference in runtime performance though. Just
> not
> > a fan of list because of more weight (forward pointers).
> 
> On the otherhand, vectors double on a resize so they're not necessary more
> efficient...and I don't think clear resizes the vector downwards.  So for a
> long-lived vectors, we basically end up with max memory usage...
> 
> Anyways, just something to think about.  I doubt it matters much.

Actually vector.clear() sets capacity to 0. vector.resize(0) clears the content
but preserve capacity.