Clean up remoting project

Clean up remoting project

Cleaned up some file names so it simplifies our project, and gets us more inline with chromium standards.
Removed several unnecessary headers that were cluttering the remoting namespace.
Simplified some of the double pimpl implementations that we had on Linux to hide X11 stuff.
Got HostAuthentication working reasonably well as a mock.

BUG=NONE
TEST=BUILD

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

[Wez, 2011-03-31 12:06:05]

Shouldn't there be corresponding D entries for capturer_gdi.*?

Is the "pam"->"linux" rename really appropriate?  That authenticator only does
PAM, not NIS/NIS+, /etc/passwd or any other Linux-supported authentication
scheme, and it should work on any platform that has PAM, not just Linux.

[Lambros (do not use), 2011-03-31 14:11:32]

I agree that UserAuthenticatorPam, not UserAuthenticatorLinux is the proper
name for that class.  I think of it as a "technology-specific" class, rather
than a "platform-specific" class.

I think, if the implementation is really tied to one specific platform, it
could be named (for example) UserAuthenticatorWin.  Otherwise, use the
technology as the name, rather than the platform.  Then each platform's
authenticator just pulls whatever auth classes it wants to use internally
(possibly chaining multiple classes together).

For example, the Linux authenticator might just use PAM, and the Solaris
authenticator might use PAM plus some other method if PAM wasn't available,
etc.

This leaves the conundrum of having the class named differently from the
.h+.cxx files.  I'd be prepared to say LGTM for this, but only if I'm
allowed to take the same liberty myself in future! :-)

Lambros


On Thu, Mar 31, 2011 at 1:06 PM, <wez@chromium.org> wrote:

> Shouldn't there be corresponding D entries for capturer_gdi.*?
>
> Is the "pam"->"linux" rename really appropriate?  That authenticator only
> does
> PAM, not NIS/NIS+, /etc/passwd or any other Linux-supported authentication
> scheme, and it should work on any platform that has PAM, not just Linux.
>
>
>
> http://codereview.chromium.org/6780014/
>

[dmaclach1, 2011-03-31 14:36:54]

On Thu, Mar 31, 2011 at 7:11 AM, Lambros Lambrou
<lambroslambrou@google.com> wrote:
> I agree that UserAuthenticatorPam, not UserAuthenticatorLinux is the proper
> name for that class.  I think of it as a "technology-specific" class, rather
> than a "platform-specific" class.
> I think, if the implementation is really tied to one specific platform, it
> could be named (for example) UserAuthenticatorWin.  Otherwise, use the
> technology as the name, rather than the platform.  Then each platform's
> authenticator just pulls whatever auth classes it wants to use internally
> (possibly chaining multiple classes together).
> For example, the Linux authenticator might just use PAM, and the Solaris
> authenticator might use PAM plus some other method if PAM wasn't available,
> etc.
> This leaves the conundrum of having the class named differently from the
> .h+.cxx files.  I'd be prepared to say LGTM for this, but only if I'm
> allowed to take the same liberty myself in future! :-)
> Lambros
>
> On Thu, Mar 31, 2011 at 1:06 PM, <wez@chromium.org> wrote:
>>
>> Shouldn't there be corresponding D entries for capturer_gdi.*?
>>
>> Is the "pam"->"linux" rename really appropriate?  That authenticator only
>> does
>> PAM, not NIS/NIS+, /etc/passwd or any other Linux-supported authentication
>> scheme, and it should work on any platform that has PAM, not just Linux.
>>
>>
>> http://codereview.chromium.org/6780014/
>
>

In chromium "linux" denotes any unix based platform in general aside
from MacOSX. The naming conventions for files are:

_win for classes that compile on windows, _mac for files that compile
on mac, _linux for files that compile on linux, and _posix for files
that compile on both linux and mac. Having them named this way means
that the files show up in the Windows and Mac project files, but don't
compile, making it easier for people to get at the files to look at
various implementations. Wez, what do you mean about D* entries?

Cheers,
Dave

[Wez, 2011-03-31 14:39:32]

On 2011/03/31 14:36:54, dmaclach1 wrote:
> _win for classes that compile on windows, _mac for files that compile
> on mac, _linux for files that compile on linux, and _posix for files
> that compile on both linux and mac.

In which case the file should be renamed to _posix, not _linux?

> Wez, what do you mean about D* entries?

I saw an A for _linux and D for _pam, and didn't realise that A+ for the other
files was equivalent.

[dmaclach1, 2011-03-31 15:13:09]

On Thu, Mar 31, 2011 at 7:39 AM,  <wez@chromium.org> wrote:
> On 2011/03/31 14:36:54, dmaclach1 wrote:
>>
>> _win for classes that compile on windows, _mac for files that compile
>> on mac, _linux for files that compile on linux, and _posix for files
>> that compile on both linux and mac.
>
> In which case the file should be renamed to _posix, not _linux?

No because it's a function of what platforms we want the files
compiled on, not which platforms they will compile on.

[Lambros (do not use), 2011-03-31 15:30:59]

On Thu, Mar 31, 2011 at 4:13 PM, David Maclachlan <dmaclach@google.com>wrote:

> On Thu, Mar 31, 2011 at 7:39 AM,  <wez@chromium.org> wrote:
> > On 2011/03/31 14:36:54, dmaclach1 wrote:
> >>
> >> _win for classes that compile on windows, _mac for files that compile
> >> on mac, _linux for files that compile on linux, and _posix for files
> >> that compile on both linux and mac.
> >
> > In which case the file should be renamed to _posix, not _linux?
>
> No because it's a function of what platforms we want the files
> compiled on, not which platforms they will compile on.
>

Does this mean that C++ class names are not required to match the filenames
of their implementations?

Dave, please could you point us to where you saw this policy, so we can look
at it in more detail?

[dmac, 2011-04-01 03:44:37]

On 2011/03/31 15:30:59, lambroslambrou wrote:
> On Thu, Mar 31, 2011 at 4:13 PM, David Maclachlan <dmaclach@google.com>wrote:
> 
> > On Thu, Mar 31, 2011 at 7:39 AM,  <mailto:wez@chromium.org> wrote:
> > > On 2011/03/31 14:36:54, dmaclach1 wrote:
> > >>
> > >> _win for classes that compile on windows, _mac for files that compile
> > >> on mac, _linux for files that compile on linux, and _posix for files
> > >> that compile on both linux and mac.
> > >
> > > In which case the file should be renamed to _posix, not _linux?
> >
> > No because it's a function of what platforms we want the files
> > compiled on, not which platforms they will compile on.
> >
> 
> Does this mean that C++ class names are not required to match the filenames
> of their implementations?
> 
> Dave, please could you point us to where you saw this policy, so we can look
> at it in more detail?

Completed the refactoring so you can see where I was going with this. The
platform implementations are hidden in a standard pimpl design. C++ class names
generally do match the filenames, when there is an exposed interface in a header
file. However you can find several examples in the chromium code base where
class implementations that are declared and defined within one source file do
not match the filename.

[dmac, 2011-04-01 03:46:36]

Added Simon and Jamie as I expanded into source files which they authored.

[simonmorris, 2011-04-01 10:29:33]

http://codereview.chromium.org/6780014/diff/10001/remoting/host/chromoting_ho...
File remoting/host/chromoting_host_unittest.cc (right):

http://codereview.chromium.org/6780014/diff/10001/remoting/host/chromoting_ho...
remoting/host/chromoting_host_unittest.cc:168: 
Nit: extra line.

http://codereview.chromium.org/6780014/diff/10001/remoting/host/chromoting_ho...
remoting/host/chromoting_host_unittest.cc:207:
scoped_refptr<MockUserAuthenticator> user_authenticator_;
It's sad to scatter refcounts throughout the code just to
make the unit test a little simpler. Maybe have functions
such as user_authenticator_pass_then_fail(), which
each return a differently tailored MockUserAuthenticator?

[Jamie, 2011-04-01 10:51:10]

One thing I'm not keen on with this refactoring is the fact that we've lost the
per-platform headers. When I see a .cc file, I naturally tend to look for a
corresponding .h, and there doesn't seem to be any compelling reason to break
that expectation here.

Specifically, if we ever want to support multiple implementation for any of
these objects (for example using the more efficient OpenGL-based capturer at the
Mac login screen, or the slower alternative to capture switched-out sessions),
then the current Create() pattern (which I'm not a fan of in general) is not
going to work; we'll need a factory pattern, which will need access to the
implementations. That's going to require undoing quite a lot of this CL.

There are also a couple of more specific nits:

http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_linu...
File remoting/host/capturer_linux_unittest.cc (right):

http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_linu...
remoting/host/capturer_linux_unittest.cc:5: #include
"testing/gtest/include/gtest/gtest.h"
Is this needed for this CL to compile? If not can it be separated out? This CL
is big enough as it is. Same goes for the equivalent line elsewhere.

http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_win.cc
File remoting/host/capturer_win.cc (right):

http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_win....
remoting/host/capturer_win.cc:110: // 3780 pixels per meter is equivalent to 96
DPI, typical on desktop monitors.
Why can't these be static?

http://codereview.chromium.org/6780014/diff/10001/remoting/host/host_mock_obj...
File remoting/host/host_mock_objects.cc (right):

http://codereview.chromium.org/6780014/diff/10001/remoting/host/host_mock_obj...
remoting/host/host_mock_objects.cc:34:
MockUserAuthenticator::MockUserAuthenticator() {}
Nit: Missing blank line.

[simonmorris, 2011-04-01 10:52:43]

http://codereview.chromium.org/6780014/diff/10001/remoting/host/chromoting_ho...
File remoting/host/chromoting_host_unittest.cc (right):

http://codereview.chromium.org/6780014/diff/10001/remoting/host/chromoting_ho...
remoting/host/chromoting_host_unittest.cc:207:
scoped_refptr<MockUserAuthenticator> user_authenticator_;
On 2011/04/01 10:29:34, simonmorris wrote:
> It's sad to scatter refcounts throughout the code just to
> make the unit test a little simpler. Maybe have functions
> such as user_authenticator_pass_then_fail(), which
> each return a differently tailored MockUserAuthenticator?

After a chat with Jamie, I realise that wouldn't work.
Maybe we need a UserAuthenticatorFactory class, with mocks?
Maybe in a different CL?

[dmac, 2011-04-01 15:30:42]

On 2011/04/01 10:51:10, Jamie wrote:
> One thing I'm not keen on with this refactoring is the fact that we've lost
the
> per-platform headers. When I see a .cc file, I naturally tend to look for a
> corresponding .h, and there doesn't seem to be any compelling reason to break
> that expectation here.

Interesting, because I dislike the per platform headers. In my opinion, headers
are for exposing interface. Once you get down to the actual nuts and bolts cross
platform issues, the contents of the per-platform header is really more
implementation detail than interface. That's what leads to the double impl
pattern that we were seeing on the linux side of things. I personally feel that
having the headers is just more code to keep track of, and in general more
visual clutter in a project. It sounds stupid, but the extra lines of code (even
as far as the copyrights that need to be maintained now sigh) just add extra
work. Also, being able to put stuff in unnamed namespaces cuts down on
cluttering up the namespace.

> 
> Specifically, if we ever want to support multiple implementation for any of
> these objects (for example using the more efficient OpenGL-based capturer at
the
> Mac login screen, or the slower alternative to capture switched-out sessions),
> then the current Create() pattern (which I'm not a fan of in general) is not
> going to work; we'll need a factory pattern, which will need access to the
> implementations. That's going to require undoing quite a lot of this CL.

I don't see how "create" is not already factory pattern. I think in the cross
platform case if you are going to have multiple implementations on a single
platform (which I feel is unlikely) these can still be hidden in a single impl
file. The factory is already in there with the implementation. There may be the
odd case where we want to split those out, but I think those are going to be in
the minority.

> There are also a couple of more specific nits:
> 
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_linu...
> File remoting/host/capturer_linux_unittest.cc (right):
> 
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_linu...
> remoting/host/capturer_linux_unittest.cc:5: #include
> "testing/gtest/include/gtest/gtest.h"
> Is this needed for this CL to compile? If not can it be separated out? This CL
> is big enough as it is. Same goes for the equivalent line elsewhere.

Sure. I was just minimizing the amount of header information we pulled in to
this file. It doesn't use mocks currently, so there's no reason to parse all of
the mock include code.

> 
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_win.cc
> File remoting/host/capturer_win.cc (right):
> 
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_win....
> remoting/host/capturer_win.cc:110: // 3780 pixels per meter is equivalent to
96
> DPI, typical on desktop monitors.
> Why can't these be static?

AFAIK by putting them in the unnamed namespace they are static, and the keyword
is redundant.

> 
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/host_mock_obj...
> File remoting/host/host_mock_objects.cc (right):
> 
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/host_mock_obj...
> remoting/host/host_mock_objects.cc:34:
> MockUserAuthenticator::MockUserAuthenticator() {}
> Nit: Missing blank line.

Will fix.

[dmac, 2011-04-01 15:34:32]

On 2011/04/01 10:52:43, simonmorris wrote:
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/chromoting_ho...
> File remoting/host/chromoting_host_unittest.cc (right):
> 
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/chromoting_ho...
> remoting/host/chromoting_host_unittest.cc:207:
> scoped_refptr<MockUserAuthenticator> user_authenticator_;
> On 2011/04/01 10:29:34, simonmorris wrote:
> > It's sad to scatter refcounts throughout the code just to
> > make the unit test a little simpler. Maybe have functions
> > such as user_authenticator_pass_then_fail(), which
> > each return a differently tailored MockUserAuthenticator?
> 
> After a chat with Jamie, I realise that wouldn't work.
> Maybe we need a UserAuthenticatorFactory class, with mocks?
> Maybe in a different CL?

Yeah, I wasn't 100% happy with the ref count either, but didn't want to
over-engineer a solution. The factory class seemed like writing a lot of code
just to make things "feel" better, without actually gaining us much. I'm
certainly willing to be overruled but the refcount was fast to write, known
technology that didn't add any code to our codebase at the cost of adding a
refcount to the class. The factory would require writing a block of code and
restructuring the call-sites.

[Lambros, 2011-04-01 15:54:00]

I have nothing major to add to others' comments.  If we do decide to keep the
ref-counting stuff, we should consider using the thread-safe version, or
documenting why the unsafe version is OK.

http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_linu...
File remoting/host/capturer_linux.cc (right):

http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_linu...
remoting/host/capturer_linux.cc:89: uint8* last_buffer_;
Nit: Maybe a bit too pedantic, but do we want DISALLOW_COPY_AND_ASSIGN here, and
in other similar classes?

http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_linu...
File remoting/host/capturer_linux_unittest.cc (right):

http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_linu...
remoting/host/capturer_linux_unittest.cc:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
Nit: 2011? (check other files as well)

http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_mac.cc
File remoting/host/capturer_mac.cc (right):

http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_mac....
remoting/host/capturer_mac.cc:8: #include <OpenGL/OpenGL.h>
Nit: alphabetize

http://codereview.chromium.org/6780014/diff/10001/remoting/host/chromoting_ho...
File remoting/host/chromoting_host_unittest.cc (right):

http://codereview.chromium.org/6780014/diff/10001/remoting/host/chromoting_ho...
remoting/host/chromoting_host_unittest.cc:97: connection_ = new
MockConnectionToClient(
Nit: indentation

http://codereview.chromium.org/6780014/diff/10001/remoting/host/desktop_envir...
File remoting/host/desktop_environment.cc (right):

http://codereview.chromium.org/6780014/diff/10001/remoting/host/desktop_envir...
remoting/host/desktop_environment.cc:7: #include "remoting/host/capturer.h"
Nit: These three #includes might not be needed.

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

http://codereview.chromium.org/6780014/diff/10001/remoting/host/event_executo...
remoting/host/event_executor_linux.cc:368:
DISABLE_RUNNABLE_METHOD_REFCOUNT(remoting::EventExecutorLinux);
Is this OK here?  Do we know this object will outlive any tasks posted to it?

http://codereview.chromium.org/6780014/diff/10001/remoting/host/event_executo...
File remoting/host/event_executor_mac.cc (right):

http://codereview.chromium.org/6780014/diff/10001/remoting/host/event_executo...
remoting/host/event_executor_mac.cc:13: #include "base/mac/scoped_cftyperef.h"
Nit: alphabetize

http://codereview.chromium.org/6780014/diff/10001/remoting/host/event_executo...
remoting/host/event_executor_mac.cc:16: #include "remoting/proto/internal.pb.h"
Nit: proto before protocol?

http://codereview.chromium.org/6780014/diff/10001/remoting/host/host_mock_obj...
File remoting/host/host_mock_objects.cc (right):

http://codereview.chromium.org/6780014/diff/10001/remoting/host/host_mock_obj...
remoting/host/host_mock_objects.cc:13: MockCurtain::MockCurtain() { }
Nit: remove spaces between {} for consistency

[Jamie, 2011-04-01 15:57:04]

> Interesting, because I dislike the per platform headers. In my opinion,
headers
> are for exposing interface. Once you get down to the actual nuts and bolts
cross
> platform issues, the contents of the per-platform header is really more
> implementation detail than interface. That's what leads to the double impl
> pattern that we were seeing on the linux side of things. I personally feel
that
> having the headers is just more code to keep track of, and in general more
> visual clutter in a project. It sounds stupid, but the extra lines of code
(even
> as far as the copyrights that need to be maintained now sigh) just add extra
> work. Also, being able to put stuff in unnamed namespaces cuts down on
> cluttering up the namespace.

Those are all fair points.

> > Specifically, if we ever want to support multiple implementation for any of
> > these objects (for example using the more efficient OpenGL-based capturer at
> the
> > Mac login screen, or the slower alternative to capture switched-out
sessions),
> > then the current Create() pattern (which I'm not a fan of in general) is not
> > going to work; we'll need a factory pattern, which will need access to the
> > implementations. That's going to require undoing quite a lot of this CL.
> 
> I don't see how "create" is not already factory pattern. I think in the cross
> platform case if you are going to have multiple implementations on a single
> platform (which I feel is unlikely) these can still be hidden in a single impl
> file. The factory is already in there with the implementation. There may be
the
> odd case where we want to split those out, but I think those are going to be
in
> the minority.

I'm almost certain that we will need more than one implementation for the Mac
capturer. Of course, that doesn't necessarily mean more than one class, but that
would be my preferred implementation if I wasn't constrained by our use of
Create. You're right of course that Create is a form of factory pattern, but
it's a particularly inflexible one. I've been bitten by a similar design in the
past so I'm a bit wary of it :) However, I'm not suggesting we get rid of it in
this CL, just that this CL entrenches it more deeply than I would like.

>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_linu...
> > File remoting/host/capturer_linux_unittest.cc (right):
> > 
> >
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_linu...
> > remoting/host/capturer_linux_unittest.cc:5: #include
> > "testing/gtest/include/gtest/gtest.h"
> > Is this needed for this CL to compile? If not can it be separated out? This
CL
> > is big enough as it is. Same goes for the equivalent line elsewhere.
> 
> Sure. I was just minimizing the amount of header information we pulled in to
> this file. It doesn't use mocks currently, so there's no reason to parse all
of
> the mock include code.

Fair enough.

>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_win.cc
> > File remoting/host/capturer_win.cc (right):
> > 
> >
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_win....
> > remoting/host/capturer_win.cc:110: // 3780 pixels per meter is equivalent to
> 96
> > DPI, typical on desktop monitors.
> > Why can't these be static?
> 
> AFAIK by putting them in the unnamed namespace they are static, and the
keyword
> is redundant.

You're probably right--code outside this file can't refer to it by name, so
static linkage is the only thing that makes sense. On the other hand, it's not
obvious from the placement of these that they are inside an anonymous namespace,
so I think there's value in making it explicit.

Cheers,
Jamie

[simonmorris, 2011-04-01 16:17:03]

On 2011/04/01 15:34:32, dmac wrote:
> On 2011/04/01 10:52:43, simonmorris wrote:
> >
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/chromoting_ho...
> > File remoting/host/chromoting_host_unittest.cc (right):
> > 
> >
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/chromoting_ho...
> > remoting/host/chromoting_host_unittest.cc:207:
> > scoped_refptr<MockUserAuthenticator> user_authenticator_;
> > On 2011/04/01 10:29:34, simonmorris wrote:
> > > It's sad to scatter refcounts throughout the code just to
> > > make the unit test a little simpler. Maybe have functions
> > > such as user_authenticator_pass_then_fail(), which
> > > each return a differently tailored MockUserAuthenticator?
> > 
> > After a chat with Jamie, I realise that wouldn't work.
> > Maybe we need a UserAuthenticatorFactory class, with mocks?
> > Maybe in a different CL?
> 
> Yeah, I wasn't 100% happy with the ref count either, but didn't want to
> over-engineer a solution. The factory class seemed like writing a lot of code
> just to make things "feel" better, without actually gaining us much. I'm
> certainly willing to be overruled but the refcount was fast to write, known
> technology that didn't add any code to our codebase at the cost of adding a
> refcount to the class. The factory would require writing a block of code and
> restructuring the call-sites.

It wasn't obvious to me why the UserAuthenticator needed to be ref-counted.
In the main code, a UserAuthenticator has a short, well-defined lifetime:
the opposite of the usage pattern that suggests ref-counting. Only in the
unit test does it have a longer lifetime. It's better for the unit test usage to
be similar to the real code usage. Changing an API to help unit tests often
makes
it better for everyone, but here I think the tail's wagging the dog.

Having said that, I've haven't used ref-counting much, so maybe I'm
overestimating
the readability hit of ref-counting, for the average developer.

[dmac, 2011-04-01 21:15:07]

I think I addressed all the style concerns.

http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_linu...
File remoting/host/capturer_linux.cc (right):

http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_linu...
remoting/host/capturer_linux.cc:89: uint8* last_buffer_;
On 2011/04/01 15:54:00, Lambros wrote:
> Nit: Maybe a bit too pedantic, but do we want DISALLOW_COPY_AND_ASSIGN here,
and
> in other similar classes?

Done.

http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_linu...
File remoting/host/capturer_linux_unittest.cc (right):

http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_linu...
remoting/host/capturer_linux_unittest.cc:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
On 2011/04/01 15:54:00, Lambros wrote:
> Nit: 2011? (check other files as well)

Done.

http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_mac.cc
File remoting/host/capturer_mac.cc (right):

http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_mac....
remoting/host/capturer_mac.cc:8: #include <OpenGL/OpenGL.h>
On 2011/04/01 15:54:00, Lambros wrote:
> Nit: alphabetize

Done.

http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_win.cc
File remoting/host/capturer_win.cc (right):

http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_win....
remoting/host/capturer_win.cc:110: // 3780 pixels per meter is equivalent to 96
DPI, typical on desktop monitors.
On 2011/04/01 10:51:10, Jamie wrote:
> Why can't these be static?

I turned them back to static.

http://codereview.chromium.org/6780014/diff/10001/remoting/host/chromoting_ho...
File remoting/host/chromoting_host_unittest.cc (right):

http://codereview.chromium.org/6780014/diff/10001/remoting/host/chromoting_ho...
remoting/host/chromoting_host_unittest.cc:97: connection_ = new
MockConnectionToClient(
On 2011/04/01 15:54:00, Lambros wrote:
> Nit: indentation

Done.

http://codereview.chromium.org/6780014/diff/10001/remoting/host/chromoting_ho...
remoting/host/chromoting_host_unittest.cc:168: 
On 2011/04/01 10:29:34, simonmorris wrote:
> Nit: extra line.

Done.

http://codereview.chromium.org/6780014/diff/10001/remoting/host/desktop_envir...
File remoting/host/desktop_environment.cc (right):

http://codereview.chromium.org/6780014/diff/10001/remoting/host/desktop_envir...
remoting/host/desktop_environment.cc:7: #include "remoting/host/capturer.h"
On 2011/04/01 15:54:00, Lambros wrote:
> Nit: These three #includes might not be needed.

Sadly they are required for the destructor to compile.

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

http://codereview.chromium.org/6780014/diff/10001/remoting/host/event_executo...
remoting/host/event_executor_linux.cc:368:
DISABLE_RUNNABLE_METHOD_REFCOUNT(remoting::EventExecutorLinux);
On 2011/04/01 15:54:00, Lambros wrote:
> Is this OK here?  Do we know this object will outlive any tasks posted to it?

This is how it was before. I just moved the macro.

http://codereview.chromium.org/6780014/diff/10001/remoting/host/event_executo...
File remoting/host/event_executor_mac.cc (right):

http://codereview.chromium.org/6780014/diff/10001/remoting/host/event_executo...
remoting/host/event_executor_mac.cc:13: #include "base/mac/scoped_cftyperef.h"
On 2011/04/01 15:54:00, Lambros wrote:
> Nit: alphabetize

Done.

http://codereview.chromium.org/6780014/diff/10001/remoting/host/event_executo...
remoting/host/event_executor_mac.cc:16: #include "remoting/proto/internal.pb.h"
On 2011/04/01 15:54:00, Lambros wrote:
> Nit: proto before protocol?

Done.

http://codereview.chromium.org/6780014/diff/10001/remoting/host/host_mock_obj...
File remoting/host/host_mock_objects.cc (right):

http://codereview.chromium.org/6780014/diff/10001/remoting/host/host_mock_obj...
remoting/host/host_mock_objects.cc:13: MockCurtain::MockCurtain() { }
On 2011/04/01 15:54:00, Lambros wrote:
> Nit: remove spaces between {} for consistency

Done.

http://codereview.chromium.org/6780014/diff/10001/remoting/host/host_mock_obj...
remoting/host/host_mock_objects.cc:34:
MockUserAuthenticator::MockUserAuthenticator() {}
On 2011/04/01 10:51:10, Jamie wrote:
> Nit: Missing blank line.

Done.

[simonmorris, 2011-04-04 10:44:25]

On 2011/04/01 16:17:03, simonmorris wrote:
> On 2011/04/01 15:34:32, dmac wrote:
> > On 2011/04/01 10:52:43, simonmorris wrote:
> > >
> >
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/chromoting_ho...
> > > File remoting/host/chromoting_host_unittest.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/chromoting_ho...
> > > remoting/host/chromoting_host_unittest.cc:207:
> > > scoped_refptr<MockUserAuthenticator> user_authenticator_;
> > > On 2011/04/01 10:29:34, simonmorris wrote:
> > > > It's sad to scatter refcounts throughout the code just to
> > > > make the unit test a little simpler. Maybe have functions
> > > > such as user_authenticator_pass_then_fail(), which
> > > > each return a differently tailored MockUserAuthenticator?
> > > 
> > > After a chat with Jamie, I realise that wouldn't work.
> > > Maybe we need a UserAuthenticatorFactory class, with mocks?
> > > Maybe in a different CL?
> > 
> > Yeah, I wasn't 100% happy with the ref count either, but didn't want to
> > over-engineer a solution. The factory class seemed like writing a lot of
code
> > just to make things "feel" better, without actually gaining us much. I'm
> > certainly willing to be overruled but the refcount was fast to write, known
> > technology that didn't add any code to our codebase at the cost of adding a
> > refcount to the class. The factory would require writing a block of code and
> > restructuring the call-sites.
> 
> It wasn't obvious to me why the UserAuthenticator needed to be ref-counted.
> In the main code, a UserAuthenticator has a short, well-defined lifetime:
> the opposite of the usage pattern that suggests ref-counting. Only in the
> unit test does it have a longer lifetime. It's better for the unit test usage
to
> be similar to the real code usage. Changing an API to help unit tests often
> makes
> it better for everyone, but here I think the tail's wagging the dog.
> 
> Having said that, I've haven't used ref-counting much, so maybe I'm
> overestimating
> the readability hit of ref-counting, for the average developer.

LGTM, if you still prefer the refcounts.

[Jamie, 2011-04-04 10:48:30]

On 2011/04/01 21:15:07, dmac wrote:
> I think I addressed all the style concerns.
> 
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_linu...
> File remoting/host/capturer_linux.cc (right):
> 
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_linu...
> remoting/host/capturer_linux.cc:89: uint8* last_buffer_;
> On 2011/04/01 15:54:00, Lambros wrote:
> > Nit: Maybe a bit too pedantic, but do we want DISALLOW_COPY_AND_ASSIGN here,
> and
> > in other similar classes?
> 
> Done.
> 
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_linu...
> File remoting/host/capturer_linux_unittest.cc (right):
> 
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_linu...
> remoting/host/capturer_linux_unittest.cc:1: // Copyright (c) 2010 The Chromium
> Authors. All rights reserved.
> On 2011/04/01 15:54:00, Lambros wrote:
> > Nit: 2011? (check other files as well)
> 
> Done.
> 
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_mac.cc
> File remoting/host/capturer_mac.cc (right):
> 
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_mac....
> remoting/host/capturer_mac.cc:8: #include <OpenGL/OpenGL.h>
> On 2011/04/01 15:54:00, Lambros wrote:
> > Nit: alphabetize
> 
> Done.
> 
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_win.cc
> File remoting/host/capturer_win.cc (right):
> 
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/capturer_win....
> remoting/host/capturer_win.cc:110: // 3780 pixels per meter is equivalent to
96
> DPI, typical on desktop monitors.
> On 2011/04/01 10:51:10, Jamie wrote:
> > Why can't these be static?
> 
> I turned them back to static.
> 
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/chromoting_ho...
> File remoting/host/chromoting_host_unittest.cc (right):
> 
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/chromoting_ho...
> remoting/host/chromoting_host_unittest.cc:97: connection_ = new
> MockConnectionToClient(
> On 2011/04/01 15:54:00, Lambros wrote:
> > Nit: indentation
> 
> Done.
> 
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/chromoting_ho...
> remoting/host/chromoting_host_unittest.cc:168: 
> On 2011/04/01 10:29:34, simonmorris wrote:
> > Nit: extra line.
> 
> Done.
> 
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/desktop_envir...
> File remoting/host/desktop_environment.cc (right):
> 
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/desktop_envir...
> remoting/host/desktop_environment.cc:7: #include "remoting/host/capturer.h"
> On 2011/04/01 15:54:00, Lambros wrote:
> > Nit: These three #includes might not be needed.
> 
> Sadly they are required for the destructor to compile.
> 
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/event_executo...
> File remoting/host/event_executor_linux.cc (right):
> 
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/event_executo...
> remoting/host/event_executor_linux.cc:368:
> DISABLE_RUNNABLE_METHOD_REFCOUNT(remoting::EventExecutorLinux);
> On 2011/04/01 15:54:00, Lambros wrote:
> > Is this OK here?  Do we know this object will outlive any tasks posted to
it?
> 
> This is how it was before. I just moved the macro.
> 
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/event_executo...
> File remoting/host/event_executor_mac.cc (right):
> 
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/event_executo...
> remoting/host/event_executor_mac.cc:13: #include "base/mac/scoped_cftyperef.h"
> On 2011/04/01 15:54:00, Lambros wrote:
> > Nit: alphabetize
> 
> Done.
> 
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/event_executo...
> remoting/host/event_executor_mac.cc:16: #include
"remoting/proto/internal.pb.h"
> On 2011/04/01 15:54:00, Lambros wrote:
> > Nit: proto before protocol?
> 
> Done.
> 
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/host_mock_obj...
> File remoting/host/host_mock_objects.cc (right):
> 
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/host_mock_obj...
> remoting/host/host_mock_objects.cc:13: MockCurtain::MockCurtain() { }
> On 2011/04/01 15:54:00, Lambros wrote:
> > Nit: remove spaces between {} for consistency
> 
> Done.
> 
>
http://codereview.chromium.org/6780014/diff/10001/remoting/host/host_mock_obj...
> remoting/host/host_mock_objects.cc:34:
> MockUserAuthenticator::MockUserAuthenticator() {}
> On 2011/04/01 10:51:10, Jamie wrote:
> > Nit: Missing blank line.
> 
> Done.

LGTM.

[Lambros, 2011-04-04 15:21:30]

LGTM

[dmaclach1, 2011-04-04 17:36:38]

Lambros, for the user authenticator, why are we passing in a creator
function as opposed to just passing in the created object like we do
with all the rest of our APIs?

[Lambros (do not use), 2011-04-04 18:05:34]

I'm not aware that I was "passing in a creator function".  Please could you
point me to the lines of code where you think I'm passing function pointers
around?


On Mon, Apr 4, 2011 at 6:36 PM, David Maclachlan <dmaclach@google.com>wrote:

> Lambros, for the user authenticator, why are we passing in a creator
> function as opposed to just passing in the created object like we do
> with all the rest of our APIs?
>

[dmac, 2011-04-04 21:56:36]

Sorry.. creator function was "loose" terminology to say the least. I meant the
bound function that was passed into clientsession. Take a look at this latest
change and see what you think. The big thing is that I have removed the
constraint on an instance of user_authenticator that it only be called once.
Getting rid of that constraint simplified things considerably, and it didn't
appear to be required. Please educate me if I messed up here somewhere. Even if
the constraint is required, it looked like we could hide it behind the user
authentication API since "authenticate" is the only method of note at this
point.

On 2011/04/04 18:05:34, lambroslambrou wrote:
> I'm not aware that I was "passing in a creator function".  Please could you
> point me to the lines of code where you think I'm passing function pointers
> around?
> 
> 
> On Mon, Apr 4, 2011 at 6:36 PM, David Maclachlan <dmaclach@google.com>wrote:
> 
> > Lambros, for the user authenticator, why are we passing in a creator
> > function as opposed to just passing in the created object like we do
> > with all the rest of our APIs?
> >

[Lambros, 2011-04-04 22:40:50]

LGTM on the UserAuth changes.